From 2a1ba730cb614ec180dfc0ff70af00e819bcb4f8 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 21 May 2026 12:41:31 -0700 Subject: [PATCH 01/28] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20feat:=20Lock=20YA?= =?UTF-8?q?ML-Defined=20MCP=20Servers=20In=20Admin=20Panel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detects servers that come from the YAML config by reading the un-merged base config keys threaded from getBaseConfigFn's new yamlMcpKeys signal, so admin overrides on cosmetic fields like title or iconPath no longer flip the detection and re-enable rename or delete on YAML-defined servers. For YAML-source servers the rename pencil and trash button are hidden and the transport-type selector is disabled because changing it breaks the Zod union. All other fields including url, command, args, env, stderr, headers, apiKey, and oauth remain editable to support the multi-tenant model where YAML provides global defaults and the admin panel is the per-tenant customization surface. Inspector-derived fields are hidden from the editor since admin overrides of those are silently ignored at the registry layer. MCP entry edits now write per-leaf paths under mcpServers.. so single-field reset, baseline-equality dirty detection, and rename orphan cleanup all work correctly through the existing global Save bar. The rename path enumerates leaves recursively so nested per-leaf data like headers and oauth survive rename intact. Dotted server names are rejected at create and rename time because the path model treats dots as delimiters, and a custom anyOfSchema wrapper resolves Zod union sub-schemas during per-field validation without relying on a specific zod major version. --- src/components/configuration/ConfigPage.tsx | 110 ++- .../configuration/ConfigTabContent.tsx | 5 +- .../sections/McpServersRenderer.tsx | 611 +++++++++++++--- .../__tests__/McpServersRenderer.test.tsx | 686 ++++++++++++++++++ .../configuration/sections/mcpFieldMeta.ts | 41 ++ src/locales/en/translation.json | 2 + src/server/config.test.ts | 61 +- src/server/config.ts | 106 ++- src/types/config-ui.ts | 16 + 9 files changed, 1488 insertions(+), 150 deletions(-) create mode 100644 src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx create mode 100644 src/components/configuration/sections/mcpFieldMeta.ts diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 085b4a0..bc3c76e 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -130,6 +130,21 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return new Set(Object.keys(flattenObject(dbOverrides))); }, [dbOverrides]); + /** + * Authoritative set of YAML-defined entries per section, sourced from the + * un-merged baseOnly response. Identity affordances (rename, delete) are + * locked iff a name appears here, regardless of whether admin overrides + * also exist for it. + */ + const baseRecordKeys = useMemo(() => { + const result: Record> = {}; + const yamlMcpKeys = baseConfigData?.yamlMcpKeys; + if (yamlMcpKeys && Array.isArray(yamlMcpKeys)) { + result.mcpServers = new Set(yamlMcpKeys); + } + return result; + }, [baseConfigData]); + const hasUnmappedSections = useMemo( () => schemaTree.some( @@ -344,41 +359,70 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return scopeResolvedValues ?? {}; }, [isEditingScope, flatBaseline, scopeResolvedValues]); + /** + * Every intermediate (non-leaf) path implied by the baseline's flat keys. + * Used by handleFieldChange to recognize "delete the whole subtree" writes + * whose target path doesn't itself appear in scopeBaseline because + * flatBaseline only stores leaf paths. + */ + const baselineIntermediates = useMemo(() => { + const set = new Set(); + for (const leaf of Object.keys(scopeBaseline)) { + const parts = leaf.split('.'); + for (let i = 1; i < parts.length; i++) { + set.add(parts.slice(0, i).join('.')); + } + } + return set; + }, [scopeBaseline]); + const handleFieldChange = useCallback( (path: string, value: t.ConfigValue) => { - startTransition(() => { - setTouchedPaths((prev) => { - if (prev.has(path)) return prev; - const next = new Set(prev); - next.add(path); + setTouchedPaths((prev) => { + if (prev.has(path)) return prev; + const next = new Set(prev); + next.add(path); + return next; + }); + setEditedValues((prev) => { + const baseline = scopeBaseline[path]; + const match = + value === baseline || + (typeof value === 'object' && + typeof baseline === 'object' && + JSON.stringify(value) === JSON.stringify(baseline)); + /** + * Suppress the prune for undefined writes that target an intermediate + * container path. handleRemove/handleRename emit these to request + * "delete the whole subtree"; pruning would silently drop them + * because flatBaseline only stores leaf paths, so scopeBaseline at + * the container is always undefined and would falsely match. + */ + const isContainerDelete = value === undefined && baselineIntermediates.has(path); + if (match && !isContainerDelete) { + const next = { ...prev }; + delete next[path]; return next; - }); - setEditedValues((prev) => { - const baseline = scopeBaseline[path]; - const match = - value === baseline || - (typeof value === 'object' && - typeof baseline === 'object' && - JSON.stringify(value) === JSON.stringify(baseline)); - if (match) { - const next = { ...prev }; - delete next[path]; - return next; + } + const next = { ...prev, [path]: value }; + if (Array.isArray(value)) { + const prefix = `${path}.`; + for (const k of Object.keys(next)) { + if (k.startsWith(prefix) && /\.\d+$/.test(k)) delete next[k]; } - const next = { ...prev, [path]: value }; - if (Array.isArray(value)) { - const prefix = `${path}.`; - for (const k of Object.keys(next)) { - if (k.startsWith(prefix) && /\.\d+$/.test(k)) delete next[k]; - } + } + const indexMatch = /^(.+)\.\d+$/.exec(path); + if (indexMatch) delete next[indexMatch[1]]; + for (const existing of Object.keys(next)) { + if (existing === path) continue; + if (path.startsWith(`${existing}.`)) { + delete next[existing]; } - const indexMatch = /^(.+)\.\d+$/.exec(path); - if (indexMatch) delete next[indexMatch[1]]; - return next; - }); + } + return next; }); }, - [scopeBaseline], + [scopeBaseline, baselineIntermediates], ); const isDirty = Object.keys(editedValues).length > 0; @@ -457,7 +501,9 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi .filter((p) => editedValues[p] !== undefined) .map((p) => ({ fieldPath: p, - value: /\.\d+$/.test(p) ? deepSerializeKVPairs(editedValues[p]) : serializeKVPairs(editedValues[p]), + value: /\.\d+$/.test(p) + ? deepSerializeKVPairs(editedValues[p]) + : serializeKVPairs(editedValues[p]), })); const resets = touched.filter((p) => editedValues[p] === undefined); @@ -540,7 +586,10 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const segments = path.split('.'); let current: t.ConfigValue = configValues; for (const seg of segments) { - if (current == null || typeof current !== 'object') { current = undefined; break; } + if (current == null || typeof current !== 'object') { + current = undefined; + break; + } current = Array.isArray(current) ? (current as t.ConfigValue[])[Number(seg)] : (current as Record)[seg]; @@ -827,6 +876,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi sectionPermissions={sectionPermissions} schemaDefaults={schemaDefaults} showConfiguredOnly={showConfiguredOnly} + baseRecordKeys={baseRecordKeys} /> diff --git a/src/components/configuration/ConfigTabContent.tsx b/src/components/configuration/ConfigTabContent.tsx index 7ce6520..8e94f20 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -54,6 +54,7 @@ export function ConfigTabContent({ sectionPermissions, schemaDefaults, showConfiguredOnly, + baseRecordKeys, }: t.ConfigTabContentProps) { const localize = useLocalize(); const fieldsDisabled = readOnly; @@ -164,6 +165,7 @@ export function ConfigTabContent({ getValue, onChange: onFieldChange, onResetField, + editedValues, disabled: sectionDisabled, profileMap, previewMode, @@ -179,6 +181,7 @@ export function ConfigTabContent({ pendingResets, schemaDefaults, showConfiguredOnly, + yamlBaseKeys: baseRecordKeys?.[dataKey], }; return ( <> @@ -232,7 +235,7 @@ export function ConfigTabContent({ const isInlineSection = (section: t.ConfigSectionConfig): boolean => Boolean( (section.sectionField && isSimpleScalar(section.sectionField)) || - (section.fields.length === 1 && isSimpleScalar(section.fields[0])), + (section.fields.length === 1 && isSimpleScalar(section.fields[0])), ); type SectionGroup = diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index ca71181..f187e9d 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -6,12 +6,17 @@ * - Semantic field groups (Connection, Authentication, Server Options, Advanced) * - A "Create MCP Server" dialog for adding new entries * - TOC-compatible scroll targets via entry card IDs + * + * All edits write per-leaf paths (`mcpServers..`) into editedValues + * so single-field reset, baseline-equality dirty pruning, and rename orphan + * cleanup all work uniformly. */ import { Icon } from '@clickhouse/click-ui'; -import { useState, useCallback } from 'react'; +import { memo, useRef, useMemo, useState, useEffect, useCallback } from 'react'; import type { ReactNode } from 'react'; import type * as t from '@/types'; +import { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED } from './mcpFieldMeta'; import { useCollapsibleSection } from '../useCollapsibleSection'; import { ObjectEntryCard } from '../fields/ObjectEntryCard'; import { renderCollapsible } from '../renderCollapsible'; @@ -97,6 +102,86 @@ function withFieldOverrides(field: t.SchemaField, transportType: string): t.Sche return field; } +// --------------------------------------------------------------------------- +// Per-leaf edit overlay helpers +// --------------------------------------------------------------------------- + +function setLeaf( + target: Record, + segments: string[], + value: t.ConfigValue, +): void { + let cursor: Record = target; + for (let i = 0; i < segments.length - 1; i++) { + const seg = segments[i]; + const next = cursor[seg]; + if (next == null || typeof next !== 'object' || Array.isArray(next)) { + cursor[seg] = {}; + } + cursor = cursor[seg] as Record; + } + const leaf = segments[segments.length - 1]; + if (value === undefined) { + delete cursor[leaf]; + } else { + cursor[leaf] = value; + } +} + +function applyLeafOverlay( + base: Record, + leafEdits: Array<[string[], t.ConfigValue]>, +): Record { + const cloned = deepClone(base); + for (const [segments, value] of leafEdits) { + setLeaf(cloned, segments, value); + } + return cloned; +} + +function deepClone(value: Record): Record { + const result: Record = {}; + for (const [k, v] of Object.entries(value)) { + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + result[k] = deepClone(v as Record); + } else if (Array.isArray(v)) { + result[k] = v.slice(); + } else { + result[k] = v; + } + } + return result; +} + +function isPlainObject(value: t.ConfigValue): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** + * Walks a nested record and returns one entry per leaf (primitive or array). + * Used by rename to enumerate every leaf path beneath an MCP entry so nested + * structures like `headers.Authorization` survive the rename intact instead of + * collapsing into one whole-object write. + */ +function enumerateLeafPaths( + obj: Record, + prefix: string[] = [], + seen: WeakSet = new WeakSet(), +): Array<{ segments: string[]; value: t.ConfigValue }> { + if (seen.has(obj)) return []; + seen.add(obj); + const out: Array<{ segments: string[]; value: t.ConfigValue }> = []; + for (const [k, v] of Object.entries(obj)) { + const next = [...prefix, k]; + if (isPlainObject(v)) { + out.push(...enumerateLeafPaths(v, next, seen)); + } else { + out.push({ segments: next, value: v }); + } + } + return out; +} + // --------------------------------------------------------------------------- // Semantic field groups // --------------------------------------------------------------------------- @@ -162,14 +247,13 @@ function flattenGroupFields( transportType: string, disabled?: boolean, collectionRenderOverrides?: Record, + lockedKeys?: Set, ): ReactNode[] { - const values = - typeof parentValue === 'object' && parentValue !== null && !Array.isArray(parentValue) - ? (parentValue as Record) - : {}; + const values = isPlainObject(parentValue) ? parentValue : {}; const nodes: ReactNode[] = []; for (const field of fields) { + const fieldDisabled = disabled || (lockedKeys?.has(field.key) ?? false); // Custom render for transport type select — curated options with lowercase labels. // When `type` is omitted (common in YAML configs), infer it from other fields // to mirror backend Zod union resolution. @@ -194,7 +278,7 @@ function flattenGroupFields( value={displayValue} options={TRANSPORT_TYPE_OPTIONS} onChange={(v) => onChange(field.key, v)} - disabled={disabled} + disabled={fieldDisabled} aria-label={label} /> @@ -205,10 +289,7 @@ function flattenGroupFields( if (field.children && field.children.length > 0 && !field.isArray && field.type !== 'record') { const nested = values[field.key]; - const nestedObj = - typeof nested === 'object' && nested !== null && !Array.isArray(nested) - ? (nested as Record) - : {}; + const nestedObj = isPlainObject(nested) ? nested : {}; for (const child of field.children) { nodes.push( renderInlineField( @@ -219,7 +300,7 @@ function flattenGroupFields( onChange(field.key, { ...nestedObj, [childKey]: childValue }); }, localize, - disabled, + fieldDisabled, collectionRenderOverrides, true, ), @@ -233,7 +314,7 @@ function flattenGroupFields( parentPath, onChange, localize, - disabled, + fieldDisabled, collectionRenderOverrides, true, ), @@ -298,6 +379,7 @@ function FieldGroup({ disabled, defaultExpanded, transportType, + lockedKeys, }: { labelKey: string; fields: t.SchemaField[]; @@ -307,6 +389,7 @@ function FieldGroup({ disabled?: boolean; defaultExpanded: boolean; transportType: string; + lockedKeys?: Set; }) { const localize = useLocalize(); const { isExpanded, hasEverExpanded, sectionRef, toggle } = useCollapsibleSection({ @@ -349,6 +432,8 @@ function FieldGroup({ localize, transportType, disabled, + undefined, + lockedKeys, )} , )} @@ -366,18 +451,17 @@ function McpEntryFields({ parentPath, onChange, disabled, + lockedKeys, }: { fields: t.SchemaField[]; parentValue: t.ConfigValue; parentPath: string; onChange: (path: string, value: t.ConfigValue) => void; disabled?: boolean; + lockedKeys?: Set; }) { const localize = useLocalize(); - const values = - typeof parentValue === 'object' && parentValue !== null && !Array.isArray(parentValue) - ? (parentValue as Record) - : {}; + const values = isPlainObject(parentValue) ? parentValue : {}; const explicitType = typeof values.type === 'string' ? values.type : ''; const currentType = explicitType || inferTransportType(values); @@ -386,12 +470,11 @@ function McpEntryFields({ const isRemote = REMOTE_TRANSPORTS.has(currentType); const visibleKeys = new Set(); for (const field of fields) { - // Transport-specific fields: only show if they belong to the current transport + if (INSPECTOR_DERIVED.has(field.key)) continue; if (ALL_TRANSPORT_KEYS.has(field.key)) { if (currentTransportFields.has(field.key)) { visibleKeys.add(field.key); } - // Auth fields: only show for remote transports } else if (REMOTE_ONLY_FIELDS.has(field.key)) { if (isRemote) { visibleKeys.add(field.key); @@ -401,16 +484,6 @@ function McpEntryFields({ } } - // When type changes, we only update the type field itself. Transport-specific - // fields from the old type are already hidden by the visibility logic above, - // so stale values are invisible. We intentionally avoid clearing them here - // because ObjectEntryCard.handleFieldChange captures a stale `value` snapshot - // per call — multiple synchronous onChange calls would lose all but the last. - const handleChange = (key: string, value: t.ConfigValue) => { - onChange(key, value); - }; - - // Filter fields through visibility, then organize into groups const fieldsByKey = new Map(fields.map((f) => [f.key, f])); const collectGroupKeys = (groups: FieldGroupDef[]): string[] => groups.flatMap((g) => [...g.fields, ...(g.children ? collectGroupKeys(g.children) : [])]); @@ -426,7 +499,6 @@ function McpEntryFields({ const hasChildren = group.children && group.children.length > 0; const groupFields = resolveFields(group.fields); - // For parent groups with children, check if any child sub-group has visible fields if (hasChildren) { const childGroups = group.children!.filter((child) => resolveFields(child.fields).length > 0); if (childGroups.length === 0 && groupFields.length === 0) return null; @@ -442,10 +514,12 @@ function McpEntryFields({ groupFields, parentValue, parentPath, - handleChange, + onChange, localize, currentType, disabled, + undefined, + lockedKeys, )} )} @@ -456,10 +530,11 @@ function McpEntryFields({ fields={resolveFields(child.fields)} parentValue={parentValue} parentPath={parentPath} - onChange={handleChange} + onChange={onChange} disabled={disabled} defaultExpanded={child.defaultExpanded} transportType={currentType} + lockedKeys={lockedKeys} /> ))} @@ -473,10 +548,11 @@ function McpEntryFields({ fields={groupFields} parentValue={parentValue} parentPath={parentPath} - onChange={handleChange} + onChange={onChange} disabled={disabled} defaultExpanded={group.defaultExpanded} transportType={currentType} + lockedKeys={lockedKeys} /> ); }; @@ -490,10 +566,11 @@ function McpEntryFields({ fields={ungrouped} parentValue={parentValue} parentPath={parentPath} - onChange={handleChange} + onChange={onChange} disabled={disabled} defaultExpanded={false} transportType={currentType} + lockedKeys={lockedKeys} /> )} @@ -535,6 +612,14 @@ function CreateMcpServerDialog({ setError(localize('com_config_server_name_required')); return; } + if (name.includes('.')) { + setError(localize('com_config_server_name_no_dots')); + return; + } + if (name === '__proto__' || name === 'constructor' || name === 'prototype') { + setError(localize('com_config_server_name_invalid')); + return; + } if (existingKeys.has(name)) { setError(localize('com_config_server_name_exists')); return; @@ -603,71 +688,275 @@ function CreateMcpServerDialog({ // --------------------------------------------------------------------------- export function McpServersRenderer(props: t.FieldRendererProps) { - const { fields, parentPath, parentValue, getValue, onChange, disabled } = props; + const { + fields, + parentPath, + parentValue, + getValue, + onChange, + disabled, + editedValues, + yamlBaseKeys, + } = props; const localize = useLocalize(); const [createOpen, setCreateOpen] = useState(false); const [justAddedKey, setJustAddedKey] = useState(null); - - const renderGroupedMcpFields: t.CollectionRenderFields = useCallback( - (entryFields, entryValue, entryPath, entryOnChange) => ( - - ), - [disabled], - ); + const [validationError, setValidationError] = useState(null); const path = parentPath; - const value = getValue(path, parentValue ?? {}); - const record = - value && typeof value === 'object' && !Array.isArray(value) - ? (value as Record) - : {}; - const entries = Object.entries(record); - const existingKeys = new Set(Object.keys(record)); + const entryPrefix = `${path}.`; + const baseValue = getValue(path, parentValue ?? {}); + const baseRecord: Record = isPlainObject(baseValue) ? baseValue : {}; + + /** + * Group edited paths by their entry key. Per-leaf paths are + * `mcpServers..<...>`; the entry key is the first path segment after + * the renderer's parent path. We collect [segmentsAfterEntry, value] pairs + * keyed by entry name so the record overlay can apply edits per entry + * without re-walking the full edit map. + */ + const editsByEntry = useMemo(() => { + const map = new Map>(); + if (!editedValues) return map; + for (const [editPath, value] of Object.entries(editedValues)) { + if (!editPath.startsWith(entryPrefix)) continue; + const rest = editPath.slice(entryPrefix.length); + const parts = rest.split('.'); + const entryKey = parts[0]; + if (!entryKey) continue; + const segments = parts.slice(1); + const list = map.get(entryKey) ?? []; + list.push({ segments, value }); + map.set(entryKey, list); + } + return map; + }, [editedValues, entryPrefix]); + + /** + * Effective record overlay: walks every per-leaf edit for each entry and + * applies it on top of the base value. Leaf undefined deletes the leaf; + * if every leaf under an entry is undefined and the entry has no remaining + * keys, the entry is removed from the rendered list. Entries with no edits + * are referenced by identity from `baseRecord` (no deep clone) so steady + * state does not pay for cloning unchanged data. + */ + const record = useMemo(() => { + if (editsByEntry.size === 0) return baseRecord; + const result: Record = {}; + for (const [k, v] of Object.entries(baseRecord)) { + if (!editsByEntry.has(k)) { + result[k] = v; + } + } + for (const [entryKey, leafEdits] of editsByEntry) { + // Whole-entry writes (empty segments) are kept for backward compatibility + // with the previous create/delete flows. + const wholeEntryWrites = leafEdits.filter((e) => e.segments.length === 0); + const leafWrites = leafEdits.filter((e) => e.segments.length > 0); + + let current: t.ConfigValue | undefined; + if (wholeEntryWrites.length > 0) { + const last = wholeEntryWrites[wholeEntryWrites.length - 1]; + if (last.value === undefined) { + continue; + } + current = last.value; + } else if (entryKey in baseRecord) { + current = baseRecord[entryKey]; + } + + if (leafWrites.length === 0) { + if (current !== undefined) result[entryKey] = current; + continue; + } + const existingObj = isPlainObject(current) ? current : {}; + const overlay = applyLeafOverlay( + existingObj, + leafWrites.map((e) => [e.segments, e.value] as [string[], t.ConfigValue]), + ); + result[entryKey] = overlay; + } + return result; + }, [baseRecord, editsByEntry]); + + const entries = useMemo(() => Object.entries(record), [record]); + + const existingKeys = useMemo(() => new Set(Object.keys(record)), [record]); + + /** + * Server identity is locked iff the entry's name appears in the un-merged + * YAML/file base config (sourced via getBaseConfigFn's yamlMcpKeys). + * + * When the upstream YAML key set is unavailable (older LibreChat backend + * without ?baseOnly=true support), return an empty set so nothing is + * locked. That degrades to "everything editable" rather than to a + * subtraction heuristic which produced false positives on YAML servers + * with admin overrides on cosmetic fields like title or iconPath. + */ + const yamlSourceKeys = useMemo(() => { + return yamlBaseKeys ?? new Set(); + }, [yamlBaseKeys]); + + /** + * Refs let the create/remove/rename callbacks stay referentially stable so + * memo(McpEntryRow) actually bails on entries that didn't change. The refs + * shadow editedValues/baseRecord/record (each of which changes on every + * keystroke) and are read at call time from inside the callback bodies. + */ + const editedValuesRef = useRef(editedValues); + useEffect(() => { + editedValuesRef.current = editedValues; + }, [editedValues]); + + const baseRecordRef = useRef(baseRecord); + useEffect(() => { + baseRecordRef.current = baseRecord; + }, [baseRecord]); + + const recordRef = useRef(record); + useEffect(() => { + recordRef.current = record; + }, [record]); const handleCreate = useCallback( (serverName: string, entry: Record) => { - const next: Record = { [serverName]: entry }; - for (const [k, v] of entries) { - next[k] = v; + if (serverName.includes('.')) { + setValidationError(localize('com_config_server_name_no_dots')); + return; + } + if ( + serverName === '__proto__' || + serverName === 'constructor' || + serverName === 'prototype' + ) { + setValidationError(localize('com_config_server_name_invalid')); + return; } - onChange(path, next); + for (const [fieldKey, fieldValue] of Object.entries(entry)) { + if (fieldValue === undefined || fieldValue === null) continue; + if (fieldValue === '') continue; + if (Array.isArray(fieldValue) && fieldValue.length === 0) continue; + if (isPlainObject(fieldValue)) { + for (const { segments, value } of enumerateLeafPaths(fieldValue, [fieldKey])) { + if (value === undefined || value === null || value === '') continue; + if (Array.isArray(value) && value.length === 0) continue; + onChange(`${path}.${serverName}.${segments.join('.')}`, value); + } + } else { + onChange(`${path}.${serverName}.${fieldKey}`, fieldValue); + } + } + setValidationError(null); setJustAddedKey(serverName); }, - [entries, onChange, path], + [onChange, path, localize], ); const handleRemove = useCallback( (key: string) => { - const next = { ...record }; - delete next[key]; - onChange(path, next); + const editedValues = editedValuesRef.current; + const baseRecord = baseRecordRef.current; + const prefix = `${path}.${key}.`; + const entryPath = `${path}.${key}`; + const seen = new Set(); + if (editedValues) { + for (const editPath of Object.keys(editedValues)) { + if (editPath.startsWith(prefix) || editPath === entryPath) { + onChange(editPath, undefined); + seen.add(editPath); + } + } + } + const baseEntry = baseRecord[key]; + if (isPlainObject(baseEntry)) { + for (const { segments } of enumerateLeafPaths(baseEntry)) { + const leafPath = `${prefix}${segments.join('.')}`; + if (!seen.has(leafPath)) onChange(leafPath, undefined); + } + } + /** + * Also emit a delete at the entry path so MongoDB's $unset collapses + * the whole subtree. Per-leaf $unset alone leaves an empty parent + * object that refetches as a phantom server entry. + */ + if (!seen.has(entryPath)) { + onChange(entryPath, undefined); + } }, - [record, onChange, path], + [onChange, path], ); const handleRename = useCallback( (oldKey: string, newKey: string) => { - if (newKey === oldKey || newKey in record) return; - const next: Record = {}; - for (const [k, v] of Object.entries(record)) { - next[k === oldKey ? newKey : k] = v; + if (newKey === oldKey) { + setValidationError(null); + return; } - onChange(path, next); - }, - [record, onChange, path], - ); + const editedValues = editedValuesRef.current; + const baseRecord = baseRecordRef.current; + const record = recordRef.current; + if (newKey.includes('.')) { + setValidationError(localize('com_config_server_name_no_dots')); + return; + } + if (newKey === '__proto__' || newKey === 'constructor' || newKey === 'prototype') { + setValidationError(localize('com_config_server_name_invalid')); + return; + } + if (Object.hasOwn(record, newKey)) { + setValidationError(localize('com_config_server_name_exists')); + return; + } + const oldPrefixFull = `${path}.${oldKey}`; + if (editedValues) { + for (const editPath of Object.keys(editedValues)) { + if (editPath === oldPrefixFull || editPath.startsWith(`${oldPrefixFull}.`)) { + onChange(editPath, undefined); + } + } + } + const oldPrefix = `${path}.${oldKey}.`; + const newPrefix = `${path}.${newKey}.`; + const baseEntry = baseRecord[oldKey]; + const overlayEntry = record[oldKey]; + + /** + * Enumerate leaves recursively so nested per-leaf data like + * `headers.Authorization` survives the rename intact. The overlay + * representation is authoritative because the edit overlay already + * reflects in-flight changes (including deletions); we still walk the + * base to find leaves removed only in the overlay so the old paths get + * proper undefined-cleanup writes. + */ + const baseLeaves = isPlainObject(baseEntry) ? enumerateLeafPaths(baseEntry) : []; + const overlayLeaves = isPlainObject(overlayEntry) ? enumerateLeafPaths(overlayEntry) : []; + + const overlayBySeg = new Map(); + for (const { segments, value } of overlayLeaves) { + overlayBySeg.set(segments.join('.'), value); + } + const allSegKeys = new Set([ + ...overlayBySeg.keys(), + ...baseLeaves.map((l) => l.segments.join('.')), + ]); - const handleEntryChange = useCallback( - (key: string, newValue: t.ConfigValue) => { - onChange(path, { ...record, [key]: newValue }); + for (const segKey of allSegKeys) { + const segments = segKey.split('.'); + const leafValue = overlayBySeg.get(segKey); + if (leafValue !== undefined) { + onChange(`${newPrefix}${segments.join('.')}`, leafValue); + } + onChange(`${oldPrefix}${segments.join('.')}`, undefined); + } + /** + * Emit an entry-path delete on the old key so MongoDB's $unset collapses + * the whole subtree. Without this, per-leaf unsets leave an empty parent + * object that refetches as a phantom entry under the old name. + */ + onChange(`${path}.${oldKey}`, undefined); + setValidationError(null); }, - [record, onChange, path], + [onChange, path, localize], ); return ( @@ -683,37 +972,34 @@ export function McpServersRenderer(props: t.FieldRendererProps) { {localize('com_config_create_mcp_server')} - {entries.map(([key, entryValue]) => { - // Normalize type for card summary display: resolve http alias and infer - // omitted type so the collapsed card shows the effective transport. - const entryObj = - entryValue && typeof entryValue === 'object' && !Array.isArray(entryValue) - ? (entryValue as Record) - : {}; - const rawType = typeof entryObj.type === 'string' ? entryObj.type : ''; - const effectiveType = - (rawType || inferTransportType(entryObj)) === 'http' - ? 'streamable-http' - : rawType || inferTransportType(entryObj); - const displayValue = - effectiveType !== rawType ? { ...entryObj, type: effectiveType } : entryValue; - - return ( - handleEntryChange(key, v)} - onRemove={disabled ? undefined : () => handleRemove(key)} - onRename={disabled ? undefined : (renamed) => handleRename(key, renamed)} - disabled={disabled} - defaultExpanded={key === justAddedKey} - renderFields={renderGroupedMcpFields} - /> - ); - })} + {validationError && ( +
+ {validationError} + +
+ )} + {entries.map(([key, entryValue]) => ( + + ))} {entries.length === 0 && (

{localize('com_config_no_entries')} @@ -725,8 +1011,119 @@ export function McpServersRenderer(props: t.FieldRendererProps) { onSave={handleCreate} fields={fields} existingKeys={existingKeys} - renderFields={renderGroupedMcpFields} + renderFields={(entryFields, ev, ep, eoc) => ( + + )} /> ); } + +// --------------------------------------------------------------------------- +// McpEntryRow — single entry card, memoized per entry +// --------------------------------------------------------------------------- + +const McpEntryRow = memo(function McpEntryRowImpl({ + entryKey, + entryValue, + fields, + path, + disabled, + isYamlSource, + onChange, + onRemove, + onRename, + justAddedKey, +}: { + entryKey: string; + entryValue: t.ConfigValue; + fields: t.SchemaField[]; + path: string; + disabled?: boolean; + isYamlSource: boolean; + onChange: (path: string, value: t.ConfigValue) => void; + onRemove: (key: string) => void; + onRename: (oldKey: string, newKey: string) => void; + justAddedKey: string | null; +}) { + const entryObj = isPlainObject(entryValue) ? entryValue : {}; + const rawType = typeof entryObj.type === 'string' ? entryObj.type : ''; + const inferred = rawType || inferTransportType(entryObj); + const effectiveType = inferred === 'http' ? 'streamable-http' : inferred; + const displayValue = + effectiveType !== rawType ? { ...entryObj, type: effectiveType } : entryValue; + + const entryPathBase = `${path}.${entryKey}`; + const lockedKeys = isYamlSource ? YAML_LOCKED_FIELDS : undefined; + + /** + * Stable per-leaf onChange shared across renderFields invocations. Hoisting + * this out of the renderEntryFields closure keeps child component identity + * steady when the parent re-renders with the same props. + */ + const entryOnChange = useCallback( + (leafKey: string, leafValue: t.ConfigValue) => { + onChange(`${entryPathBase}.${leafKey}`, leafValue); + }, + [onChange, entryPathBase], + ); + + /** + * Bypass ObjectEntryCard.handleFieldChange and write absolute per-leaf paths + * directly. The renderFields callback ignores the handleFieldChange supplied + * by ObjectEntryCard and uses an entry-scoped onChange built here that + * prefixes with the full entry path. + */ + const renderEntryFields: t.CollectionRenderFields = useCallback( + (entryFields, ev, ep) => ( + + ), + [entryOnChange, disabled, lockedKeys], + ); + + /** Whole-entry replace path (kept for back-compat with ObjectEntryCard's + * onValueChange contract). Not used by typical leaf edits. */ + const handleWholeEntryChange = useCallback( + (v: t.ConfigValue) => { + onChange(entryPathBase, v); + }, + [onChange, entryPathBase], + ); + + return ( + onRemove(entryKey)} + onRename={disabled || isYamlSource ? undefined : (renamed) => onRename(entryKey, renamed)} + disabled={disabled} + defaultExpanded={entryKey === justAddedKey} + renderFields={renderEntryFields} + /> + ); +}); + +// Re-export metadata constants used by tests / future schema-driven extraction. +export { + YAML_LOCKED_FIELDS, + INSPECTOR_DERIVED, + REQUIRED_BY_TRANSPORT, + inferTransportType, + enumerateLeafPaths, +}; diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx new file mode 100644 index 0000000..3baccbd --- /dev/null +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -0,0 +1,686 @@ +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import { render, screen, fireEvent } from '@testing-library/react'; +import type * as t from '@/types'; +import { + McpServersRenderer, + YAML_LOCKED_FIELDS, + INSPECTOR_DERIVED, + enumerateLeafPaths, +} from '../McpServersRenderer'; +import { createField } from '@/test/fixtures'; + +vi.mock('@/hooks/useLocalize', () => ({ + default: () => (key: string) => key, + useLocalize: () => (key: string) => key, +})); + +interface IconProps { + name: string; +} +interface SwitchProps { + checked: boolean; + onCheckedChange?: (v: boolean) => void; + 'aria-label'?: string; +} +interface SelectProps { + children: React.ReactNode; + value: string; + 'aria-label'?: string; + onValueChange?: (v: string) => void; +} +interface SelectItemProps { + children: React.ReactNode; + value: string; +} +interface ButtonProps { + label?: string; + onClick?: () => void; + children?: React.ReactNode; +} +interface TextFieldProps { + id?: string; + value?: string; + placeholder?: string; + onChange?: (v: string) => void; + onBlur?: () => void; + type?: string; + disabled?: boolean; + 'aria-label'?: string; +} +interface NumberFieldProps { + id?: string; + value?: string | number; + placeholder?: string; + onChange?: (v: string) => void; + onBlur?: () => void; + disabled?: boolean; +} +interface IconButtonProps { + icon: string; + onClick?: () => void; + 'aria-label'?: string; +} + +vi.mock('@clickhouse/click-ui', () => ({ + Icon: ({ name }: IconProps) => , + Switch: (props: SwitchProps) => ( + + ), + IconButton: ({ icon, onClick, ...props }: IconButtonProps) => ( + + + ) : null, + }; +}); + +function fieldsForMcp(): t.SchemaField[] { + return [ + createField({ key: 'type', type: 'enum(stdio | sse | streamable-http | websocket)' }), + createField({ key: 'url', type: 'string', isOptional: true }), + createField({ key: 'command', type: 'string', isOptional: true }), + createField({ key: 'title', type: 'string', isOptional: true }), + createField({ key: 'description', type: 'string', isOptional: true }), + createField({ key: 'tools', type: 'array', isOptional: true, isArray: true }), + createField({ key: 'source', type: 'string', isOptional: true }), + ]; +} + +function renderRenderer({ + baseRecord, + editedValues = {}, + dbOverridePaths, + yamlBaseKeys, + onChange = vi.fn(), +}: { + baseRecord: Record; + editedValues?: t.FlatConfigMap; + dbOverridePaths?: Set; + yamlBaseKeys?: Set; + onChange?: (path: string, value: t.ConfigValue) => void; +}) { + const fields = fieldsForMcp(); + const props: t.FieldRendererProps = { + fields, + parentValue: baseRecord, + parentPath: 'mcpServers', + getValue: (path, fallback) => { + if (path in editedValues) return editedValues[path] ?? fallback; + if (path === 'mcpServers') return baseRecord; + return fallback; + }, + onChange, + editedValues, + dbOverridePaths, + yamlBaseKeys, + }; + return { ...render(), onChange, fields }; +} + +describe('McpServersRenderer — metadata sets', () => { + it('locks only the transport-type selector on YAML-defined servers', () => { + expect(YAML_LOCKED_FIELDS.has('type')).toBe(true); + for (const key of ['url', 'command', 'args', 'env', 'stderr', 'headers']) { + expect(YAML_LOCKED_FIELDS.has(key)).toBe(false); + } + }); + + it('hides inspector-derived fields', () => { + for (const key of ['tools', 'capabilities', 'source', 'updatedAt']) { + expect(INSPECTOR_DERIVED.has(key)).toBe(true); + } + }); +}); + +describe('McpServersRenderer — per-leaf write granularity', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('writes per-leaf paths when a single string field changes', () => { + const onChange = vi.fn(); + const baseRecord = { + kapa: { type: 'sse', url: 'https://example.com' }, + }; + const { container } = renderRenderer({ baseRecord, onChange }); + + fireEvent.click(screen.getByText('kapa')); + const urlInput = container.querySelector('input#kapa-url') as HTMLInputElement | null; + expect(urlInput).not.toBeNull(); + fireEvent.change(urlInput!, { target: { value: 'https://new.example.com' } }); + fireEvent.blur(urlInput!); + + const urlCalls = onChange.mock.calls.filter(([p]) => p === 'mcpServers.kapa.url'); + expect(urlCalls.length).toBeGreaterThan(0); + expect(urlCalls[urlCalls.length - 1][1]).toBe('https://new.example.com'); + expect( + onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.kapa' && typeof v === 'object' && v !== null, + ), + ).toBeUndefined(); + }); +}); + +describe('McpServersRenderer — YAML source detection', () => { + it('keeps transport fields editable on a YAML-defined server with no DB overrides', () => { + const baseRecord = { + yamlOne: { type: 'stdio', command: 'node', title: 'YAML server' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(['yamlOne']), + dbOverridePaths: new Set(), + }); + + fireEvent.click(screen.getByText('yamlOne')); + const cmd = container.querySelector('input#yamlOne-command') as HTMLInputElement | null; + expect(cmd).not.toBeNull(); + expect(cmd!.hasAttribute('disabled')).toBe(false); + }); + + it('locks identity (rename/delete) on a YAML-defined server but leaves url editable', () => { + const baseRecord = { + kapa: { type: 'sse', url: 'https://example.com', title: 'Edited title' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(['kapa']), + dbOverridePaths: new Set(['mcpServers.kapa.title']), + }); + + fireEvent.click(screen.getByText('kapa')); + const url = container.querySelector('input#kapa-url') as HTMLInputElement | null; + expect(url).not.toBeNull(); + expect(url!.hasAttribute('disabled')).toBe(false); + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).toBeNull(); + }); + + it('does not lock identity for a server defined only via admin overrides', () => { + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com', title: 'Admin only' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + dbOverridePaths: new Set([ + 'mcpServers.adminOnly.type', + 'mcpServers.adminOnly.url', + 'mcpServers.adminOnly.title', + ]), + }); + + fireEvent.click(screen.getByText('adminOnly')); + const url = container.querySelector('input#adminOnly-url') as HTMLInputElement | null; + expect(url).not.toBeNull(); + expect(url!.hasAttribute('disabled')).toBe(false); + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).not.toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).not.toBeNull(); + }); + + it('treats a freshly-created entry (not in YAML keys) as fully editable', () => { + const baseRecord = { + brandNew: { type: 'sse', url: 'https://new.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + }); + + fireEvent.click(screen.getByText('brandNew')); + const url = container.querySelector('input#brandNew-url') as HTMLInputElement | null; + expect(url).not.toBeNull(); + expect(url!.hasAttribute('disabled')).toBe(false); + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).not.toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).not.toBeNull(); + }); +}); + +describe('McpServersRenderer — lock matrix and inspector hide', () => { + it('does not render inspector-derived fields like source/tools in the card', () => { + const baseRecord = { + one: { + type: 'sse', + url: 'https://x.com', + title: 'T', + source: 'yaml', + tools: ['t1'], + }, + }; + const { container } = renderRenderer({ + baseRecord, + dbOverridePaths: new Set(['mcpServers.one.title']), + }); + fireEvent.click(screen.getByText('one')); + const sourceInput = container.querySelector('input[id="mcpServers-one-source"]'); + expect(sourceInput).toBeNull(); + }); +}); + +describe('McpServersRenderer — rejects dotted server names', () => { + it('shows a no-dots error when create receives a name with a dot', () => { + const onChange = vi.fn(); + const baseRecord = {}; + renderRenderer({ baseRecord, onChange }); + + fireEvent.click(screen.getByText('com_config_create_mcp_server')); + const dialog = screen.getByTestId('form-dialog'); + const nameInput = dialog.querySelector('#mcp-server-name') as HTMLInputElement; + fireEvent.change(nameInput, { target: { value: 'has.dots' } }); + const submit = dialog.querySelector('button[type="button"]') as HTMLButtonElement; + fireEvent.click(submit); + + expect(onChange).not.toHaveBeenCalledWith( + expect.stringContaining('mcpServers.has.dots'), + expect.anything(), + ); + }); +}); + +describe('McpServersRenderer — handleRemove', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('emits undefined for every leaf path including pending edits and base fields', () => { + const onChange = vi.fn(); + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, + }; + const editedValues = { 'mcpServers.adminOnly.title': 'X' }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + onChange, + }); + + const trashBtn = container.querySelector( + 'button[aria-label^="com_ui_delete"]', + ) as HTMLButtonElement | null; + expect(trashBtn).not.toBeNull(); + fireEvent.click(trashBtn!); + + const undefinedPaths = onChange.mock.calls.filter(([, v]) => v === undefined).map(([p]) => p); + expect(undefinedPaths).toEqual( + expect.arrayContaining([ + 'mcpServers.adminOnly.type', + 'mcpServers.adminOnly.url', + 'mcpServers.adminOnly.title', + ]), + ); + }); + + it('emits an entry-path undefined write so MongoDB unsets the whole subtree', () => { + const onChange = vi.fn(); + const baseRecord = { + foo: { type: 'sse', url: 'https://x.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + const trashBtn = container.querySelector( + 'button[aria-label^="com_ui_delete"]', + ) as HTMLButtonElement | null; + expect(trashBtn).not.toBeNull(); + fireEvent.click(trashBtn!); + + const entryClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.foo' && v === undefined, + ); + expect(entryClear).toBeDefined(); + + const leafClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.foo.type' && v === undefined, + ); + expect(leafClear).toBeDefined(); + }); + + it('recurses into nested base fields so leaves like headers.Authorization clear individually', () => { + const onChange = vi.fn(); + const baseRecord = { + withHeaders: { + type: 'sse', + url: 'https://x.com', + headers: { Authorization: 'Bearer a' }, + }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + const trashBtn = container.querySelector( + 'button[aria-label^="com_ui_delete"]', + ) as HTMLButtonElement | null; + expect(trashBtn).not.toBeNull(); + fireEvent.click(trashBtn!); + + const authClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.withHeaders.headers.Authorization' && v === undefined, + ); + expect(authClear).toBeDefined(); + + const wholeHeadersClear = onChange.mock.calls.find( + ([p]) => p === 'mcpServers.withHeaders.headers', + ); + expect(wholeHeadersClear).toBeUndefined(); + }); +}); + +describe('McpServersRenderer — handleRename', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + function triggerRename(container: HTMLElement, newKey: string) { + const pencil = container.querySelector( + 'button[aria-label^="com_a11y_rename_entry"]', + ) as HTMLButtonElement | null; + expect(pencil).not.toBeNull(); + fireEvent.click(pencil!); + const renameInput = container.querySelector( + 'input.config-input-ghost', + ) as HTMLInputElement | null; + expect(renameInput).not.toBeNull(); + fireEvent.change(renameInput!, { target: { value: newKey } }); + fireEvent.blur(renameInput!); + } + + it('preserves nested per-leaf data when renaming an entry with headers', () => { + const onChange = vi.fn(); + const baseRecord = { + foo: { + type: 'sse', + url: 'https://x.com', + headers: { Authorization: 'Bearer a' }, + }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + fireEvent.click(screen.getByText('foo')); + triggerRename(container, 'bar'); + + const newAuth = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.bar.headers.Authorization' && v === 'Bearer a', + ); + expect(newAuth).toBeDefined(); + + const oldAuthClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.foo.headers.Authorization' && v === undefined, + ); + expect(oldAuthClear).toBeDefined(); + + const wholeHeadersWrite = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.bar.headers' && typeof v === 'object' && v !== null, + ); + expect(wholeHeadersWrite).toBeUndefined(); + }); + + it('emits an entry-path undefined write for the old key so MongoDB unsets the whole subtree', () => { + const onChange = vi.fn(); + const baseRecord = { + foo: { type: 'sse', url: 'https://x.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + fireEvent.click(screen.getByText('foo')); + triggerRename(container, 'bar'); + + const oldEntryClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.foo' && v === undefined, + ); + expect(oldEntryClear).toBeDefined(); + + const oldLeafClear = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.foo.url' && v === undefined, + ); + expect(oldLeafClear).toBeDefined(); + + const newUrlWrite = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.bar.url' && v === 'https://x.com', + ); + expect(newUrlWrite).toBeDefined(); + }); + + it('rejects rename to an existing entry name and surfaces an error banner', () => { + const onChange = vi.fn(); + const baseRecord = { + foo: { type: 'sse', url: 'https://x.com' }, + bar: { type: 'sse', url: 'https://y.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + fireEvent.click(screen.getByText('foo')); + triggerRename(container, 'bar'); + + const renamePaths = onChange.mock.calls.filter( + ([p]) => typeof p === 'string' && p.startsWith('mcpServers.bar.'), + ); + expect(renamePaths).toEqual([]); + expect(screen.getByText('com_config_server_name_exists')).toBeInTheDocument(); + }); +}); + +describe('McpServersRenderer — record overlay', () => { + it('applies per-leaf edits over the base record (url visible by default)', () => { + const baseRecord = { + kapa: { type: 'sse', url: 'https://x.com' }, + }; + const editedValues = { 'mcpServers.kapa.url': 'https://overlay.example.com' }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + fireEvent.click(screen.getByText('kapa')); + const urlInput = container.querySelector('input#kapa-url') as HTMLInputElement | null; + expect(urlInput).not.toBeNull(); + expect(urlInput!.value).toBe('https://overlay.example.com'); + }); +}); + +describe('McpServersRenderer — handleCreate per-leaf writes', () => { + it('enumerateLeafPaths emits one leaf per primitive even under a nested object', () => { + const leaves = enumerateLeafPaths({ + type: 'sse', + url: 'https://example.com', + headers: { Authorization: 'Bearer a' }, + }); + const flat = leaves.map((l) => ({ path: l.segments.join('.'), value: l.value })); + expect(flat).toEqual( + expect.arrayContaining([ + { path: 'type', value: 'sse' }, + { path: 'url', value: 'https://example.com' }, + { path: 'headers.Authorization', value: 'Bearer a' }, + ]), + ); + const headersWhole = flat.find((l) => l.path === 'headers'); + expect(headersWhole).toBeUndefined(); + }); +}); + +describe('McpServersRenderer — rejects reserved server names', () => { + it('blocks __proto__ at create without emitting onChange', () => { + const onChange = vi.fn(); + const baseRecord = {}; + renderRenderer({ baseRecord, onChange }); + + fireEvent.click(screen.getByText('com_config_create_mcp_server')); + const dialog = screen.getByTestId('form-dialog'); + const nameInput = dialog.querySelector('#mcp-server-name') as HTMLInputElement; + fireEvent.change(nameInput, { target: { value: '__proto__' } }); + const submit = dialog.querySelector('button[type="button"]') as HTMLButtonElement; + fireEvent.click(submit); + + const protoCalls = onChange.mock.calls.filter( + ([p]) => typeof p === 'string' && p.startsWith('mcpServers.__proto__'), + ); + expect(protoCalls).toEqual([]); + }); +}); + +describe('McpServersRenderer — yamlBaseKeys undefined fallback', () => { + it('locks nothing when yamlBaseKeys is undefined (newer-UI / older-API)', () => { + const baseRecord = { + yamlServer: { type: 'sse', url: 'https://x.com', title: 'YAML' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: undefined, + dbOverridePaths: new Set(['mcpServers.yamlServer.title']), + }); + + fireEvent.click(screen.getByText('yamlServer')); + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).not.toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).not.toBeNull(); + }); +}); + +describe('McpServersRenderer — create then edit then rename preserves nested data', () => { + it('writes per-leaf paths through edit and rename in sequence', () => { + const baseRecord = { + kapa: { + type: 'sse', + url: 'https://x.com', + headers: { Authorization: 'old' }, + }, + }; + const editedValues: t.FlatConfigMap = { + 'mcpServers.kapa.headers.Authorization': 'new', + }; + const onChange = vi.fn(); + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + onChange, + }); + + fireEvent.click(screen.getByText('kapa')); + const renameBtn = container.querySelector( + 'button[aria-label^="com_a11y_rename_entry"]', + ) as HTMLButtonElement | null; + expect(renameBtn).not.toBeNull(); + fireEvent.click(renameBtn!); + const renameInput = container.querySelector( + 'input.config-input-ghost', + ) as HTMLInputElement | null; + expect(renameInput).not.toBeNull(); + fireEvent.change(renameInput!, { target: { value: 'kapa2' } }); + fireEvent.blur(renameInput!); + + const authWrite = onChange.mock.calls.find( + ([p]) => p === 'mcpServers.kapa2.headers.Authorization', + ); + expect(authWrite).toBeDefined(); + expect(authWrite![1]).toBe('new'); + + const oldClears = onChange.mock.calls.filter( + ([p, v]) => typeof p === 'string' && p.startsWith('mcpServers.kapa.') && v === undefined, + ); + expect(oldClears.length).toBeGreaterThan(0); + + const wholeHeadersWrite = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.kapa2.headers' && typeof v === 'object' && v !== null, + ); + expect(wholeHeadersWrite).toBeUndefined(); + }); +}); diff --git a/src/components/configuration/sections/mcpFieldMeta.ts b/src/components/configuration/sections/mcpFieldMeta.ts new file mode 100644 index 0000000..4b4c515 --- /dev/null +++ b/src/components/configuration/sections/mcpFieldMeta.ts @@ -0,0 +1,41 @@ +/** + * Static metadata for MCP server field handling. + * + * IMPORTANT: These sets MUST be updated when LibreChat adds new MCP fields. + * The long-term plan is to encode this information directly in the Zod schema + * as field-level tags (for example, `meta({ readonly: true })` or + * `meta({ runtimeOnly: true })`) so the renderer can read it from the schema + * tree instead of duplicating field-key knowledge. Tracked as future work. + */ + +/** + * Fields on YAML-defined MCP servers that the admin panel cannot edit because + * mutating them is structurally unsafe under the deployment model. + * + * Multi-tenant LibreChat uses YAML for global defaults shared across all + * tenants; the admin panel is the per-tenant customization surface, so most + * fields (url, command, args, env, headers, apiKey, oauth) are legitimately + * tenant-overrideable. Only `type` is locked because changing the transport + * selector breaks the Zod union (MCPOptionsSchema) and produces + * inspectionFailed stubs that cannot connect. + * + * Server name is locked separately via the hidden onRename affordance, not + * through this set, because a name change creates a duplicate map key rather + * than a true rename. + */ +export const YAML_LOCKED_FIELDS = new Set(['type']); + +/** + * Fields populated by the MCP inspector at runtime. Admin overrides for these + * are silently ignored by the inspector, so we hide them from the editor + * entirely (for both YAML and config-tier servers). + */ +export const INSPECTOR_DERIVED = new Set([ + 'tools', + 'capabilities', + 'initDuration', + 'inspectionFailed', + 'updatedAt', + 'dbId', + 'source', +]); diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index 9719e25..9a18ee2 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -75,6 +75,8 @@ "com_config_server_name": "Server name", "com_config_server_name_required": "Server name is required", "com_config_server_name_exists": "A server with this name already exists", + "com_config_server_name_no_dots": "Server name cannot contain dots", + "com_config_server_name_invalid": "Server name is not allowed", "com_config_group_connection": "Connection", "com_config_group_authentication": "Authentication", "com_config_group_api_key": "API key", diff --git a/src/server/config.test.ts b/src/server/config.test.ts index fddc4bf..6781fe6 100644 --- a/src/server/config.test.ts +++ b/src/server/config.test.ts @@ -621,6 +621,62 @@ describe('validateFieldValue', () => { const result = validateFieldValue('interface', { privacyPolicy: {} }); expect(result).toBeDefined(); }); + + it('accepts streamable-http type for MCP server', () => { + expect(validateFieldValue('mcpServers.foo.type', 'streamable-http')).toEqual({ success: true }); + }); + + it('accepts http type for MCP server', () => { + expect(validateFieldValue('mcpServers.foo.type', 'http')).toEqual({ success: true }); + }); + + it('accepts stdio type for MCP server', () => { + expect(validateFieldValue('mcpServers.foo.type', 'stdio')).toEqual({ success: true }); + }); + + it('rejects unknown type for MCP server', () => { + const result = validateFieldValue('mcpServers.foo.type', 'made-up-transport'); + expect(result.success).toBe(false); + }); + + it('returns the most-specific branch on union failure (anyOfSchema heuristic)', () => { + const result = validateFieldValue('mcpServers.foo.type', 'made-up-transport'); + expect(result.success).toBe(false); + if (!result.success) { + const semicolonSplits = result.error.split(';'); + // The most-specific branch is the literal-mismatch failure with a single + // issue, not the cumulative dump of every transport-branch failure. + expect(semicolonSplits.length).toBeLessThanOrEqual(2); + expect(result.error.length).toBeGreaterThan(0); + } + }); + + it('accepts https URL for MCP server (matches sse/streamable-http branches)', () => { + expect(validateFieldValue('mcpServers.foo.url', 'https://example.com/mcp')).toEqual({ + success: true, + }); + }); + + it('accepts ws URL for MCP server (matches websocket branch)', () => { + expect(validateFieldValue('mcpServers.foo.url', 'wss://example.com/mcp')).toEqual({ + success: true, + }); + }); + + it('rejects non-URL value for MCP server', () => { + const result = validateFieldValue('mcpServers.foo.url', 'not a url'); + expect(result.success).toBe(false); + }); + + it('returns a single-branch error on union URL failure, not a concatenated dump', () => { + const result = validateFieldValue('mcpServers.foo.url', 'not a url'); + expect(result.success).toBe(false); + if (!result.success) { + const semicolonSplits = result.error.split(';'); + expect(semicolonSplits.length).toBeLessThanOrEqual(2); + expect(result.error.length).toBeGreaterThan(0); + } + }); }); /* --------------------------------------------------------------------------- @@ -857,9 +913,8 @@ describe('YAML-editor fallback audit', () => { * Custom endpoint validation and schema extraction * -----------------------------------------------------------------------*/ -const ldpEndpoint = ( - require3('librechat-data-provider') as { endpointSchema: ZodV3Schema } -).endpointSchema; +const ldpEndpoint = (require3('librechat-data-provider') as { endpointSchema: ZodV3Schema }) + .endpointSchema; describe('custom endpoint schema', () => { const endpointTree = extractSchemaTree(ldpEndpoint); diff --git a/src/server/config.ts b/src/server/config.ts index 148bf65..332f287 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -129,7 +129,6 @@ function inferRecordKVTypes(schema: t.ZodSchemaLike): t.KVValueType[] | undefine return types.size > 0 ? [...types] : undefined; } - /** Merges fields from union object variants into a single list. * When the same key appears in multiple variants with different literal * types, the literals are combined into a union(literal(...) | literal(...)) @@ -454,6 +453,70 @@ export function getZodTypeName( return typeName || 'unknown'; } +/** + * Builds a schema-like wrapper that accepts a value if any candidate's `safeParse` + * accepts it. Avoids `z.union` so we don't tie callers to a specific zod major + * version: candidates here may come from packages bundling an older zod release. + */ +function anyOfSchema(candidates: t.ZodSchemaLike[]): t.ZodSchemaLike { + type ParseResult = { + success: boolean; + error?: { issues: Array<{ message: string; path: (string | number)[] }> }; + }; + const safeParse = (value: unknown): ParseResult => { + const errors: NonNullable[] = []; + for (const candidate of candidates) { + const c = candidate as t.ZodSchemaLike & { + safeParse?: (v: unknown) => ParseResult; + }; + if (typeof c.safeParse !== 'function') continue; + let result: ParseResult; + try { + result = c.safeParse(value); + } catch (e) { + errors.push({ + issues: [{ message: e instanceof Error ? e.message : 'Validation failed', path: [] }], + }); + continue; + } + if (result.success) return { success: true }; + if (result.error) errors.push(result.error); + } + /** + * Heuristic: when every branch failed, return the branch with the FEWEST + * issues. That's almost always the one the user was aiming at (a single + * typed field mismatch is more specific than a multi-field shape error), + * so it produces a much cleaner message than dumping all branch issues. + * Ties on issue count are broken by preferring the longest issue path + * (more specific to the user's input shape) so a remote-URL field doesn't + * fall back to the first branch's generic literal mismatch. + */ + const sorted = errors + .filter((e): e is NonNullable => e != null && Array.isArray(e.issues)) + .sort((a, b) => { + const ca = a.issues?.length ?? Infinity; + const cb = b.issues?.length ?? Infinity; + if (ca !== cb) return ca - cb; + const pa = Math.max(0, ...(a.issues ?? []).map((i) => i.path?.length ?? 0)); + const pb = Math.max(0, ...(b.issues ?? []).map((i) => i.path?.length ?? 0)); + return pb - pa; + }); + const best = sorted[0]; + return { + success: false, + error: best ?? { issues: [{ message: 'Validation failed', path: [] }] }, + }; + }; + /** + * SAFETY: We synthesize a partial ZodSchemaLike here that only implements + * safeParse + _def.typeName. validateFieldValue only invokes safeParse on + * the returned schema, so the partial shape is sufficient. We avoid + * constructing a real z.union to bypass the v3/v4 cross-version pitfall + * (configSchema bundles zod@3 while this file uses zod@4). + */ + return { _def: { typeName: 'ZodUnion' }, safeParse } as t.ZodSchemaLike; +} + /** Walks a Zod schema tree to find the sub-schema at a given dot-path. * Returns the schema **with wrappers intact** so `.safeParse()` runs the * full validation chain (refine, transform, pipe). Returns `null` if the @@ -484,16 +547,25 @@ export function resolveSubSchema( current = valueType; } else if (typeName === 'ZodUnion') { const options = unwrapped._def.options ?? []; - let found: t.ZodSchemaLike | null = null; + const candidates: t.ZodSchemaLike[] = []; for (const opt of options) { const optUnwrapped = unwrapSchema(opt); if (optUnwrapped?.shape?.[segment]) { - found = optUnwrapped.shape[segment]; - break; + candidates.push(optUnwrapped.shape[segment]); } } - if (!found) return null; - current = found; + if (candidates.length === 0) return null; + if (candidates.length === 1) { + current = candidates[0]; + } else { + /** + * Multiple union branches expose this field with potentially different schemas + * (e.g. MCPOptionsSchema's `type` is a different literal in each transport). + * Accept any of them by reconstructing a runtime union so per-field validation + * works without needing to pre-discriminate from sibling fields. + */ + current = anyOfSchema(candidates); + } } else if (typeName === 'ZodIntersection') { const resolved = resolveIntersection(unwrapped); if (!resolved?.shape?.[segment]) return null; @@ -761,8 +833,9 @@ function normalizeAppServiceKeys( } export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async () => { - const [baseResponse, dbBaseResponse] = await Promise.all([ + const [baseResponse, baseOnlyResponse, dbBaseResponse] = await Promise.all([ apiFetch('/api/admin/config/base'), + apiFetch('/api/admin/config/base?baseOnly=true'), apiFetch(`/api/admin/config/role/${BASE_CONFIG_PRINCIPAL_ID}`), ]); @@ -792,7 +865,19 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( dbOverrides = dbConfig.overrides as Record; } - return { config, dbOverrides, configuredFromBase, schemaDefaults: flatDefaults }; + let yamlMcpKeys: string[] | undefined; + if (baseOnlyResponse.ok) { + const { config: baseOnlyRaw } = (await baseOnlyResponse.json()) as { + config: Record; + }; + const baseOnly = normalizeAppServiceKeys(baseOnlyRaw); + const mcp = baseOnly.mcpServers; + if (mcp && typeof mcp === 'object' && !Array.isArray(mcp)) { + yamlMcpKeys = Object.keys(mcp as Record); + } + } + + return { config, dbOverrides, configuredFromBase, schemaDefaults: flatDefaults, yamlMcpKeys }; }); export const baseConfigOptions = queryOptions({ @@ -833,7 +918,10 @@ async function mergeIndexedArrayEntries( const segments = arrayPath.split('.'); let current: unknown = baseConfig; for (const seg of segments) { - if (current == null || typeof current !== 'object') { current = undefined; break; } + if (current == null || typeof current !== 'object') { + current = undefined; + break; + } current = (current as Record)[seg]; } const arr = Array.isArray(current) ? [...current] : []; diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index b13e873..d0f2ec0 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -84,6 +84,12 @@ export interface ConfigTabContentProps { sectionPermissions?: Record; schemaDefaults?: FlatConfigMap; showConfiguredOnly?: boolean; + /** Entry-key sets for record-type sections that come from the un-merged + * YAML/file base config. Keyed by the section's parent path (e.g. + * `mcpServers`). Record-type renderers use this to distinguish entries + * whose identity is locked by YAML from entries that exist only via + * admin overrides. */ + baseRecordKeys?: Record>; } export interface ConfigTableOfContentsProps { @@ -206,6 +212,10 @@ export interface FieldRendererProps { getValue: (path: string, fallback: ConfigValue) => ConfigValue; onChange: (path: string, value: ConfigValue) => void; onResetField?: (path: string) => void; + /** Current edited values map keyed by dot-path. Used by record-type custom + * renderers to drive per-entry dirty state without re-deriving from + * touchedPaths (which sticks even after edits revert to baseline). */ + editedValues?: FlatConfigMap; disabled?: boolean; profileMap?: Record; previewMode?: boolean; @@ -224,6 +234,12 @@ export interface FieldRendererProps { /** When true, never strip labels via isSoleField. Use when rendering a * subset of fields within a larger section (e.g. priority fields). */ alwaysShowLabels?: boolean; + /** Entry keys that come from the un-merged YAML/file base config for + * the section currently being rendered. Record-type renderers (e.g. + * MCP servers) use this to lock identity affordances (rename, delete) + * for entries whose names originate in YAML, regardless of whether + * admin overrides have been added on top of them. */ + yamlBaseKeys?: Set; } export interface ImportYamlDialogProps { From 90e90e8cd58dd72fcae8e1fd06ac8c6860f0864b Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 22 May 2026 19:52:30 -0700 Subject: [PATCH 02/28] =?UTF-8?q?=F0=9F=8D=9E=20fix:=20Surface=20MCP=20Dis?= =?UTF-8?q?allowed-Name=20Errors=20As=20Save-Style=20Toasts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the inline danger banner in McpServersRenderer with the same toast channel ConfigPage uses for save errors. ConfigTabContent forwards an optional onValidationError prop down to record-type renderers, ConfigPage wires it to showToast, and the renderer fires toasts instead of holding local error state. Dot, prototype, and existing-name guards remain to prevent dotted names from corrupting the per-leaf write path on save. --- src/components/configuration/ConfigPage.tsx | 1 + .../configuration/ConfigTabContent.tsx | 2 ++ .../sections/McpServersRenderer.tsx | 32 +++++-------------- .../__tests__/McpServersRenderer.test.tsx | 16 ++++++++-- src/types/config-ui.ts | 2 ++ 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index bc3c76e..e405205 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -877,6 +877,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi schemaDefaults={schemaDefaults} showConfiguredOnly={showConfiguredOnly} baseRecordKeys={baseRecordKeys} + onValidationError={(message) => showToast({ type: 'error', message }, 5000)} /> diff --git a/src/components/configuration/ConfigTabContent.tsx b/src/components/configuration/ConfigTabContent.tsx index 8e94f20..20a7e01 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -55,6 +55,7 @@ export function ConfigTabContent({ schemaDefaults, showConfiguredOnly, baseRecordKeys, + onValidationError, }: t.ConfigTabContentProps) { const localize = useLocalize(); const fieldsDisabled = readOnly; @@ -182,6 +183,7 @@ export function ConfigTabContent({ schemaDefaults, showConfiguredOnly, yamlBaseKeys: baseRecordKeys?.[dataKey], + onValidationError, }; return ( <> diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index f187e9d..8c71a32 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -697,11 +697,11 @@ export function McpServersRenderer(props: t.FieldRendererProps) { disabled, editedValues, yamlBaseKeys, + onValidationError, } = props; const localize = useLocalize(); const [createOpen, setCreateOpen] = useState(false); const [justAddedKey, setJustAddedKey] = useState(null); - const [validationError, setValidationError] = useState(null); const path = parentPath; const entryPrefix = `${path}.`; @@ -821,7 +821,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const handleCreate = useCallback( (serverName: string, entry: Record) => { if (serverName.includes('.')) { - setValidationError(localize('com_config_server_name_no_dots')); + onValidationError?.(localize('com_config_server_name_no_dots')); return; } if ( @@ -829,7 +829,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { serverName === 'constructor' || serverName === 'prototype' ) { - setValidationError(localize('com_config_server_name_invalid')); + onValidationError?.(localize('com_config_server_name_invalid')); return; } for (const [fieldKey, fieldValue] of Object.entries(entry)) { @@ -846,10 +846,9 @@ export function McpServersRenderer(props: t.FieldRendererProps) { onChange(`${path}.${serverName}.${fieldKey}`, fieldValue); } } - setValidationError(null); setJustAddedKey(serverName); }, - [onChange, path, localize], + [onChange, path, localize, onValidationError], ); const handleRemove = useCallback( @@ -889,22 +888,21 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const handleRename = useCallback( (oldKey: string, newKey: string) => { if (newKey === oldKey) { - setValidationError(null); return; } const editedValues = editedValuesRef.current; const baseRecord = baseRecordRef.current; const record = recordRef.current; if (newKey.includes('.')) { - setValidationError(localize('com_config_server_name_no_dots')); + onValidationError?.(localize('com_config_server_name_no_dots')); return; } if (newKey === '__proto__' || newKey === 'constructor' || newKey === 'prototype') { - setValidationError(localize('com_config_server_name_invalid')); + onValidationError?.(localize('com_config_server_name_invalid')); return; } if (Object.hasOwn(record, newKey)) { - setValidationError(localize('com_config_server_name_exists')); + onValidationError?.(localize('com_config_server_name_exists')); return; } const oldPrefixFull = `${path}.${oldKey}`; @@ -954,9 +952,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { * object that refetches as a phantom entry under the old name. */ onChange(`${path}.${oldKey}`, undefined); - setValidationError(null); }, - [onChange, path, localize], + [onChange, path, localize, onValidationError], ); return ( @@ -972,19 +969,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { {localize('com_config_create_mcp_server')} - {validationError && ( -

- {validationError} - -
- )} {entries.map(([key, entryValue]) => ( ; editedValues?: t.FlatConfigMap; dbOverridePaths?: Set; yamlBaseKeys?: Set; onChange?: (path: string, value: t.ConfigValue) => void; + onValidationError?: (message: string) => void; }) { const fields = fieldsForMcp(); const props: t.FieldRendererProps = { @@ -194,8 +196,14 @@ function renderRenderer({ editedValues, dbOverridePaths, yamlBaseKeys, + onValidationError, + }; + return { + ...render(), + onChange, + onValidationError, + fields, }; - return { ...render(), onChange, fields }; } describe('McpServersRenderer — metadata sets', () => { @@ -536,8 +544,9 @@ describe('McpServersRenderer — handleRename', () => { expect(newUrlWrite).toBeDefined(); }); - it('rejects rename to an existing entry name and surfaces an error banner', () => { + it('rejects rename to an existing entry name and reports a validation error', () => { const onChange = vi.fn(); + const onValidationError = vi.fn(); const baseRecord = { foo: { type: 'sse', url: 'https://x.com' }, bar: { type: 'sse', url: 'https://y.com' }, @@ -546,6 +555,7 @@ describe('McpServersRenderer — handleRename', () => { baseRecord, yamlBaseKeys: new Set(), onChange, + onValidationError, }); fireEvent.click(screen.getByText('foo')); @@ -555,7 +565,7 @@ describe('McpServersRenderer — handleRename', () => { ([p]) => typeof p === 'string' && p.startsWith('mcpServers.bar.'), ); expect(renamePaths).toEqual([]); - expect(screen.getByText('com_config_server_name_exists')).toBeInTheDocument(); + expect(onValidationError).toHaveBeenCalledWith('com_config_server_name_exists'); }); }); diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index d0f2ec0..085b27d 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -90,6 +90,7 @@ export interface ConfigTabContentProps { * whose identity is locked by YAML from entries that exist only via * admin overrides. */ baseRecordKeys?: Record>; + onValidationError?: (message: string) => void; } export interface ConfigTableOfContentsProps { @@ -240,6 +241,7 @@ export interface FieldRendererProps { * for entries whose names originate in YAML, regardless of whether * admin overrides have been added on top of them. */ yamlBaseKeys?: Set; + onValidationError?: (message: string) => void; } export interface ImportYamlDialogProps { From b3a1cf4b80c59acbb75201850445cf3f8472f1f3 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 22 May 2026 20:18:18 -0700 Subject: [PATCH 03/28] =?UTF-8?q?=F0=9F=A7=B9=20chore:=20Trim=20Narrating?= =?UTF-8?q?=20Comments=20From=20MCP=20Renderer=20And=20Helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop file-header docblocks, section dividers, and JSDocs that restated what well-named identifiers already say. Keep only the WHY comments that explain non-obvious behavior: MongoDB \$unset subtree collapse, zod v3/v4 cross-version anyOfSchema, container-delete prune bypass, ref-stable callbacks for memo bail, and YAML-source detection fallback. 171 added comment lines down to 21. --- src/components/configuration/ConfigPage.tsx | 21 +-- .../sections/McpServersRenderer.tsx | 146 ++---------------- .../configuration/sections/mcpFieldMeta.ts | 32 +--- src/server/config.test.ts | 2 - src/server/config.ts | 29 +--- src/types/config-ui.ts | 15 +- 6 files changed, 19 insertions(+), 226 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index e405205..67e35b0 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -130,12 +130,6 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return new Set(Object.keys(flattenObject(dbOverrides))); }, [dbOverrides]); - /** - * Authoritative set of YAML-defined entries per section, sourced from the - * un-merged baseOnly response. Identity affordances (rename, delete) are - * locked iff a name appears here, regardless of whether admin overrides - * also exist for it. - */ const baseRecordKeys = useMemo(() => { const result: Record> = {}; const yamlMcpKeys = baseConfigData?.yamlMcpKeys; @@ -359,12 +353,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return scopeResolvedValues ?? {}; }, [isEditingScope, flatBaseline, scopeResolvedValues]); - /** - * Every intermediate (non-leaf) path implied by the baseline's flat keys. - * Used by handleFieldChange to recognize "delete the whole subtree" writes - * whose target path doesn't itself appear in scopeBaseline because - * flatBaseline only stores leaf paths. - */ + /** Container paths inferred from leaf baselines, used to tell apart subtree-deletes from no-op writes. */ const baselineIntermediates = useMemo(() => { const set = new Set(); for (const leaf of Object.keys(scopeBaseline)) { @@ -391,13 +380,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi (typeof value === 'object' && typeof baseline === 'object' && JSON.stringify(value) === JSON.stringify(baseline)); - /** - * Suppress the prune for undefined writes that target an intermediate - * container path. handleRemove/handleRename emit these to request - * "delete the whole subtree"; pruning would silently drop them - * because flatBaseline only stores leaf paths, so scopeBaseline at - * the container is always undefined and would falsely match. - */ + /** A container-path undefined write must survive; baseline only stores leaves, so it would otherwise match `undefined === undefined` and get pruned. */ const isContainerDelete = value === undefined && baselineIntermediates.has(path); if (match && !isContainerDelete) { const next = { ...prev }; diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 8c71a32..889202a 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -1,17 +1,3 @@ -/** - * Custom section renderer for `mcpServers` — the MCP Servers tab. - * - * Renders MCP server entries as expandable cards with: - * - Transport-type-aware field visibility (stdio vs sse vs streamable-http vs websocket) - * - Semantic field groups (Connection, Authentication, Server Options, Advanced) - * - A "Create MCP Server" dialog for adding new entries - * - TOC-compatible scroll targets via entry card IDs - * - * All edits write per-leaf paths (`mcpServers..`) into editedValues - * so single-field reset, baseline-equality dirty pruning, and rename orphan - * cleanup all work uniformly. - */ - import { Icon } from '@clickhouse/click-ui'; import { memo, useRef, useMemo, useState, useEffect, useCallback } from 'react'; import type { ReactNode } from 'react'; @@ -26,11 +12,6 @@ import { FormDialog } from '@/components/shared'; import { useLocalize } from '@/hooks'; import { cn } from '@/utils'; -// --------------------------------------------------------------------------- -// Transport type → field visibility -// --------------------------------------------------------------------------- - -/** Fields specific to each transport type. */ const TRANSPORT_FIELDS: Record = { stdio: ['command', 'args', 'env', 'stderr'], sse: ['url', 'headers'], @@ -39,15 +20,10 @@ const TRANSPORT_FIELDS: Record = { websocket: ['url'], }; -/** All transport-specific field keys (union of all TRANSPORT_FIELDS values). */ const ALL_TRANSPORT_KEYS = new Set(Object.values(TRANSPORT_FIELDS).flat()); - -/** Auth-related fields only shown for remote transports (not stdio). */ const REMOTE_ONLY_FIELDS = new Set(['requiresOAuth', 'apiKey', 'oauth', 'oauth_headers']); - const REMOTE_TRANSPORTS = new Set(['sse', 'streamable-http', 'http', 'websocket']); -/** Fields that require a value depending on transport type. */ const REQUIRED_BY_TRANSPORT: Record> = { stdio: new Set(['command', 'args']), sse: new Set(['url']), @@ -56,7 +32,6 @@ const REQUIRED_BY_TRANSPORT: Record> = { websocket: new Set(['url']), }; -/** Curated transport type options with lowercase labels, excluding `http` alias. */ const TRANSPORT_TYPE_OPTIONS: { label: string; value: string }[] = [ { label: 'streamable-http', value: 'streamable-http' }, { label: 'sse', value: 'sse' }, @@ -64,17 +39,12 @@ const TRANSPORT_TYPE_OPTIONS: { label: string; value: string }[] = [ { label: 'websocket', value: 'websocket' }, ]; -/** The `type` field is always required. */ const ALWAYS_REQUIRED = new Set(['type']); /** - * Infer transport type from configured fields, mirroring Zod's union resolution - * order in MCPOptionsSchema: Stdio → WebSocket → SSE → StreamableHTTP. - * * YAML configs can omit `type` because each transport schema (except - * streamable-http) provides a default. The backend infers the type from the - * discriminating fields (command, url protocol). We replicate that here so the - * UI shows the effective type for existing configs. + * streamable-http) defaults from the discriminating fields. Mirror the + * backend's Zod union resolution order so the UI shows the effective type. */ function inferTransportType(values: Record): string { if (typeof values.type === 'string' && values.type) return values.type; @@ -84,7 +54,7 @@ function inferTransportType(values: Record): string { const protocol = new URL(values.url).protocol; if (protocol === 'ws:' || protocol === 'wss:') return 'websocket'; } catch { - // invalid URL — fall through to sse as default for any url presence + /* fall through */ } return 'sse'; } @@ -102,10 +72,6 @@ function withFieldOverrides(field: t.SchemaField, transportType: string): t.Sche return field; } -// --------------------------------------------------------------------------- -// Per-leaf edit overlay helpers -// --------------------------------------------------------------------------- - function setLeaf( target: Record, segments: string[], @@ -157,12 +123,6 @@ function isPlainObject(value: t.ConfigValue): value is Record, prefix: string[] = [], @@ -182,15 +142,10 @@ function enumerateLeafPaths( return out; } -// --------------------------------------------------------------------------- -// Semantic field groups -// --------------------------------------------------------------------------- - interface FieldGroupDef { labelKey: string; fields: string[]; defaultExpanded: boolean; - /** Nested sub-groups rendered inside this group (fields should be empty when using children). */ children?: FieldGroupDef[]; } @@ -234,10 +189,6 @@ const MCP_FIELD_GROUPS: FieldGroupDef[] = [ }, ]; -// --------------------------------------------------------------------------- -// FieldGroup — collapsible group within a card (replicates EndpointsRenderer) -// --------------------------------------------------------------------------- - function flattenGroupFields( fields: t.SchemaField[], parentValue: t.ConfigValue, @@ -254,9 +205,6 @@ function flattenGroupFields( const nodes: ReactNode[] = []; for (const field of fields) { const fieldDisabled = disabled || (lockedKeys?.has(field.key) ?? false); - // Custom render for transport type select — curated options with lowercase labels. - // When `type` is omitted (common in YAML configs), infer it from other fields - // to mirror backend Zod union resolution. if (field.key === 'type') { const fieldId = `${parentPath}-${field.key}`; const label = localize(`com_config_field_${field.key}`); @@ -324,7 +272,6 @@ function flattenGroupFields( return nodes; } -/** Parent-level collapsible section that wraps child sub-groups. */ function FieldGroupSection({ labelKey, defaultExpanded, @@ -441,10 +388,6 @@ function FieldGroup({ ); } -// --------------------------------------------------------------------------- -// McpEntryFields — dynamic visibility based on transport type -// --------------------------------------------------------------------------- - function McpEntryFields({ fields, parentValue, @@ -465,7 +408,6 @@ function McpEntryFields({ const explicitType = typeof values.type === 'string' ? values.type : ''; const currentType = explicitType || inferTransportType(values); - // Build visible field keys based on current transport type const currentTransportFields = new Set(TRANSPORT_FIELDS[currentType] ?? []); const isRemote = REMOTE_TRANSPORTS.has(currentType); const visibleKeys = new Set(); @@ -577,10 +519,6 @@ function McpEntryFields({ ); } -// --------------------------------------------------------------------------- -// CreateMcpServerDialog -// --------------------------------------------------------------------------- - function CreateMcpServerDialog({ open, onClose, @@ -683,9 +621,6 @@ function CreateMcpServerDialog({ ); } -// --------------------------------------------------------------------------- -// McpServersRenderer — main export -// --------------------------------------------------------------------------- export function McpServersRenderer(props: t.FieldRendererProps) { const { @@ -708,13 +643,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const baseValue = getValue(path, parentValue ?? {}); const baseRecord: Record = isPlainObject(baseValue) ? baseValue : {}; - /** - * Group edited paths by their entry key. Per-leaf paths are - * `mcpServers..<...>`; the entry key is the first path segment after - * the renderer's parent path. We collect [segmentsAfterEntry, value] pairs - * keyed by entry name so the record overlay can apply edits per entry - * without re-walking the full edit map. - */ const editsByEntry = useMemo(() => { const map = new Map>(); if (!editedValues) return map; @@ -732,14 +660,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { return map; }, [editedValues, entryPrefix]); - /** - * Effective record overlay: walks every per-leaf edit for each entry and - * applies it on top of the base value. Leaf undefined deletes the leaf; - * if every leaf under an entry is undefined and the entry has no remaining - * keys, the entry is removed from the rendered list. Entries with no edits - * are referenced by identity from `baseRecord` (no deep clone) so steady - * state does not pay for cloning unchanged data. - */ const record = useMemo(() => { if (editsByEntry.size === 0) return baseRecord; const result: Record = {}; @@ -749,8 +669,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { } } for (const [entryKey, leafEdits] of editsByEntry) { - // Whole-entry writes (empty segments) are kept for backward compatibility - // with the previous create/delete flows. const wholeEntryWrites = leafEdits.filter((e) => e.segments.length === 0); const leafWrites = leafEdits.filter((e) => e.segments.length > 0); @@ -784,25 +702,15 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const existingKeys = useMemo(() => new Set(Object.keys(record)), [record]); /** - * Server identity is locked iff the entry's name appears in the un-merged - * YAML/file base config (sourced via getBaseConfigFn's yamlMcpKeys). - * - * When the upstream YAML key set is unavailable (older LibreChat backend - * without ?baseOnly=true support), return an empty set so nothing is - * locked. That degrades to "everything editable" rather than to a - * subtraction heuristic which produced false positives on YAML servers - * with admin overrides on cosmetic fields like title or iconPath. + * Fall back to an empty set when the backend predates ?baseOnly support, so + * nothing is locked rather than falling back to a subtraction heuristic that + * produces false positives on YAML servers with admin overrides. */ const yamlSourceKeys = useMemo(() => { return yamlBaseKeys ?? new Set(); }, [yamlBaseKeys]); - /** - * Refs let the create/remove/rename callbacks stay referentially stable so - * memo(McpEntryRow) actually bails on entries that didn't change. The refs - * shadow editedValues/baseRecord/record (each of which changes on every - * keystroke) and are read at call time from inside the callback bodies. - */ + /** Refs keep the callbacks referentially stable so memo(McpEntryRow) can bail. */ const editedValuesRef = useRef(editedValues); useEffect(() => { editedValuesRef.current = editedValues; @@ -873,11 +781,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { if (!seen.has(leafPath)) onChange(leafPath, undefined); } } - /** - * Also emit a delete at the entry path so MongoDB's $unset collapses - * the whole subtree. Per-leaf $unset alone leaves an empty parent - * object that refetches as a phantom server entry. - */ + /** Entry-path delete is needed to make MongoDB's $unset collapse the subtree; per-leaf $unset alone leaves an empty parent that refetches as a phantom entry. */ if (!seen.has(entryPath)) { onChange(entryPath, undefined); } @@ -918,14 +822,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const baseEntry = baseRecord[oldKey]; const overlayEntry = record[oldKey]; - /** - * Enumerate leaves recursively so nested per-leaf data like - * `headers.Authorization` survives the rename intact. The overlay - * representation is authoritative because the edit overlay already - * reflects in-flight changes (including deletions); we still walk the - * base to find leaves removed only in the overlay so the old paths get - * proper undefined-cleanup writes. - */ + /** Walk overlay AND base: overlay holds in-flight edits, base catches leaves the overlay has already deleted so their old paths still get undefined-cleanup writes. */ const baseLeaves = isPlainObject(baseEntry) ? enumerateLeafPaths(baseEntry) : []; const overlayLeaves = isPlainObject(overlayEntry) ? enumerateLeafPaths(overlayEntry) : []; @@ -946,11 +843,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { } onChange(`${oldPrefix}${segments.join('.')}`, undefined); } - /** - * Emit an entry-path delete on the old key so MongoDB's $unset collapses - * the whole subtree. Without this, per-leaf unsets leave an empty parent - * object that refetches as a phantom entry under the old name. - */ + /** See $unset note in handleRemove. */ onChange(`${path}.${oldKey}`, undefined); }, [onChange, path, localize, onValidationError], @@ -1009,10 +902,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { ); } -// --------------------------------------------------------------------------- -// McpEntryRow — single entry card, memoized per entry -// --------------------------------------------------------------------------- - const McpEntryRow = memo(function McpEntryRowImpl({ entryKey, entryValue, @@ -1046,11 +935,6 @@ const McpEntryRow = memo(function McpEntryRowImpl({ const entryPathBase = `${path}.${entryKey}`; const lockedKeys = isYamlSource ? YAML_LOCKED_FIELDS : undefined; - /** - * Stable per-leaf onChange shared across renderFields invocations. Hoisting - * this out of the renderEntryFields closure keeps child component identity - * steady when the parent re-renders with the same props. - */ const entryOnChange = useCallback( (leafKey: string, leafValue: t.ConfigValue) => { onChange(`${entryPathBase}.${leafKey}`, leafValue); @@ -1058,12 +942,6 @@ const McpEntryRow = memo(function McpEntryRowImpl({ [onChange, entryPathBase], ); - /** - * Bypass ObjectEntryCard.handleFieldChange and write absolute per-leaf paths - * directly. The renderFields callback ignores the handleFieldChange supplied - * by ObjectEntryCard and uses an entry-scoped onChange built here that - * prefixes with the full entry path. - */ const renderEntryFields: t.CollectionRenderFields = useCallback( (entryFields, ev, ep) => ( { onChange(entryPathBase, v); @@ -1103,7 +980,6 @@ const McpEntryRow = memo(function McpEntryRowImpl({ ); }); -// Re-export metadata constants used by tests / future schema-driven extraction. export { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED, diff --git a/src/components/configuration/sections/mcpFieldMeta.ts b/src/components/configuration/sections/mcpFieldMeta.ts index 4b4c515..a949ecc 100644 --- a/src/components/configuration/sections/mcpFieldMeta.ts +++ b/src/components/configuration/sections/mcpFieldMeta.ts @@ -1,35 +1,7 @@ -/** - * Static metadata for MCP server field handling. - * - * IMPORTANT: These sets MUST be updated when LibreChat adds new MCP fields. - * The long-term plan is to encode this information directly in the Zod schema - * as field-level tags (for example, `meta({ readonly: true })` or - * `meta({ runtimeOnly: true })`) so the renderer can read it from the schema - * tree instead of duplicating field-key knowledge. Tracked as future work. - */ - -/** - * Fields on YAML-defined MCP servers that the admin panel cannot edit because - * mutating them is structurally unsafe under the deployment model. - * - * Multi-tenant LibreChat uses YAML for global defaults shared across all - * tenants; the admin panel is the per-tenant customization surface, so most - * fields (url, command, args, env, headers, apiKey, oauth) are legitimately - * tenant-overrideable. Only `type` is locked because changing the transport - * selector breaks the Zod union (MCPOptionsSchema) and produces - * inspectionFailed stubs that cannot connect. - * - * Server name is locked separately via the hidden onRename affordance, not - * through this set, because a name change creates a duplicate map key rather - * than a true rename. - */ +/** Changing `type` breaks the MCPOptionsSchema Zod union and produces inspectionFailed stubs that cannot connect. */ export const YAML_LOCKED_FIELDS = new Set(['type']); -/** - * Fields populated by the MCP inspector at runtime. Admin overrides for these - * are silently ignored by the inspector, so we hide them from the editor - * entirely (for both YAML and config-tier servers). - */ +/** Inspector-populated fields; admin overrides for these are silently ignored. */ export const INSPECTOR_DERIVED = new Set([ 'tools', 'capabilities', diff --git a/src/server/config.test.ts b/src/server/config.test.ts index 6781fe6..3a1620a 100644 --- a/src/server/config.test.ts +++ b/src/server/config.test.ts @@ -644,8 +644,6 @@ describe('validateFieldValue', () => { expect(result.success).toBe(false); if (!result.success) { const semicolonSplits = result.error.split(';'); - // The most-specific branch is the literal-mismatch failure with a single - // issue, not the cumulative dump of every transport-branch failure. expect(semicolonSplits.length).toBeLessThanOrEqual(2); expect(result.error.length).toBeGreaterThan(0); } diff --git a/src/server/config.ts b/src/server/config.ts index 332f287..9672c95 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -453,11 +453,7 @@ export function getZodTypeName( return typeName || 'unknown'; } -/** - * Builds a schema-like wrapper that accepts a value if any candidate's `safeParse` - * accepts it. Avoids `z.union` so we don't tie callers to a specific zod major - * version: candidates here may come from packages bundling an older zod release. - */ +/** Synthesizes a union without z.union to avoid the zod v3/v4 cross-version pitfall. */ function anyOfSchema(candidates: t.ZodSchemaLike[]): t.ZodSchemaLike { type ParseResult = { success: boolean; @@ -482,15 +478,7 @@ function anyOfSchema(candidates: t.ZodSchemaLike[]): t.ZodSchemaLike { if (result.success) return { success: true }; if (result.error) errors.push(result.error); } - /** - * Heuristic: when every branch failed, return the branch with the FEWEST - * issues. That's almost always the one the user was aiming at (a single - * typed field mismatch is more specific than a multi-field shape error), - * so it produces a much cleaner message than dumping all branch issues. - * Ties on issue count are broken by preferring the longest issue path - * (more specific to the user's input shape) so a remote-URL field doesn't - * fall back to the first branch's generic literal mismatch. - */ + /** Pick the branch with the fewest issues (most likely the intended one); tiebreak on longest path. */ const sorted = errors .filter((e): e is NonNullable => e != null && Array.isArray(e.issues)) .sort((a, b) => { @@ -507,13 +495,6 @@ function anyOfSchema(candidates: t.ZodSchemaLike[]): t.ZodSchemaLike { error: best ?? { issues: [{ message: 'Validation failed', path: [] }] }, }; }; - /** - * SAFETY: We synthesize a partial ZodSchemaLike here that only implements - * safeParse + _def.typeName. validateFieldValue only invokes safeParse on - * the returned schema, so the partial shape is sufficient. We avoid - * constructing a real z.union to bypass the v3/v4 cross-version pitfall - * (configSchema bundles zod@3 while this file uses zod@4). - */ return { _def: { typeName: 'ZodUnion' }, safeParse } as t.ZodSchemaLike; } @@ -558,12 +539,6 @@ export function resolveSubSchema( if (candidates.length === 1) { current = candidates[0]; } else { - /** - * Multiple union branches expose this field with potentially different schemas - * (e.g. MCPOptionsSchema's `type` is a different literal in each transport). - * Accept any of them by reconstructing a runtime union so per-field validation - * works without needing to pre-discriminate from sibling fields. - */ current = anyOfSchema(candidates); } } else if (typeName === 'ZodIntersection') { diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index 085b27d..21e4e26 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -84,11 +84,7 @@ export interface ConfigTabContentProps { sectionPermissions?: Record; schemaDefaults?: FlatConfigMap; showConfiguredOnly?: boolean; - /** Entry-key sets for record-type sections that come from the un-merged - * YAML/file base config. Keyed by the section's parent path (e.g. - * `mcpServers`). Record-type renderers use this to distinguish entries - * whose identity is locked by YAML from entries that exist only via - * admin overrides. */ + /** YAML-defined entry keys per section, keyed by parent path. */ baseRecordKeys?: Record>; onValidationError?: (message: string) => void; } @@ -213,9 +209,6 @@ export interface FieldRendererProps { getValue: (path: string, fallback: ConfigValue) => ConfigValue; onChange: (path: string, value: ConfigValue) => void; onResetField?: (path: string) => void; - /** Current edited values map keyed by dot-path. Used by record-type custom - * renderers to drive per-entry dirty state without re-deriving from - * touchedPaths (which sticks even after edits revert to baseline). */ editedValues?: FlatConfigMap; disabled?: boolean; profileMap?: Record; @@ -235,11 +228,7 @@ export interface FieldRendererProps { /** When true, never strip labels via isSoleField. Use when rendering a * subset of fields within a larger section (e.g. priority fields). */ alwaysShowLabels?: boolean; - /** Entry keys that come from the un-merged YAML/file base config for - * the section currently being rendered. Record-type renderers (e.g. - * MCP servers) use this to lock identity affordances (rename, delete) - * for entries whose names originate in YAML, regardless of whether - * admin overrides have been added on top of them. */ + /** YAML-defined entry keys for the section being rendered. */ yamlBaseKeys?: Set; onValidationError?: (message: string) => void; } From a083be3b2c10c4a4e104cd537d47febd3eb283d3 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 13:40:27 -0700 Subject: [PATCH 04/28] =?UTF-8?q?=F0=9F=A9=BA=20fix:=20Preserve=20Union=20?= =?UTF-8?q?Traversal=20And=20Legacy=20Dotted=20MCP=20Keys?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit anyOfSchema now keeps its candidate options on _def so resolveSubSchema can keep walking when a path crosses more than one union segment; the union walker also recurses into each candidate so records, arrays, and nested unions resolve through their own case instead of relying on shape lookup. Without this, validateFieldValue silently returned success for any leaf reached through a multi-candidate union. editsByEntry now resolves the entry key by longest-prefix match against baseRecord keys, so legacy server names that already contain dots stay attributed to their real entry rather than getting carved up at the first dot. New names are still blocked from containing dots at create and rename time. Regression tests cover the through-union nested-field path in config.test.ts and the dotted legacy entry editsByEntry plus remove behavior in McpServersRenderer.test.tsx. --- .../sections/McpServersRenderer.tsx | 36 +++++++++++--- .../__tests__/McpServersRenderer.test.tsx | 49 +++++++++++++++++++ src/server/config.test.ts | 32 ++++++++++++ src/server/config.ts | 10 ++-- 4 files changed, 114 insertions(+), 13 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 889202a..40d8508 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -123,6 +123,27 @@ function isPlainObject(value: t.ConfigValue): value is Record, +): { entryKey: string; segments: string[] } | null { + if (rest === '') return null; + let best: string | null = null; + for (const key of knownKeys) { + if (rest === key || rest.startsWith(`${key}.`)) { + if (best === null || key.length > best.length) best = key; + } + } + if (best !== null) { + const remainder = rest.length > best.length ? rest.slice(best.length + 1) : ''; + return { entryKey: best, segments: remainder === '' ? [] : remainder.split('.') }; + } + const dotIdx = rest.indexOf('.'); + if (dotIdx === -1) return { entryKey: rest, segments: [] }; + return { entryKey: rest.slice(0, dotIdx), segments: rest.slice(dotIdx + 1).split('.') }; +} + function enumerateLeafPaths( obj: Record, prefix: string[] = [], @@ -646,19 +667,18 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const editsByEntry = useMemo(() => { const map = new Map>(); if (!editedValues) return map; + const knownKeys = Object.keys(baseRecord); for (const [editPath, value] of Object.entries(editedValues)) { if (!editPath.startsWith(entryPrefix)) continue; const rest = editPath.slice(entryPrefix.length); - const parts = rest.split('.'); - const entryKey = parts[0]; - if (!entryKey) continue; - const segments = parts.slice(1); - const list = map.get(entryKey) ?? []; - list.push({ segments, value }); - map.set(entryKey, list); + const parsed = resolveEntryKey(rest, knownKeys); + if (!parsed) continue; + const list = map.get(parsed.entryKey) ?? []; + list.push({ segments: parsed.segments, value }); + map.set(parsed.entryKey, list); } return map; - }, [editedValues, entryPrefix]); + }, [editedValues, entryPrefix, baseRecord]); const record = useMemo(() => { if (editsByEntry.size === 0) return baseRecord; diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index dfb7c05..c2612a2 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -366,6 +366,55 @@ describe('McpServersRenderer — rejects dotted server names', () => { }); }); +describe('McpServersRenderer — legacy dotted entry keys', () => { + it('groups edits under the dotted base key instead of splitting on the first dot', () => { + const baseRecord = { + 'legacy.dotted': { type: 'sse', url: 'https://old.example.com' }, + }; + const editedValues = { 'mcpServers.legacy.dotted.url': 'https://new.example.com' }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + + const header = screen.queryByText('legacy.dotted'); + expect(header).not.toBeNull(); + fireEvent.click(header!); + const url = container.querySelector('input#legacy-dotted-url') as HTMLInputElement | null; + expect(url).not.toBeNull(); + expect(url!.value).toBe('https://new.example.com'); + }); + + it('clears the dotted base entry on remove, not a phantom shortened key', () => { + const onChange = vi.fn(); + const baseRecord = { + 'legacy.dotted': { type: 'sse', url: 'https://old.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + const trashBtn = container.querySelector( + 'button[aria-label^="com_ui_delete"]', + ) as HTMLButtonElement | null; + expect(trashBtn).not.toBeNull(); + fireEvent.click(trashBtn!); + + const undefinedPaths = onChange.mock.calls.filter(([, v]) => v === undefined).map(([p]) => p); + expect(undefinedPaths).toEqual( + expect.arrayContaining([ + 'mcpServers.legacy.dotted', + 'mcpServers.legacy.dotted.type', + 'mcpServers.legacy.dotted.url', + ]), + ); + expect(undefinedPaths).not.toContain('mcpServers.legacy'); + }); +}); + describe('McpServersRenderer — handleRemove', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/src/server/config.test.ts b/src/server/config.test.ts index 3a1620a..60426d0 100644 --- a/src/server/config.test.ts +++ b/src/server/config.test.ts @@ -675,6 +675,38 @@ describe('validateFieldValue', () => { expect(result.error.length).toBeGreaterThan(0); } }); + + it('validates a nested field reached through a union (header value must be string)', () => { + expect( + validateFieldValue('mcpServers.foo.headers.Authorization', 'Bearer xyz'), + ).toEqual({ success: true }); + const bad = validateFieldValue('mcpServers.foo.headers.Authorization', 42); + expect(bad.success).toBe(false); + }); +}); + +/* --------------------------------------------------------------------------- + * Regression — resolveSubSchema must keep walking through synthetic unions. + * The anyOfSchema wrapper used to drop _def.options, so the second segment + * after a multi-candidate union could not be resolved and validateFieldValue + * silently passed everything under unioned objects. + * -----------------------------------------------------------------------*/ + +describe('resolveSubSchema — traversal through synthesized union', () => { + it('walks into a nested field after a union with multiple candidates', () => { + const schema = z3.object({ + payload: z3.union([ + z3.object({ headers: z3.record(z3.string(), z3.string()) }), + z3.object({ headers: z3.record(z3.string(), z3.string()) }), + ]), + }); + const sub = resolveSubSchema(schema, ['payload', 'headers', 'Authorization']); + expect(sub).not.toBeNull(); + const safeParse = (sub as { safeParse?: (v: unknown) => { success: boolean } }).safeParse; + expect(typeof safeParse).toBe('function'); + expect(safeParse!('Bearer xyz').success).toBe(true); + expect(safeParse!(42).success).toBe(false); + }); }); /* --------------------------------------------------------------------------- diff --git a/src/server/config.ts b/src/server/config.ts index 9672c95..38f1243 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -495,7 +495,8 @@ function anyOfSchema(candidates: t.ZodSchemaLike[]): t.ZodSchemaLike { error: best ?? { issues: [{ message: 'Validation failed', path: [] }] }, }; }; - return { _def: { typeName: 'ZodUnion' }, safeParse } as t.ZodSchemaLike; + /** _def.options is preserved so resolveSubSchema can keep traversing into nested fields under union branches; without it, the next segment short-circuits and validateFieldValue silently passes everything. */ + return { _def: { typeName: 'ZodUnion', options: candidates }, safeParse } as t.ZodSchemaLike; } /** Walks a Zod schema tree to find the sub-schema at a given dot-path. @@ -530,10 +531,9 @@ export function resolveSubSchema( const options = unwrapped._def.options ?? []; const candidates: t.ZodSchemaLike[] = []; for (const opt of options) { - const optUnwrapped = unwrapSchema(opt); - if (optUnwrapped?.shape?.[segment]) { - candidates.push(optUnwrapped.shape[segment]); - } + /** Recurse so options that are records, arrays, or further unions resolve through their own walker case, not just shape lookup. */ + const resolved = resolveSubSchema(opt, [segment]); + if (resolved) candidates.push(resolved); } if (candidates.length === 0) return null; if (candidates.length === 1) { From 2f29f227343881c040a351fdeb281a58141a26a4 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 13:52:26 -0700 Subject: [PATCH 05/28] =?UTF-8?q?=F0=9F=94=92=20fix:=20Render=20Legacy=20D?= =?UTF-8?q?otted=20MCP=20Entries=20Read-Only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A per-leaf write under a dotted server name (mcpServers.legacy.dotted.url for entry "legacy.dotted") is indistinguishable on the save side from a nested mcpServers.legacy.dotted.url path, because the fields PATCH endpoint parses fieldPath as dot-delimited segments. Pre-existing dotted entries previously survived because the renderer saved the whole mcpServers record; per-leaf granularity broke that. McpEntryRow now flags entryKey.includes('.') as a dotted-legacy entry and renders it disabled with rename and delete hidden. The entry remains visible so admins know it exists and can fix it via YAML or the underlying datastore. entryOnChange and handleWholeEntryChange short-circuit for dotted keys, and handleRemove and handleRename add the same guard as defense-in-depth so no programmatic caller can emit a colliding fieldPath. Regression tests cover the disabled-input render, missing rename and delete buttons, and the no-onChange-writes invariant. --- .../sections/McpServersRenderer.tsx | 26 ++++++++---- .../__tests__/McpServersRenderer.test.tsx | 41 +++++++++++-------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 40d8508..5e0a231 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -781,6 +781,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const handleRemove = useCallback( (key: string) => { + /** Dotted entry names cannot survive the per-leaf save path; the row hides the trash button for them, this is defense-in-depth so no programmatic caller can corrupt sibling subtrees by emitting `mcpServers..<...>` writes. */ + if (key.includes('.')) return; const editedValues = editedValuesRef.current; const baseRecord = baseRecordRef.current; const prefix = `${path}.${key}.`; @@ -814,6 +816,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { if (newKey === oldKey) { return; } + /** Same dotted-key reasoning as handleRemove; defense-in-depth even though the row hides the rename pencil. */ + if (oldKey.includes('.')) return; const editedValues = editedValuesRef.current; const baseRecord = baseRecordRef.current; const record = recordRef.current; @@ -953,13 +957,18 @@ const McpEntryRow = memo(function McpEntryRowImpl({ effectiveType !== rawType ? { ...entryObj, type: effectiveType } : entryValue; const entryPathBase = `${path}.${entryKey}`; - const lockedKeys = isYamlSource ? YAML_LOCKED_FIELDS : undefined; + /** Dotted entry names predate the dot-rejecting create/rename validators; the save endpoint parses fieldPath as dot-delimited so any per-leaf write under such a key collides with a parallel "legacy" → "dotted" nested-object interpretation. Render them read-only so they stay visible in the list but never round-trip through the per-field save API. */ + const isDottedLegacy = entryKey.includes('.'); + const isReadOnly = !!disabled || isDottedLegacy; + const isLockedIdentity = isYamlSource || isDottedLegacy; + const lockedKeys = isYamlSource && !isDottedLegacy ? YAML_LOCKED_FIELDS : undefined; const entryOnChange = useCallback( (leafKey: string, leafValue: t.ConfigValue) => { + if (isDottedLegacy) return; onChange(`${entryPathBase}.${leafKey}`, leafValue); }, - [onChange, entryPathBase], + [onChange, entryPathBase, isDottedLegacy], ); const renderEntryFields: t.CollectionRenderFields = useCallback( @@ -969,19 +978,20 @@ const McpEntryRow = memo(function McpEntryRowImpl({ parentValue={ev} parentPath={ep} onChange={entryOnChange} - disabled={disabled} + disabled={isReadOnly} lockedKeys={lockedKeys} /> ), - [entryOnChange, disabled, lockedKeys], + [entryOnChange, isReadOnly, lockedKeys], ); /** Required by ObjectEntryCard's onValueChange contract; unused on leaf edits. */ const handleWholeEntryChange = useCallback( (v: t.ConfigValue) => { + if (isDottedLegacy) return; onChange(entryPathBase, v); }, - [onChange, entryPathBase], + [onChange, entryPathBase, isDottedLegacy], ); return ( @@ -991,9 +1001,9 @@ const McpEntryRow = memo(function McpEntryRowImpl({ fields={fields} value={displayValue} onValueChange={handleWholeEntryChange} - onRemove={disabled || isYamlSource ? undefined : () => onRemove(entryKey)} - onRename={disabled || isYamlSource ? undefined : (renamed) => onRename(entryKey, renamed)} - disabled={disabled} + onRemove={isReadOnly || isLockedIdentity ? undefined : () => onRemove(entryKey)} + onRename={isReadOnly || isLockedIdentity ? undefined : (renamed) => onRename(entryKey, renamed)} + disabled={isReadOnly} defaultExpanded={entryKey === justAddedKey} renderFields={renderEntryFields} /> diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index c2612a2..8b4348a 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -369,12 +369,10 @@ describe('McpServersRenderer — rejects dotted server names', () => { describe('McpServersRenderer — legacy dotted entry keys', () => { it('groups edits under the dotted base key instead of splitting on the first dot', () => { const baseRecord = { - 'legacy.dotted': { type: 'sse', url: 'https://old.example.com' }, + 'legacy.dotted': { type: 'sse', url: 'https://new.example.com' }, }; - const editedValues = { 'mcpServers.legacy.dotted.url': 'https://new.example.com' }; const { container } = renderRenderer({ baseRecord, - editedValues, yamlBaseKeys: new Set(), }); @@ -386,7 +384,7 @@ describe('McpServersRenderer — legacy dotted entry keys', () => { expect(url!.value).toBe('https://new.example.com'); }); - it('clears the dotted base entry on remove, not a phantom shortened key', () => { + it('renders dotted legacy entries read-only with no rename or delete affordances', () => { const onChange = vi.fn(); const baseRecord = { 'legacy.dotted': { type: 'sse', url: 'https://old.example.com' }, @@ -397,21 +395,30 @@ describe('McpServersRenderer — legacy dotted entry keys', () => { onChange, }); - const trashBtn = container.querySelector( - 'button[aria-label^="com_ui_delete"]', - ) as HTMLButtonElement | null; - expect(trashBtn).not.toBeNull(); - fireEvent.click(trashBtn!); + fireEvent.click(screen.getByText('legacy.dotted')); + const url = container.querySelector('input#legacy-dotted-url') as HTMLInputElement | null; + expect(url).not.toBeNull(); + expect(url!.hasAttribute('disabled')).toBe(true); - const undefinedPaths = onChange.mock.calls.filter(([, v]) => v === undefined).map(([p]) => p); - expect(undefinedPaths).toEqual( - expect.arrayContaining([ - 'mcpServers.legacy.dotted', - 'mcpServers.legacy.dotted.type', - 'mcpServers.legacy.dotted.url', - ]), + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).toBeNull(); + }); + + it('emits no per-leaf writes for a dotted entry even if the field would otherwise fire onChange', () => { + const onChange = vi.fn(); + const baseRecord = { + 'legacy.dotted': { type: 'sse', url: 'https://old.example.com' }, + }; + renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + const dottedWrites = onChange.mock.calls.filter(([p]) => + typeof p === 'string' && p.startsWith('mcpServers.legacy'), ); - expect(undefinedPaths).not.toContain('mcpServers.legacy'); + expect(dottedWrites).toEqual([]); }); }); From f0f6712b53fb0326e3ff5b750ea8bec91a7de58f Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 14:05:08 -0700 Subject: [PATCH 06/28] =?UTF-8?q?=F0=9F=A7=AE=20fix:=20Preserve=20MCP=20En?= =?UTF-8?q?try=20Position=20And=20Empty-Container=20Deletes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record useMemo walked baseRecord first then appended every edited entry, so changing a single field bumped its row to the bottom of the list. The merged loop now walks baseRecord and applies the edits overlay in place, with newly-created entries appended in their editsByEntry insertion order. baselineContainerPaths walks the structured config to record every container path (including orphaned mcpServers.foo with no leaves under it). The dirty check now treats an entry-path undefined as a container delete when either the leaf-derived intermediates or the structured container set knows about it, so an orphaned empty entry can actually be removed instead of being pruned as a no-op write. Regression tests cover the in-place ordering for an edited entry, the bottom-append ordering for a freshly-created entry, and the existing handleRemove flow continues to assert leaf cleanup for non-empty entries. --- src/components/configuration/ConfigPage.tsx | 26 +++++++++-- .../sections/McpServersRenderer.tsx | 42 +++++++++++------- .../__tests__/McpServersRenderer.test.tsx | 44 +++++++++++++++++++ 3 files changed, 92 insertions(+), 20 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 67e35b0..2d3e83a 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -365,6 +365,24 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return set; }, [scopeBaseline]); + /** Container paths walked directly off the structured config, so an orphaned `{}` entry whose flatten dropped (or never produced) any leaf is still recognized as a real subtree-delete target. */ + const baselineContainerPaths = useMemo(() => { + const set = new Set(); + const walk = (obj: unknown, prefix: string): void => { + if (obj == null || typeof obj !== 'object' || Array.isArray(obj)) return; + for (const k of Object.keys(obj as Record)) { + const path = prefix ? `${prefix}.${k}` : k; + const v = (obj as Record)[k]; + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + set.add(path); + walk(v, path); + } + } + }; + walk(baseActiveConfigValues, ''); + return set; + }, [baseActiveConfigValues]); + const handleFieldChange = useCallback( (path: string, value: t.ConfigValue) => { setTouchedPaths((prev) => { @@ -380,8 +398,10 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi (typeof value === 'object' && typeof baseline === 'object' && JSON.stringify(value) === JSON.stringify(baseline)); - /** A container-path undefined write must survive; baseline only stores leaves, so it would otherwise match `undefined === undefined` and get pruned. */ - const isContainerDelete = value === undefined && baselineIntermediates.has(path); + /** A container-path undefined write must survive; baseline only stores leaves, so it would otherwise match `undefined === undefined` and get pruned. baselineContainerPaths catches the orphaned empty-object case where leaf-derived intermediates miss the entry. */ + const isContainerDelete = + value === undefined && + (baselineIntermediates.has(path) || baselineContainerPaths.has(path)); if (match && !isContainerDelete) { const next = { ...prev }; delete next[path]; @@ -405,7 +425,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return next; }); }, - [scopeBaseline, baselineIntermediates], + [scopeBaseline, baselineIntermediates, baselineContainerPaths], ); const isDirty = Object.keys(editedValues).length > 0; diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 5e0a231..cbe1a81 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -683,36 +683,44 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const record = useMemo(() => { if (editsByEntry.size === 0) return baseRecord; const result: Record = {}; - for (const [k, v] of Object.entries(baseRecord)) { - if (!editsByEntry.has(k)) { - result[k] = v; - } - } - for (const [entryKey, leafEdits] of editsByEntry) { + /** Resolves a single entry's overlay value or `undefined` when the entry should drop out (whole-entry delete with no resurrecting writes). */ + const resolveEntryValue = ( + entryKey: string, + leafEdits: Array<{ segments: string[]; value: t.ConfigValue }>, + ): t.ConfigValue | undefined => { const wholeEntryWrites = leafEdits.filter((e) => e.segments.length === 0); const leafWrites = leafEdits.filter((e) => e.segments.length > 0); - let current: t.ConfigValue | undefined; if (wholeEntryWrites.length > 0) { const last = wholeEntryWrites[wholeEntryWrites.length - 1]; - if (last.value === undefined) { - continue; - } + if (last.value === undefined) return undefined; current = last.value; } else if (entryKey in baseRecord) { current = baseRecord[entryKey]; } - - if (leafWrites.length === 0) { - if (current !== undefined) result[entryKey] = current; - continue; - } + if (leafWrites.length === 0) return current; const existingObj = isPlainObject(current) ? current : {}; - const overlay = applyLeafOverlay( + return applyLeafOverlay( existingObj, leafWrites.map((e) => [e.segments, e.value] as [string[], t.ConfigValue]), ); - result[entryKey] = overlay; + }; + + /** Walk baseRecord first so edited entries keep their original position in the list instead of jumping to the bottom. */ + for (const [k, v] of Object.entries(baseRecord)) { + const leafEdits = editsByEntry.get(k); + if (!leafEdits) { + result[k] = v; + continue; + } + const resolved = resolveEntryValue(k, leafEdits); + if (resolved !== undefined) result[k] = resolved; + } + /** Newly-created entries appear in editsByEntry insertion order at the bottom. */ + for (const [entryKey, leafEdits] of editsByEntry) { + if (entryKey in baseRecord) continue; + const resolved = resolveEntryValue(entryKey, leafEdits); + if (resolved !== undefined) result[entryKey] = resolved; } return result; }, [baseRecord, editsByEntry]); diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 8b4348a..c541641 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -366,6 +366,50 @@ describe('McpServersRenderer — rejects dotted server names', () => { }); }); +describe('McpServersRenderer — entry order', () => { + it('keeps an edited entry in its original position instead of moving it to the bottom', () => { + const baseRecord = { + alpha: { type: 'sse', url: 'https://a.example.com' }, + beta: { type: 'sse', url: 'https://b.example.com' }, + gamma: { type: 'sse', url: 'https://c.example.com' }, + }; + const editedValues = { 'mcpServers.alpha.url': 'https://a-new.example.com' }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + + const headers = container.querySelectorAll('[data-section-id^="section-mcpServers-"]'); + const renderedKeys = Array.from(headers).map((h) => + decodeURIComponent((h.getAttribute('data-section-id') ?? '').replace(/^section-mcpServers-/, '')), + ); + expect(renderedKeys).toEqual(['alpha', 'beta', 'gamma']); + }); + + it('appends a freshly-created entry at the bottom', () => { + const baseRecord = { + alpha: { type: 'sse', url: 'https://a.example.com' }, + beta: { type: 'sse', url: 'https://b.example.com' }, + }; + const editedValues = { + 'mcpServers.brandNew.type': 'sse', + 'mcpServers.brandNew.url': 'https://new.example.com', + }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + + const headers = container.querySelectorAll('[data-section-id^="section-mcpServers-"]'); + const renderedKeys = Array.from(headers).map((h) => + decodeURIComponent((h.getAttribute('data-section-id') ?? '').replace(/^section-mcpServers-/, '')), + ); + expect(renderedKeys).toEqual(['alpha', 'beta', 'brandNew']); + }); +}); + describe('McpServersRenderer — legacy dotted entry keys', () => { it('groups edits under the dotted base key instead of splitting on the first dot', () => { const baseRecord = { From edfdaaf2118f26a5705ce59bba009ccc045fe80c Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 14:10:38 -0700 Subject: [PATCH 07/28] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Clear=20Ancestor=20?= =?UTF-8?q?Container=20Deletes=20On=20Baseline-Match=20Prune?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prune branch in handleFieldChange dropped only the current leaf path from editedValues when its value returned to baseline, leaving any ancestor `undefined` write in place. After a delete-then-recreate sequence where every recreated field happens to match its baseline, the leftover `mcpServers.: undefined` survived and the record overlay continued to hide the resurrected entry. The prune branch now mirrors the dedup loop in the non-prune branch: on a baseline-match drop, any ancestor entry whose value is `undefined` is removed too. Non-undefined ancestors are untouched so explicit subtree overrides keep their semantics. --- src/components/configuration/ConfigPage.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 2d3e83a..4bcedd2 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -405,6 +405,13 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi if (match && !isContainerDelete) { const next = { ...prev }; delete next[path]; + /** Drop any orphaned ancestor `undefined` writes; a leaf returning to baseline means the user is re-asserting that subtree, so a stale "delete this whole entry" left over from a prior remove (e.g. delete-then-recreate-at-baseline) must not survive to hide the resurrected entry. */ + for (const existing of Object.keys(next)) { + if (existing === path) continue; + if (path.startsWith(`${existing}.`) && next[existing] === undefined) { + delete next[existing]; + } + } return next; } const next = { ...prev, [path]: value }; From 01626eb57b3210403e7241ce37dfd00724fe75c1 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 14:17:07 -0700 Subject: [PATCH 08/28] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Detect=20Legacy=20b?= =?UTF-8?q?aseOnly=20Backend=20Before=20Locking=20MCP=20Identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A LibreChat without the companion baseOnly support returns the same merged config payload for both /api/admin/config/base and the ?baseOnly=true variant. The previous code treated every key from the baseOnly response as YAML-defined, which on legacy backends locked admin-only servers against rename and delete. getBaseConfigFn now checks dbOverrides.mcpServers first. When admin overrides on mcpServers exist, the baseOnly response is only treated as authoritative if it differs from the merged config; when the two are byte-identical despite known overrides, the backend is legacy and yamlMcpKeys stays undefined so the renderer falls back to its safer "lock nothing" path. When no admin overrides exist on mcpServers the two responses are legitimately identical, so baseOnly is used unchanged and YAML servers remain locked. --- src/server/config.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/server/config.ts b/src/server/config.ts index 38f1243..aef5275 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -848,7 +848,17 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( const baseOnly = normalizeAppServiceKeys(baseOnlyRaw); const mcp = baseOnly.mcpServers; if (mcp && typeof mcp === 'object' && !Array.isArray(mcp)) { - yamlMcpKeys = Object.keys(mcp as Record); + /** A LibreChat that predates the companion baseOnly support returns the same merged payload for both endpoints, so the response itself is not a reliable signal. When admin overrides on mcpServers exist (so we _expect_ baseOnly to be a strict subset of regular) but the two responses are byte-identical, treat the backend as legacy and fall back to "lock nothing" rather than locking admin-only servers. */ + const mcpOverrides = + dbOverrides && typeof dbOverrides.mcpServers === 'object' && dbOverrides.mcpServers !== null + ? (dbOverrides.mcpServers as Record) + : null; + const hasMcpOverrides = !!mcpOverrides && Object.keys(mcpOverrides).length > 0; + const baseOnlyIsAuthoritative = + !hasMcpOverrides || JSON.stringify(mcp) !== JSON.stringify(config.mcpServers); + if (baseOnlyIsAuthoritative) { + yamlMcpKeys = Object.keys(mcp as Record); + } } } From 51a2001ea52bfe6dcc2c2209c7628901dd414277 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 14:28:45 -0700 Subject: [PATCH 09/28] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Sweep=20Descendant?= =?UTF-8?q?=20Edits=20On=20Parent-Level=20Writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-prune dedup loop only cleared ancestor entries (existing keys that path descends from). A subsequent parent-level write (e.g. an edit to mcpServers.bar.headers as a whole) left previously-emitted per-leaf writes like mcpServers.bar.headers.Authorization in editedValues. Both reached the field PATCH endpoint as separate entries and raced order-dependently, letting the stale leaf overwrite the user's parent edit. The loop now also removes descendants of the current path so a write at any subtree level supersedes any deeper entries beneath it. --- src/components/configuration/ConfigPage.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 4bcedd2..3b4f945 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -423,9 +423,10 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi } const indexMatch = /^(.+)\.\d+$/.exec(path); if (indexMatch) delete next[indexMatch[1]]; + /** Two-way dedup: drop ancestors that the new leaf supersedes AND descendants that the new parent supersedes. Without the descendant sweep, a rename's per-leaf writes plus a subsequent parent-level edit of the same subtree would both reach the field PATCH endpoint and race order-dependently against each other. */ for (const existing of Object.keys(next)) { if (existing === path) continue; - if (path.startsWith(`${existing}.`)) { + if (path.startsWith(`${existing}.`) || existing.startsWith(`${path}.`)) { delete next[existing]; } } From dfdde5d18e43a5c9ad71f6833a03661df33e95b4 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 14:48:01 -0700 Subject: [PATCH 10/28] =?UTF-8?q?=E2=9A=A1=20perf:=20Stabilize=20Validatio?= =?UTF-8?q?n=20And=20Localize=20Refs=20To=20Preserve=20MCP=20Row=20Memo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleCreate and handleRename listed onValidationError and localize in their useCallback dependency arrays. ConfigPage passes onValidationError as an inline arrow each render, and useLocalize returns a fresh function reference each render, so the callbacks were recreated on every parent re-render. That defeated memo(McpEntryRow) because the onRename prop changed identity every cycle and every row re-rendered even when its data was untouched. Mirror the existing editedValuesRef / baseRecordRef / recordRef pattern with refs for both onValidationError and localize, then drop those unstable values from the callback dep lists. handleCreate, handleRemove, and handleRename are now stable as long as onChange and path are stable, which they are. --- .../sections/McpServersRenderer.tsx | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index cbe1a81..545df5d 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -754,10 +754,21 @@ export function McpServersRenderer(props: t.FieldRendererProps) { recordRef.current = record; }, [record]); + /** ConfigPage passes a fresh inline arrow each render for onValidationError, and useLocalize returns a new function reference each render too; without these refs the create/rename callbacks below would change identity every render and defeat the memo on McpEntryRow. */ + const onValidationErrorRef = useRef(onValidationError); + useEffect(() => { + onValidationErrorRef.current = onValidationError; + }, [onValidationError]); + + const localizeRef = useRef(localize); + useEffect(() => { + localizeRef.current = localize; + }, [localize]); + const handleCreate = useCallback( (serverName: string, entry: Record) => { if (serverName.includes('.')) { - onValidationError?.(localize('com_config_server_name_no_dots')); + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_no_dots')); return; } if ( @@ -765,7 +776,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { serverName === 'constructor' || serverName === 'prototype' ) { - onValidationError?.(localize('com_config_server_name_invalid')); + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_invalid')); return; } for (const [fieldKey, fieldValue] of Object.entries(entry)) { @@ -784,7 +795,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { } setJustAddedKey(serverName); }, - [onChange, path, localize, onValidationError], + [onChange, path], ); const handleRemove = useCallback( @@ -830,15 +841,15 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const baseRecord = baseRecordRef.current; const record = recordRef.current; if (newKey.includes('.')) { - onValidationError?.(localize('com_config_server_name_no_dots')); + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_no_dots')); return; } if (newKey === '__proto__' || newKey === 'constructor' || newKey === 'prototype') { - onValidationError?.(localize('com_config_server_name_invalid')); + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_invalid')); return; } if (Object.hasOwn(record, newKey)) { - onValidationError?.(localize('com_config_server_name_exists')); + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_exists')); return; } const oldPrefixFull = `${path}.${oldKey}`; @@ -878,7 +889,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { /** See $unset note in handleRemove. */ onChange(`${path}.${oldKey}`, undefined); }, - [onChange, path, localize, onValidationError], + [onChange, path], ); return ( From 22e211e671bdfbb8da497d127918952d378733fd Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 15:03:25 -0700 Subject: [PATCH 11/28] =?UTF-8?q?=E2=9C=85=20fix:=20Validate=20Transport-R?= =?UTF-8?q?equired=20Fields=20At=20MCP=20Create=20Time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-leaf save granularity bypasses the whole-object Zod validation that previously caught missing required siblings, so an admin could pick transport sse and submit without a url; the resulting per-field PATCH succeeded because type=sse is a valid enum on its own and no sibling check ran on the server. The persisted entry was invalid for its transport but stuck around. CreateMcpServerDialog.handleSubmit now consults REQUIRED_BY_TRANSPORT for the effective transport (http normalized to streamable-http) and surfaces a missing-required-field error inline before onSave runs. The dialog's submit-disabled invariant already covers the always- required type field; this adds the transport-specific sibling check so a partial create never reaches the field-PATCH endpoint. --- .../sections/McpServersRenderer.tsx | 19 +++++++++++++++++++ src/locales/en/translation.json | 1 + 2 files changed, 20 insertions(+) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 545df5d..c35ad4c 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -589,6 +589,25 @@ function CreateMcpServerDialog({ if (Array.isArray(val) && val.length === 0) continue; entry[key] = val; } + /** Per-leaf saves bypass whole-object Zod validation, so a partial create (e.g. transport `sse` with no url) would persist as an invalid server. Validate transport-specific required fields here while we still hold the dialog draft. */ + const rawType = typeof entry.type === 'string' ? entry.type : ''; + const transportType = rawType || inferTransportType(entry); + const normalizedTransport = transportType === 'http' ? 'streamable-http' : transportType; + const required = REQUIRED_BY_TRANSPORT[normalizedTransport]; + if (required) { + for (const field of required) { + const val = entry[field]; + const missing = + val === undefined || + val === null || + val === '' || + (Array.isArray(val) && val.length === 0); + if (missing) { + setError(localize('com_config_server_missing_required', { field })); + return; + } + } + } onSave(name, entry); setServerName(''); setDraft({}); diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index 9a18ee2..7f0bc02 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -77,6 +77,7 @@ "com_config_server_name_exists": "A server with this name already exists", "com_config_server_name_no_dots": "Server name cannot contain dots", "com_config_server_name_invalid": "Server name is not allowed", + "com_config_server_missing_required": "Missing required field: {{field}}", "com_config_group_connection": "Connection", "com_config_group_authentication": "Authentication", "com_config_group_api_key": "API key", From 7f263005f112b6fd911f714cbcb4cec967e03294 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 29 May 2026 15:29:56 -0700 Subject: [PATCH 12/28] =?UTF-8?q?=F0=9F=94=90=20fix:=20Disable=20MCP=20Ren?= =?UTF-8?q?ame/Delete=20In=20Scope=20Mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scope-edit save path treats an undefined field write as a reset that calls the profile DELETE endpoint, which can only remove an existing scope override. Deleting an inherited base entry from a role/group scope therefore left it visible after save because no scope override existed to remove; renaming had the same problem and created a parallel scope entry alongside the surviving inherited original. Per-leaf saves cannot express a scope-level tombstone, so the proper fix needs backend support that does not exist yet. Add a scopeMode prop on FieldRendererProps and ConfigTabContentProps, plumb isEditingScope through ConfigPage so the MCP renderer knows when it is editing a scope, and treat scopeMode the same as isYamlSource / isDottedLegacy for the identity lock: the create button is disabled, an inline note explains the limitation, and McpEntryRow hides rename and delete on every entry. Field edits still produce per-leaf scope overrides as expected. Regression tests cover the create-disabled state and the absence of rename/delete affordances in scope mode, plus that a field edit still emits a per-leaf write so the user can still customize values per scope. --- src/components/configuration/ConfigPage.tsx | 1 + .../configuration/ConfigTabContent.tsx | 2 + .../sections/McpServersRenderer.tsx | 14 +++++- .../__tests__/McpServersRenderer.test.tsx | 46 +++++++++++++++++++ src/locales/en/translation.json | 1 + src/types/config-ui.ts | 4 ++ 6 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 3b4f945..4fa6bfe 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -889,6 +889,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi showConfiguredOnly={showConfiguredOnly} baseRecordKeys={baseRecordKeys} onValidationError={(message) => showToast({ type: 'error', message }, 5000)} + scopeMode={isEditingScope} /> diff --git a/src/components/configuration/ConfigTabContent.tsx b/src/components/configuration/ConfigTabContent.tsx index 20a7e01..f4bb536 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -56,6 +56,7 @@ export function ConfigTabContent({ showConfiguredOnly, baseRecordKeys, onValidationError, + scopeMode, }: t.ConfigTabContentProps) { const localize = useLocalize(); const fieldsDisabled = readOnly; @@ -184,6 +185,7 @@ export function ConfigTabContent({ showConfiguredOnly, yamlBaseKeys: baseRecordKeys?.[dataKey], onValidationError, + scopeMode, }; return ( <> diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index c35ad4c..9ce0e3d 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -673,6 +673,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { editedValues, yamlBaseKeys, onValidationError, + scopeMode, } = props; const localize = useLocalize(); const [createOpen, setCreateOpen] = useState(false); @@ -917,13 +918,18 @@ export function McpServersRenderer(props: t.FieldRendererProps) { + {scopeMode && ( +

+ {localize('com_config_mcp_scope_structural_disabled')} +

+ )} {entries.map(([key, entryValue]) => ( void; onRemove: (key: string) => void; onRename: (oldKey: string, newKey: string) => void; @@ -998,7 +1007,8 @@ const McpEntryRow = memo(function McpEntryRowImpl({ /** Dotted entry names predate the dot-rejecting create/rename validators; the save endpoint parses fieldPath as dot-delimited so any per-leaf write under such a key collides with a parallel "legacy" → "dotted" nested-object interpretation. Render them read-only so they stay visible in the list but never round-trip through the per-field save API. */ const isDottedLegacy = entryKey.includes('.'); const isReadOnly = !!disabled || isDottedLegacy; - const isLockedIdentity = isYamlSource || isDottedLegacy; + /** Scope mode can't tombstone an inherited entry through per-leaf saves alone, so rename/delete are unreachable until the backend grows tombstone support; field edits still produce per-leaf scope overrides as expected. */ + const isLockedIdentity = isYamlSource || isDottedLegacy || !!scopeMode; const lockedKeys = isYamlSource && !isDottedLegacy ? YAML_LOCKED_FIELDS : undefined; const entryOnChange = useCallback( diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index c541641..4a5e217 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -174,6 +174,7 @@ function renderRenderer({ yamlBaseKeys, onChange = vi.fn(), onValidationError = vi.fn(), + scopeMode, }: { baseRecord: Record; editedValues?: t.FlatConfigMap; @@ -181,6 +182,7 @@ function renderRenderer({ yamlBaseKeys?: Set; onChange?: (path: string, value: t.ConfigValue) => void; onValidationError?: (message: string) => void; + scopeMode?: boolean; }) { const fields = fieldsForMcp(); const props: t.FieldRendererProps = { @@ -197,6 +199,7 @@ function renderRenderer({ dbOverridePaths, yamlBaseKeys, onValidationError, + scopeMode, }; return { ...render(), @@ -366,6 +369,49 @@ describe('McpServersRenderer — rejects dotted server names', () => { }); }); +describe('McpServersRenderer — scope mode', () => { + it('disables create and hides rename/delete in scope mode', () => { + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + scopeMode: true, + }); + + const createBtn = screen.getByText('com_config_create_mcp_server') + .closest('button') as HTMLButtonElement; + expect(createBtn.hasAttribute('disabled')).toBe(true); + + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).toBeNull(); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).toBeNull(); + }); + + it('still allows field-level edits to flow as per-leaf scope overrides', () => { + const onChange = vi.fn(); + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + scopeMode: true, + onChange, + }); + + fireEvent.click(screen.getByText('adminOnly')); + const urlInput = container.querySelector('input#adminOnly-url') as HTMLInputElement | null; + expect(urlInput).not.toBeNull(); + expect(urlInput!.hasAttribute('disabled')).toBe(false); + fireEvent.change(urlInput!, { target: { value: 'https://override.example.com' } }); + fireEvent.blur(urlInput!); + + const urlCalls = onChange.mock.calls.filter(([p]) => p === 'mcpServers.adminOnly.url'); + expect(urlCalls.length).toBeGreaterThan(0); + }); +}); + describe('McpServersRenderer — entry order', () => { it('keeps an edited entry in its original position instead of moving it to the bottom', () => { const baseRecord = { diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index 7f0bc02..06019bb 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -78,6 +78,7 @@ "com_config_server_name_no_dots": "Server name cannot contain dots", "com_config_server_name_invalid": "Server name is not allowed", "com_config_server_missing_required": "Missing required field: {{field}}", + "com_config_mcp_scope_structural_disabled": "Renaming, creating, and removing MCP servers from a role or group scope is not supported. Make these changes in the base configuration; field-level overrides still work in this scope.", "com_config_group_connection": "Connection", "com_config_group_authentication": "Authentication", "com_config_group_api_key": "API key", diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index 21e4e26..0d72e7b 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -87,6 +87,8 @@ export interface ConfigTabContentProps { /** YAML-defined entry keys per section, keyed by parent path. */ baseRecordKeys?: Record>; onValidationError?: (message: string) => void; + /** True when editing a role/group scope rather than the base config; renderers may suppress structural actions (rename/delete of inherited entries) that can't be expressed without a scope-level tombstone. */ + scopeMode?: boolean; } export interface ConfigTableOfContentsProps { @@ -231,6 +233,8 @@ export interface FieldRendererProps { /** YAML-defined entry keys for the section being rendered. */ yamlBaseKeys?: Set; onValidationError?: (message: string) => void; + /** True when editing a role/group scope. Renderers that manage record entries should suppress rename/delete affordances because per-leaf saves alone can't tombstone an inherited entry. */ + scopeMode?: boolean; } export interface ImportYamlDialogProps { From 0fa52b254ac45c8c3350e07f1bdf7bfe57603edb Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 31 May 2026 23:08:08 -0700 Subject: [PATCH 13/28] =?UTF-8?q?=F0=9F=9A=A7=20fix:=20Block=20Save=20On?= =?UTF-8?q?=20Transport=20Changes=20That=20Drop=20Required=20Fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-leaf saves validate each fieldPath in isolation, so changing an existing MCP server's transport from sse to stdio queued only the mcpServers..type literal write. The server-side per-field validation accepted stdio as a valid enum on its own, persisting an entry without command/args. The create dialog already had this check, but the edit path bypassed it entirely. Add a validateMcpCrossField helper that merges the pending leaf edits onto the baseline mcpServers record (handling whole-entry deletes and upserts) and verifies REQUIRED_BY_TRANSPORT against the effective transport type for every entry touched by the edits. The helper is wired into handleConfirmSave so an invalid post-edit combination surfaces an inline error and blocks the save before any field-PATCH fires. New i18n key carries the entry name and missing field into the message. Regression tests cover the transport-switch-without-required-field case, the same switch when the new required fields are also queued, http to streamable-http normalization, and the no-flag-on-delete case so resets don't trip the validator. --- src/components/configuration/ConfigPage.tsx | 28 ++++ .../sections/McpServersRenderer.tsx | 128 ++++++++++++++++++ .../__tests__/McpServersRenderer.test.tsx | 48 +++++++ src/locales/en/translation.json | 1 + 4 files changed, 205 insertions(+) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 4fa6bfe..2e552a1 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -35,6 +35,7 @@ import { StickyActionBar } from '@/components/shared'; import { ConfigTabContent } from './ConfigTabContent'; import { ImportYamlDialog } from './ImportYamlDialog'; import { ContentToolbar } from './ContentToolbar'; +import { validateMcpCrossField } from './sections/McpServersRenderer'; import { mergeIndexedArrayEdits } from './utils'; import { SystemCapabilities } from '@/constants'; import { ConfigTabBar } from './ConfigTabBar'; @@ -508,6 +509,31 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const touched = [...touchedPaths].filter((p) => p in editedValues); if (touched.length === 0) return; + /** Per-leaf saves can land an MCP entry in a transport state whose required siblings are missing (e.g. type=stdio with no command/args). Server-side per-field validation only sees one path at a time, so do the cross-field check here against the merged effective entry before any PATCH fires. */ + const mcpBaseline = (() => { + const v = configValues?.mcpServers; + if (v && typeof v === 'object' && !Array.isArray(v)) { + return v as Record; + } + return {}; + })(); + const mcpEdits: Array<[string, t.ConfigValue]> = touched + .filter((p) => p.startsWith('mcpServers.')) + .map((p) => [p, editedValues[p]] as [string, t.ConfigValue]); + if (mcpEdits.length > 0) { + const mcpErrors = validateMcpCrossField(mcpBaseline, mcpEdits); + if (mcpErrors.length > 0) { + const { entryKey, missingField } = mcpErrors[0]; + const message = localize('com_config_mcp_invalid_after_edit', { + entry: entryKey, + field: missingField, + }); + setSaveError(message); + showToast({ type: 'error', message }, 5000); + return; + } + } + const saves = touched .filter((p) => editedValues[p] !== undefined) .map((p) => ({ @@ -579,6 +605,8 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi invalidateAndResetBase, invalidateAndResetScope, saving, + configValues, + localize, ]); const serializedEditedValues = useMemo(() => { diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 9ce0e3d..aeae6c6 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -1058,6 +1058,134 @@ const McpEntryRow = memo(function McpEntryRowImpl({ ); }); +/** Returns the entry key + field-path within it, or null if the path is not under mcpServers. */ +function splitMcpEntryPath( + fullPath: string, + knownEntryKeys: Iterable, +): { entryKey: string; field: string } | null { + if (!fullPath.startsWith('mcpServers.')) return null; + const rest = fullPath.slice('mcpServers.'.length); + if (rest === '') return null; + let best: string | null = null; + for (const key of knownEntryKeys) { + if (rest === key || rest.startsWith(`${key}.`)) { + if (best === null || key.length > best.length) best = key; + } + } + if (best !== null) { + const field = rest.length > best.length ? rest.slice(best.length + 1) : ''; + return { entryKey: best, field }; + } + const dotIdx = rest.indexOf('.'); + if (dotIdx === -1) return { entryKey: rest, field: '' }; + return { entryKey: rest.slice(0, dotIdx), field: rest.slice(dotIdx + 1) }; +} + +function setLeafByPath( + target: Record, + segments: string[], + value: t.ConfigValue, +): void { + let cursor: Record = target; + for (let i = 0; i < segments.length - 1; i++) { + const seg = segments[i]; + const next = cursor[seg]; + if (!isPlainObject(next)) cursor[seg] = {}; + cursor = cursor[seg] as Record; + } + const leaf = segments[segments.length - 1]; + if (value === undefined) delete cursor[leaf]; + else cursor[leaf] = value; +} + +/** Merges leaf edits onto the baseline MCP entries (omitting deleted entries) so callers can validate the post-save state. */ +export function mergeMcpEdits( + baseline: Record, + edits: Array<[string, t.ConfigValue]>, +): Record { + const baseEntries: Record> = {}; + for (const [k, v] of Object.entries(baseline)) { + if (isPlainObject(v)) baseEntries[k] = JSON.parse(JSON.stringify(v)); + } + const knownKeys = new Set(Object.keys(baseEntries)); + const deletedEntries = new Set(); + const upsertEntries: Record> = {}; + + for (const [path, value] of edits) { + const split = splitMcpEntryPath(path, knownKeys); + if (!split) continue; + const { entryKey, field } = split; + knownKeys.add(entryKey); + if (field === '') { + if (value === undefined) { + deletedEntries.add(entryKey); + delete upsertEntries[entryKey]; + delete baseEntries[entryKey]; + } else if (isPlainObject(value)) { + upsertEntries[entryKey] = JSON.parse(JSON.stringify(value)); + deletedEntries.delete(entryKey); + } + continue; + } + if (deletedEntries.has(entryKey) && value !== undefined) { + deletedEntries.delete(entryKey); + } + if (!upsertEntries[entryKey]) { + upsertEntries[entryKey] = baseEntries[entryKey] ?? {}; + } + setLeafByPath(upsertEntries[entryKey], field.split('.'), value); + } + + const result: Record = {}; + for (const [k, v] of Object.entries(baseEntries)) { + if (deletedEntries.has(k)) continue; + if (k in upsertEntries) continue; + result[k] = v; + } + for (const [k, v] of Object.entries(upsertEntries)) { + if (deletedEntries.has(k)) continue; + result[k] = v; + } + return result; +} + +/** Returns a list of validation errors for affected MCP entries after applying edits, so the save flow can block invalid transport-state combinations the per-leaf PATCH cannot catch. */ +export function validateMcpCrossField( + baseline: Record, + edits: Array<[string, t.ConfigValue]>, +): Array<{ entryKey: string; missingField: string }> { + const merged = mergeMcpEdits(baseline, edits); + const knownKeys = new Set(Object.keys(baseline)); + const affected = new Set(); + for (const [path] of edits) { + const split = splitMcpEntryPath(path, knownKeys); + if (split) affected.add(split.entryKey); + } + const errors: Array<{ entryKey: string; missingField: string }> = []; + for (const entryKey of affected) { + const entry = merged[entryKey]; + if (!isPlainObject(entry)) continue; + const rawType = typeof entry.type === 'string' ? entry.type : ''; + const transportType = rawType || inferTransportType(entry); + const normalized = transportType === 'http' ? 'streamable-http' : transportType; + const required = REQUIRED_BY_TRANSPORT[normalized]; + if (!required) continue; + for (const field of required) { + const v = entry[field]; + const missing = + v === undefined || + v === null || + v === '' || + (Array.isArray(v) && v.length === 0); + if (missing) { + errors.push({ entryKey, missingField: field }); + break; + } + } + } + return errors; +} + export { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED, diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 4a5e217..7159b32 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -6,6 +6,7 @@ import { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED, enumerateLeafPaths, + validateMcpCrossField, } from '../McpServersRenderer'; import { createField } from '@/test/fixtures'; @@ -369,6 +370,53 @@ describe('McpServersRenderer — rejects dotted server names', () => { }); }); +describe('validateMcpCrossField', () => { + it('flags an existing sse entry whose transport is changed to stdio without command/args', () => { + const baseline = { + foo: { type: 'sse', url: 'https://x.example.com' }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo.type', 'stdio'], + ]); + expect(errors.length).toBe(1); + expect(errors[0].entryKey).toBe('foo'); + expect(errors[0].missingField).toBe('command'); + }); + + it('passes when a transport switch is accompanied by the new required fields', () => { + const baseline = { + foo: { type: 'sse', url: 'https://x.example.com' }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo.type', 'stdio'], + ['mcpServers.foo.command', 'node'], + ['mcpServers.foo.args', ['index.js']], + ]); + expect(errors).toEqual([]); + }); + + it('treats http as streamable-http and ignores entries with no edits', () => { + const baseline = { + foo: { type: 'streamable-http', url: 'https://x.example.com' }, + bar: { type: 'stdio', command: 'node', args: ['x.js'] }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo.type', 'http'], + ]); + expect(errors).toEqual([]); + }); + + it('does not flag a deleted entry', () => { + const baseline = { + foo: { type: 'sse', url: 'https://x.example.com' }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo', undefined], + ]); + expect(errors).toEqual([]); + }); +}); + describe('McpServersRenderer — scope mode', () => { it('disables create and hides rename/delete in scope mode', () => { const baseRecord = { diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index 06019bb..0cdf77a 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -79,6 +79,7 @@ "com_config_server_name_invalid": "Server name is not allowed", "com_config_server_missing_required": "Missing required field: {{field}}", "com_config_mcp_scope_structural_disabled": "Renaming, creating, and removing MCP servers from a role or group scope is not supported. Make these changes in the base configuration; field-level overrides still work in this scope.", + "com_config_mcp_invalid_after_edit": "Cannot save: MCP server \"{{entry}}\" is missing required field \"{{field}}\" for its transport type.", "com_config_group_connection": "Connection", "com_config_group_authentication": "Authentication", "com_config_group_api_key": "API key", From cf317a6980f7a0d8543c984516e9c0b0082b62d9 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 31 May 2026 23:16:20 -0700 Subject: [PATCH 14/28] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Validate=20Scoped?= =?UTF-8?q?=20MCP=20Edits=20Against=20Scope-Resolved=20Baseline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleConfirmSave's cross-field check pulled mcpServers from configValues, which is always the base config. In scope mode a prior scope override may supply the very fields the validator expects, so flipping type=stdio on a scoped server whose command and args live in the scope tier produced a false "missing command" error and blocked the save. Use baseActiveConfigValues instead, which already resolves to scopeConfigValues in scope mode and configValues otherwise, so the validator sees the effective baseline the user is actually editing. Also consolidate setLeaf and setLeafByPath — they were functionally identical, the duplicate snuck in with the validateMcpCrossField addition. The remaining setLeaf uses isPlainObject for the intermediate-shape check so the two callers (per-leaf overlay and mergeMcpEdits) share one shape rule. --- src/components/configuration/ConfigPage.tsx | 6 ++-- .../sections/McpServersRenderer.tsx | 31 +++---------------- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 2e552a1..adf96c4 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -509,9 +509,9 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const touched = [...touchedPaths].filter((p) => p in editedValues); if (touched.length === 0) return; - /** Per-leaf saves can land an MCP entry in a transport state whose required siblings are missing (e.g. type=stdio with no command/args). Server-side per-field validation only sees one path at a time, so do the cross-field check here against the merged effective entry before any PATCH fires. */ + /** Per-leaf saves can land an MCP entry in a transport state whose required siblings are missing (e.g. type=stdio with no command/args). Server-side per-field validation only sees one path at a time, so do the cross-field check here against the merged effective entry before any PATCH fires. Use baseActiveConfigValues so scope-mode edits validate against the scope-resolved baseline (where prior scope overrides supply some required fields) instead of the base config alone. */ const mcpBaseline = (() => { - const v = configValues?.mcpServers; + const v = baseActiveConfigValues?.mcpServers; if (v && typeof v === 'object' && !Array.isArray(v)) { return v as Record; } @@ -605,7 +605,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi invalidateAndResetBase, invalidateAndResetScope, saving, - configValues, + baseActiveConfigValues, localize, ]); diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index aeae6c6..85dd7a5 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -80,18 +80,12 @@ function setLeaf( let cursor: Record = target; for (let i = 0; i < segments.length - 1; i++) { const seg = segments[i]; - const next = cursor[seg]; - if (next == null || typeof next !== 'object' || Array.isArray(next)) { - cursor[seg] = {}; - } + if (!isPlainObject(cursor[seg])) cursor[seg] = {}; cursor = cursor[seg] as Record; } const leaf = segments[segments.length - 1]; - if (value === undefined) { - delete cursor[leaf]; - } else { - cursor[leaf] = value; - } + if (value === undefined) delete cursor[leaf]; + else cursor[leaf] = value; } function applyLeafOverlay( @@ -1081,23 +1075,6 @@ function splitMcpEntryPath( return { entryKey: rest.slice(0, dotIdx), field: rest.slice(dotIdx + 1) }; } -function setLeafByPath( - target: Record, - segments: string[], - value: t.ConfigValue, -): void { - let cursor: Record = target; - for (let i = 0; i < segments.length - 1; i++) { - const seg = segments[i]; - const next = cursor[seg]; - if (!isPlainObject(next)) cursor[seg] = {}; - cursor = cursor[seg] as Record; - } - const leaf = segments[segments.length - 1]; - if (value === undefined) delete cursor[leaf]; - else cursor[leaf] = value; -} - /** Merges leaf edits onto the baseline MCP entries (omitting deleted entries) so callers can validate the post-save state. */ export function mergeMcpEdits( baseline: Record, @@ -1133,7 +1110,7 @@ export function mergeMcpEdits( if (!upsertEntries[entryKey]) { upsertEntries[entryKey] = baseEntries[entryKey] ?? {}; } - setLeafByPath(upsertEntries[entryKey], field.split('.'), value); + setLeaf(upsertEntries[entryKey], field.split('.'), value); } const result: Record = {}; From 62119ad1e26c69e0e3703642c2673824c91e6423 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 31 May 2026 23:24:01 -0700 Subject: [PATCH 15/28] =?UTF-8?q?=F0=9F=AA=9C=20fix:=20Treat=20Scope-Mode?= =?UTF-8?q?=20Field=20Resets=20As=20Inheriting=20Base=20Value?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateMcpCrossField treated `mcpServers.foo.url = undefined` as "delete this leaf" when applying the merge. In scope mode the save calls removeFieldProfileValueFn, which only removes the scope-tier override and reveals the inherited base value, so a reset that the user expects to surface the base URL was instead modeled as a fully deleted leaf and the save was blocked as "missing required field". mergeMcpEdits and validateMcpCrossField now accept an optional resetFallback baseline. When a leaf undefined write is applied in scope mode, the merge looks up the same path in resetFallback and substitutes that inherited value; whole-entry undefined writes fall back to the inherited entry the same way. handleConfirmSave passes configValues.mcpServers as the fallback when isEditingScope is true and leaves it undefined in base mode, where there is nothing to inherit from. Regression tests cover both directions: a scope reset that reveals a valid base url passes, and the same reset with no base entry to fall back to still flags missing url. --- src/components/configuration/ConfigPage.tsx | 13 +++++- .../sections/McpServersRenderer.tsx | 40 +++++++++++++++---- .../__tests__/McpServersRenderer.test.tsx | 30 ++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index adf96c4..464804a 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -520,8 +520,17 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const mcpEdits: Array<[string, t.ConfigValue]> = touched .filter((p) => p.startsWith('mcpServers.')) .map((p) => [p, editedValues[p]] as [string, t.ConfigValue]); + /** In scope mode, a leaf reset (undefined write) only removes the scope override and reveals the inherited base value, so feed configValues as the resetFallback. In base mode there is nothing to inherit from. */ + const mcpResetFallback = (() => { + if (!isEditingScope) return undefined; + const v = configValues?.mcpServers; + if (v && typeof v === 'object' && !Array.isArray(v)) { + return v as Record; + } + return undefined; + })(); if (mcpEdits.length > 0) { - const mcpErrors = validateMcpCrossField(mcpBaseline, mcpEdits); + const mcpErrors = validateMcpCrossField(mcpBaseline, mcpEdits, mcpResetFallback); if (mcpErrors.length > 0) { const { entryKey, missingField } = mcpErrors[0]; const message = localize('com_config_mcp_invalid_after_edit', { @@ -606,6 +615,8 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi invalidateAndResetScope, saving, baseActiveConfigValues, + configValues, + isEditingScope, localize, ]); diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 85dd7a5..423023a 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -1075,10 +1075,23 @@ function splitMcpEntryPath( return { entryKey: rest.slice(0, dotIdx), field: rest.slice(dotIdx + 1) }; } -/** Merges leaf edits onto the baseline MCP entries (omitting deleted entries) so callers can validate the post-save state. */ +function lookupLeaf( + obj: unknown, + segments: string[], +): t.ConfigValue | undefined { + let cursor: unknown = obj; + for (const seg of segments) { + if (!cursor || typeof cursor !== 'object' || Array.isArray(cursor)) return undefined; + cursor = (cursor as Record)[seg]; + } + return cursor as t.ConfigValue | undefined; +} + +/** Merges leaf edits onto the baseline MCP entries (omitting deleted entries) so callers can validate the post-save state. `resetFallback`, when provided, supplies the value that an `undefined` leaf write would reveal after the actual save (e.g. in scope mode, a leaf reset removes the scope override and exposes the inherited base value). */ export function mergeMcpEdits( baseline: Record, edits: Array<[string, t.ConfigValue]>, + resetFallback?: Record, ): Record { const baseEntries: Record> = {}; for (const [k, v] of Object.entries(baseline)) { @@ -1095,9 +1108,15 @@ export function mergeMcpEdits( knownKeys.add(entryKey); if (field === '') { if (value === undefined) { - deletedEntries.add(entryKey); - delete upsertEntries[entryKey]; - delete baseEntries[entryKey]; + const fallback = resetFallback?.[entryKey]; + if (isPlainObject(fallback)) { + upsertEntries[entryKey] = JSON.parse(JSON.stringify(fallback)); + deletedEntries.delete(entryKey); + } else { + deletedEntries.add(entryKey); + delete upsertEntries[entryKey]; + delete baseEntries[entryKey]; + } } else if (isPlainObject(value)) { upsertEntries[entryKey] = JSON.parse(JSON.stringify(value)); deletedEntries.delete(entryKey); @@ -1110,7 +1129,13 @@ export function mergeMcpEdits( if (!upsertEntries[entryKey]) { upsertEntries[entryKey] = baseEntries[entryKey] ?? {}; } - setLeaf(upsertEntries[entryKey], field.split('.'), value); + const segments = field.split('.'); + if (value === undefined && resetFallback) { + const inherited = lookupLeaf(resetFallback[entryKey], segments); + setLeaf(upsertEntries[entryKey], segments, inherited as t.ConfigValue); + } else { + setLeaf(upsertEntries[entryKey], segments, value); + } } const result: Record = {}; @@ -1126,12 +1151,13 @@ export function mergeMcpEdits( return result; } -/** Returns a list of validation errors for affected MCP entries after applying edits, so the save flow can block invalid transport-state combinations the per-leaf PATCH cannot catch. */ +/** Returns a list of validation errors for affected MCP entries after applying edits, so the save flow can block invalid transport-state combinations the per-leaf PATCH cannot catch. `resetFallback` is the inherited baseline (e.g. base config in scope mode) whose values get revealed when a scope reset removes an override. */ export function validateMcpCrossField( baseline: Record, edits: Array<[string, t.ConfigValue]>, + resetFallback?: Record, ): Array<{ entryKey: string; missingField: string }> { - const merged = mergeMcpEdits(baseline, edits); + const merged = mergeMcpEdits(baseline, edits, resetFallback); const knownKeys = new Set(Object.keys(baseline)); const affected = new Set(); for (const [path] of edits) { diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 7159b32..bfc9a87 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -415,6 +415,36 @@ describe('validateMcpCrossField', () => { ]); expect(errors).toEqual([]); }); + + it('treats a scope-mode leaf reset as revealing the inherited base value', () => { + /** In scope mode: scopeBaseline has the scope-overridden url; configValues (base) has the inherited url. Resetting mcpServers.foo.url should reveal the base url, so the validator must not flag missing url. */ + const scopeBaseline = { + foo: { type: 'sse', url: 'https://scope-override.example.com' }, + }; + const baseFallback = { + foo: { type: 'sse', url: 'https://base.example.com' }, + }; + const errors = validateMcpCrossField( + scopeBaseline, + [['mcpServers.foo.url', undefined]], + baseFallback, + ); + expect(errors).toEqual([]); + }); + + it('still flags a scope reset when no base inheritance is available', () => { + const scopeBaseline = { + foo: { type: 'sse', url: 'https://scope-only.example.com' }, + }; + const baseFallback = {}; + const errors = validateMcpCrossField( + scopeBaseline, + [['mcpServers.foo.url', undefined]], + baseFallback, + ); + expect(errors.length).toBe(1); + expect(errors[0].missingField).toBe('url'); + }); }); describe('McpServersRenderer — scope mode', () => { From 4ee8eedf9ed711e9b67cdd90a51890d24bbad3f5 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 31 May 2026 23:32:36 -0700 Subject: [PATCH 16/28] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Remove=20Duplicate?= =?UTF-8?q?=20isEditingScope=20From=20handleConfirmSave=20Dep=20Array?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isEditingScope was listed twice in the useCallback deps. Drop the duplicate; the existing entry above already covers it. --- src/components/configuration/ConfigPage.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 464804a..1390445 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -616,7 +616,6 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi saving, baseActiveConfigValues, configValues, - isEditingScope, localize, ]); From d02290618130b2ec01b9df0200c2b4fd4c8e2b2f Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 31 May 2026 23:47:56 -0700 Subject: [PATCH 17/28] =?UTF-8?q?=F0=9F=A7=BC=20chore:=20Tighten=20MCP=20R?= =?UTF-8?q?enderer=20Internals=20From=20Cursor=20Review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three low-severity hygiene items: mergeMcpEdits is now file-private; its only caller is the in-file validateMcpCrossField. REQUIRED_BY_TRANSPORT and inferTransportType drop out of the re-export block because nothing outside this file imports them. mergeMcpEdits clones an entry on promote from baseEntries to upsertEntries instead of sharing the reference, so the in-place setLeaf writes can never leak back into baseEntries. The assembly loop relied on baseEntries staying intact for entries that never received an upsert; the shared-reference shape would have broken that invariant the moment the function grew a second read of baseEntries after the upsert loop. handleRename uses overlayBySeg.has(segKey) for the membership check on the new key instead of comparing the looked-up value to undefined, so a future overlay that intentionally carries an undefined leaf would still be written to the renamed key as an explicit reset rather than silently dropped. --- .../sections/McpServersRenderer.tsx | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 423023a..9f3410b 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -894,9 +894,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { for (const segKey of allSegKeys) { const segments = segKey.split('.'); - const leafValue = overlayBySeg.get(segKey); - if (leafValue !== undefined) { - onChange(`${newPrefix}${segments.join('.')}`, leafValue); + if (overlayBySeg.has(segKey)) { + onChange(`${newPrefix}${segments.join('.')}`, overlayBySeg.get(segKey)); } onChange(`${oldPrefix}${segments.join('.')}`, undefined); } @@ -1088,7 +1087,7 @@ function lookupLeaf( } /** Merges leaf edits onto the baseline MCP entries (omitting deleted entries) so callers can validate the post-save state. `resetFallback`, when provided, supplies the value that an `undefined` leaf write would reveal after the actual save (e.g. in scope mode, a leaf reset removes the scope override and exposes the inherited base value). */ -export function mergeMcpEdits( +function mergeMcpEdits( baseline: Record, edits: Array<[string, t.ConfigValue]>, resetFallback?: Record, @@ -1127,7 +1126,9 @@ export function mergeMcpEdits( deletedEntries.delete(entryKey); } if (!upsertEntries[entryKey]) { - upsertEntries[entryKey] = baseEntries[entryKey] ?? {}; + /** Clone on promote so subsequent setLeaf mutations don't bleed into baseEntries; the assembly loop relies on baseEntries staying intact for entries that never received an upsert. */ + const seed = baseEntries[entryKey]; + upsertEntries[entryKey] = seed ? deepClone(seed) : {}; } const segments = field.split('.'); if (value === undefined && resetFallback) { @@ -1189,10 +1190,4 @@ export function validateMcpCrossField( return errors; } -export { - YAML_LOCKED_FIELDS, - INSPECTOR_DERIVED, - REQUIRED_BY_TRANSPORT, - inferTransportType, - enumerateLeafPaths, -}; +export { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED, enumerateLeafPaths }; From 403492b011aeed97ebf69b0f71c27d8f222aeee9 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 00:38:53 -0700 Subject: [PATCH 18/28] =?UTF-8?q?=F0=9F=9B=9F=20fix:=20Preserve=20Entry-Le?= =?UTF-8?q?vel=20Delete=20Through=20Recreate=20For=20MCP=20And=20Other=20C?= =?UTF-8?q?ontainers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleFieldChange now keeps an ancestor-undefined edit (the \"delete this whole subtree\" intent) alive when a descendant write arrives, and stops dropping the new leaf when its value happens to match baseline if an entry-delete is still pending on its container. handleConfirmSave now awaits resets before saves so the DELETE lands before the new field PATCHes, removing the race that previously let either order win. The trigger was MCP delete-then-recreate in the same session. The two-way dedup at the bottom of handleFieldChange unconditionally swept ancestor edits whenever any descendant write came in, and the match branch dropped a baseline-matching leaf entirely while also wiping any ancestor-undefined for that path. Together with Promise.all parallel saves/resets, the recreated entry shipped without the entry-level reset and the new field PATCHes merged into stale DB fields (title, headers, etc.) from the deleted entry. deepClone collapsed to JSON.parse(JSON.stringify(...)) so arrays nested under entries are deep-copied, matching mergeMcpEdits and removing the misleading shallow v.slice() footgun the function name implied. No ConfigPage unit-test infra exists, so the regression is covered by the existing 614 tests staying green and by manual delete-then- recreate verification on dev. --- src/components/configuration/ConfigPage.tsx | 66 +++++++++---------- .../sections/McpServersRenderer.tsx | 12 +--- 2 files changed, 32 insertions(+), 46 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 1390445..92a588f 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -403,16 +403,14 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const isContainerDelete = value === undefined && (baselineIntermediates.has(path) || baselineContainerPaths.has(path)); - if (match && !isContainerDelete) { + /** When the user deleted a container entry and is now writing descendants under it (delete-then-recreate of an MCP server), the new leaf must persist even if it matches baseline so the post-DELETE recreate is not missing required fields, and the ancestor-undefined must outlive the descendant write so handleConfirmSave can DELETE the entry before PATCHing the new leaves. */ + const hasPendingAncestorDelete = Object.keys(prev).some( + (existing) => + existing !== path && path.startsWith(`${existing}.`) && prev[existing] === undefined, + ); + if (match && !isContainerDelete && !hasPendingAncestorDelete) { const next = { ...prev }; delete next[path]; - /** Drop any orphaned ancestor `undefined` writes; a leaf returning to baseline means the user is re-asserting that subtree, so a stale "delete this whole entry" left over from a prior remove (e.g. delete-then-recreate-at-baseline) must not survive to hide the resurrected entry. */ - for (const existing of Object.keys(next)) { - if (existing === path) continue; - if (path.startsWith(`${existing}.`) && next[existing] === undefined) { - delete next[existing]; - } - } return next; } const next = { ...prev, [path]: value }; @@ -424,10 +422,13 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi } const indexMatch = /^(.+)\.\d+$/.exec(path); if (indexMatch) delete next[indexMatch[1]]; - /** Two-way dedup: drop ancestors that the new leaf supersedes AND descendants that the new parent supersedes. Without the descendant sweep, a rename's per-leaf writes plus a subsequent parent-level edit of the same subtree would both reach the field PATCH endpoint and race order-dependently against each other. */ + /** Two-way dedup: drop ancestors that the new leaf supersedes AND descendants that the new parent supersedes. An ancestor whose value is `undefined` expresses "delete this whole subtree" and must outlive subsequent descendant writes so DELETE-then-PATCH ordering at save time can fully replace the entry instead of leaking stale fields. */ for (const existing of Object.keys(next)) { if (existing === path) continue; - if (path.startsWith(`${existing}.`) || existing.startsWith(`${path}.`)) { + const newIsDescendant = path.startsWith(`${existing}.`); + const newIsAncestor = existing.startsWith(`${path}.`); + if (newIsDescendant && next[existing] === undefined) continue; + if (newIsDescendant || newIsAncestor) { delete next[existing]; } } @@ -558,42 +559,37 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi showToast({ type: 'saving' }); try { - const promises: Promise[] = []; - - if (saves.length > 0) { - if (isEditingScope) { - promises.push( - bulkSaveProfileValuesFn({ + /** Resets must land before saves so a delete-then-recreate at the same path (e.g. MCP entry replaced with different fields) wipes stale fields first and the new leaf PATCHes don't race against the DELETE. */ + if (resets.length > 0) { + const resetPromises = resets.map((fieldPath) => { + if (isEditingScope) { + return removeFieldProfileValueFn({ data: { + fieldPath, principalType: editingScope!.principalType, principalId: editingScope!.principalId, - entries: saves, }, - }), - ); - } else { - promises.push(saveBaseConfigFn({ data: { entries: saves } })); - } + }); + } + return resetBaseConfigFieldFn({ data: { fieldPath } }); + }); + await Promise.all(resetPromises); } - for (const fieldPath of resets) { + if (saves.length > 0) { if (isEditingScope) { - promises.push( - removeFieldProfileValueFn({ - data: { - fieldPath, - principalType: editingScope!.principalType, - principalId: editingScope!.principalId, - }, - }), - ); + await bulkSaveProfileValuesFn({ + data: { + principalType: editingScope!.principalType, + principalId: editingScope!.principalId, + entries: saves, + }, + }); } else { - promises.push(resetBaseConfigFieldFn({ data: { fieldPath } })); + await saveBaseConfigFn({ data: { entries: saves } }); } } - await Promise.all(promises); - if (isEditingScope) { invalidateAndResetScope(); } else { diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 9f3410b..776c6d0 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -100,17 +100,7 @@ function applyLeafOverlay( } function deepClone(value: Record): Record { - const result: Record = {}; - for (const [k, v] of Object.entries(value)) { - if (v !== null && typeof v === 'object' && !Array.isArray(v)) { - result[k] = deepClone(v as Record); - } else if (Array.isArray(v)) { - result[k] = v.slice(); - } else { - result[k] = v; - } - } - return result; + return JSON.parse(JSON.stringify(value)) as Record; } function isPlainObject(value: t.ConfigValue): value is Record { From 1b1de47ac7cd3a4ce9a9a1946dded9028f3ad932 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 01:05:07 -0700 Subject: [PATCH 19/28] =?UTF-8?q?=F0=9F=AA=9E=20fix:=20Order-Aware=20Overl?= =?UTF-8?q?ay=20Resolution=20For=20MCP=20Entry=20Recreate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveEntryValue inside the renderer's record useMemo short-circuited to undefined whenever the last whole-entry write was a delete, even if per-leaf writes after the delete were already creating a new entry. With the prior fix that preserves the entry-undefined alongside the recreate's leaf writes (403492b), this hid the recreated entry in the UI until save. The resolver now walks edits in insertion order, anchors the seed at the most recent whole-entry write (an undefined seed becomes an empty object so subsequent leaves attach to it), and applies the leaf writes that appear after that seed. Pure delete (no recreating leaves) still hides the entry; delete-then-recreate shows the new entry with its new fields immediately. Also collapse the duplicated longest-prefix logic in splitMcpEntryPath into resolveEntryKey so mergeMcpEdits and validateMcpCrossField share the single implementation that the renderer's edit grouping already uses. Two regression tests on the renderer cover the delete-then-recreate visible-in-UI path and the plain-delete hidden-in-UI path. --- .../sections/McpServersRenderer.tsx | 78 +++++++++---------- .../__tests__/McpServersRenderer.test.tsx | 35 +++++++++ 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 776c6d0..1a8647b 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -687,26 +687,37 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const record = useMemo(() => { if (editsByEntry.size === 0) return baseRecord; const result: Record = {}; - /** Resolves a single entry's overlay value or `undefined` when the entry should drop out (whole-entry delete with no resurrecting writes). */ + /** Resolves a single entry's overlay value or `undefined` when the entry should drop out. Iterates edits in insertion order so a whole-entry delete followed by recreating per-leaf writes shows the new entry, not the deleted one. The seed is the most recent whole-entry write (`undefined` becomes an empty object so subsequent leaves attach to it); without a whole-entry write the seed is the baseRecord entry. */ const resolveEntryValue = ( entryKey: string, leafEdits: Array<{ segments: string[]; value: t.ConfigValue }>, ): t.ConfigValue | undefined => { - const wholeEntryWrites = leafEdits.filter((e) => e.segments.length === 0); - const leafWrites = leafEdits.filter((e) => e.segments.length > 0); - let current: t.ConfigValue | undefined; - if (wholeEntryWrites.length > 0) { - const last = wholeEntryWrites[wholeEntryWrites.length - 1]; - if (last.value === undefined) return undefined; - current = last.value; - } else if (entryKey in baseRecord) { - current = baseRecord[entryKey]; + let seed: t.ConfigValue | undefined = baseRecord[entryKey]; + let seedFromDelete = false; + let seedIndex = -1; + for (let i = 0; i < leafEdits.length; i++) { + const edit = leafEdits[i]; + if (edit.segments.length === 0) { + if (edit.value === undefined) { + seed = {}; + seedFromDelete = true; + } else { + seed = edit.value; + seedFromDelete = false; + } + seedIndex = i; + } } - if (leafWrites.length === 0) return current; - const existingObj = isPlainObject(current) ? current : {}; + const subsequentLeaves = leafEdits + .slice(seedIndex + 1) + .filter((e) => e.segments.length > 0); + if (subsequentLeaves.length === 0) { + return seedFromDelete ? undefined : seed; + } + const existingObj = isPlainObject(seed) ? seed : {}; return applyLeafOverlay( existingObj, - leafWrites.map((e) => [e.segments, e.value] as [string[], t.ConfigValue]), + subsequentLeaves.map((e) => [e.segments, e.value] as [string[], t.ConfigValue]), ); }; @@ -1041,29 +1052,6 @@ const McpEntryRow = memo(function McpEntryRowImpl({ ); }); -/** Returns the entry key + field-path within it, or null if the path is not under mcpServers. */ -function splitMcpEntryPath( - fullPath: string, - knownEntryKeys: Iterable, -): { entryKey: string; field: string } | null { - if (!fullPath.startsWith('mcpServers.')) return null; - const rest = fullPath.slice('mcpServers.'.length); - if (rest === '') return null; - let best: string | null = null; - for (const key of knownEntryKeys) { - if (rest === key || rest.startsWith(`${key}.`)) { - if (best === null || key.length > best.length) best = key; - } - } - if (best !== null) { - const field = rest.length > best.length ? rest.slice(best.length + 1) : ''; - return { entryKey: best, field }; - } - const dotIdx = rest.indexOf('.'); - if (dotIdx === -1) return { entryKey: rest, field: '' }; - return { entryKey: rest.slice(0, dotIdx), field: rest.slice(dotIdx + 1) }; -} - function lookupLeaf( obj: unknown, segments: string[], @@ -1091,11 +1079,12 @@ function mergeMcpEdits( const upsertEntries: Record> = {}; for (const [path, value] of edits) { - const split = splitMcpEntryPath(path, knownKeys); - if (!split) continue; - const { entryKey, field } = split; + if (!path.startsWith('mcpServers.')) continue; + const parsed = resolveEntryKey(path.slice('mcpServers.'.length), knownKeys); + if (!parsed) continue; + const { entryKey, segments } = parsed; knownKeys.add(entryKey); - if (field === '') { + if (segments.length === 0) { if (value === undefined) { const fallback = resetFallback?.[entryKey]; if (isPlainObject(fallback)) { @@ -1120,7 +1109,6 @@ function mergeMcpEdits( const seed = baseEntries[entryKey]; upsertEntries[entryKey] = seed ? deepClone(seed) : {}; } - const segments = field.split('.'); if (value === undefined && resetFallback) { const inherited = lookupLeaf(resetFallback[entryKey], segments); setLeaf(upsertEntries[entryKey], segments, inherited as t.ConfigValue); @@ -1152,8 +1140,12 @@ export function validateMcpCrossField( const knownKeys = new Set(Object.keys(baseline)); const affected = new Set(); for (const [path] of edits) { - const split = splitMcpEntryPath(path, knownKeys); - if (split) affected.add(split.entryKey); + if (!path.startsWith('mcpServers.')) continue; + const parsed = resolveEntryKey(path.slice('mcpServers.'.length), knownKeys); + if (parsed) { + knownKeys.add(parsed.entryKey); + affected.add(parsed.entryKey); + } } const errors: Array<{ entryKey: string; missingField: string }> = []; for (const entryKey of affected) { diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index bfc9a87..2688a2d 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -809,6 +809,41 @@ describe('McpServersRenderer — record overlay', () => { expect(urlInput).not.toBeNull(); expect(urlInput!.value).toBe('https://overlay.example.com'); }); + + it('shows the recreated entry when editedValues holds both a whole-entry delete and subsequent leaf writes', () => { + const baseRecord = { + kapa: { type: 'sse', url: 'https://old.example.com' }, + }; + /** Order matters: the delete write must come before the recreate leaves so resolveEntryValue treats it as delete-then-recreate, not recreate-then-delete. */ + const editedValues = { + 'mcpServers.kapa': undefined, + 'mcpServers.kapa.type': 'sse', + 'mcpServers.kapa.url': 'https://new.example.com', + }; + const { container } = renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + expect(screen.getByText('kapa')).toBeTruthy(); + fireEvent.click(screen.getByText('kapa')); + const urlInput = container.querySelector('input#kapa-url') as HTMLInputElement | null; + expect(urlInput).not.toBeNull(); + expect(urlInput!.value).toBe('https://new.example.com'); + }); + + it('hides the entry when the last whole-entry write is a delete with no subsequent leaf writes', () => { + const baseRecord = { + kapa: { type: 'sse', url: 'https://old.example.com' }, + }; + const editedValues = { 'mcpServers.kapa': undefined }; + renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + }); + expect(screen.queryByText('kapa')).toBeNull(); + }); }); describe('McpServersRenderer — handleCreate per-leaf writes', () => { From 62e615ed07577bece89df8fc9e9ece75f5c70956 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 11:39:34 -0700 Subject: [PATCH 20/28] =?UTF-8?q?=F0=9F=AA=A6=20feat:=20Write=20Scope-Mode?= =?UTF-8?q?=20MCP=20Deletes=20As=20Tombstone=20Null=20Values?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scope-mode lock added in 7f26300 was a placeholder while the backend lacked any way to represent "this scope explicitly does not have this inherited entry." With the tombstone semantics now in the LibreChat merge layer, the lock can come off and scope-level create / rename / delete all work end-to-end. handleRemove and handleRename now write null at the entry path in scope mode instead of undefined. Undefined still routes through the DELETE-field-path endpoint and is the right shape for base mode where Mongo $unset cleanly collapses the subtree; null routes through PATCH and lands as the override sentinel the merge layer recognizes as "hide this entry for this scope." resolveEntryValue treats a null entry-path edit the same as undefined for view purposes so the tombstoned entry disappears from the UI immediately, before the save round-trip. The com_config_mcp_scope_structural_disabled string and the inline note that referenced it are removed. The disabled state on the create button and the rename/delete affordances in McpEntryRow no longer special-case scopeMode. Tests cover the new scope behavior (create enabled, rename/delete present, null at entry path on delete and rename, tombstone hides from view) and the regression boundary (base mode still writes undefined, not null). Requires danny-avila/LibreChat fix/mcp-scope-tombstones (the merge layer change) to be deployed for the round-trip to work; without it the null lands in MongoDB but the merge layer still treats it as a literal null value rather than a tombstone. --- .../sections/McpServersRenderer.tsx | 28 +++--- .../__tests__/McpServersRenderer.test.tsx | 99 ++++++++++++++++++- src/locales/en/translation.json | 1 - 3 files changed, 108 insertions(+), 20 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 1a8647b..a7937e0 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -698,7 +698,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { for (let i = 0; i < leafEdits.length; i++) { const edit = leafEdits[i]; if (edit.segments.length === 0) { - if (edit.value === undefined) { + /** `null` is the scope-mode tombstone shape; semantically equivalent to a delete from the view perspective. */ + if (edit.value === undefined || edit.value === null) { seed = {}; seedFromDelete = true; } else { @@ -780,6 +781,11 @@ export function McpServersRenderer(props: t.FieldRendererProps) { localizeRef.current = localize; }, [localize]); + const scopeModeRef = useRef(scopeMode); + useEffect(() => { + scopeModeRef.current = scopeMode; + }, [scopeMode]); + const handleCreate = useCallback( (serverName: string, entry: Record) => { if (serverName.includes('.')) { @@ -837,9 +843,9 @@ export function McpServersRenderer(props: t.FieldRendererProps) { if (!seen.has(leafPath)) onChange(leafPath, undefined); } } - /** Entry-path delete is needed to make MongoDB's $unset collapse the subtree; per-leaf $unset alone leaves an empty parent that refetches as a phantom entry. */ + /** In scope mode write `null` instead of `undefined`: undefined routes through the DELETE-field-path endpoint and silently no-ops for inherited entries that have no scope override, whereas `null` PATCHes a tombstone the merge layer recognizes as "hide this entry for this scope." Base mode keeps `undefined` so MongoDB's $unset can collapse the subtree. */ if (!seen.has(entryPath)) { - onChange(entryPath, undefined); + onChange(entryPath, scopeModeRef.current ? null : undefined); } }, [onChange, path], @@ -900,8 +906,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { } onChange(`${oldPrefix}${segments.join('.')}`, undefined); } - /** See $unset note in handleRemove. */ - onChange(`${path}.${oldKey}`, undefined); + /** See tombstone note in handleRemove for why scope mode writes null instead of undefined. */ + onChange(`${path}.${oldKey}`, scopeModeRef.current ? null : undefined); }, [onChange, path], ); @@ -912,18 +918,13 @@ export function McpServersRenderer(props: t.FieldRendererProps) { - {scopeMode && ( -

- {localize('com_config_mcp_scope_structural_disabled')} -

- )} {entries.map(([key, entryValue]) => ( void; onRemove: (key: string) => void; onRename: (oldKey: string, newKey: string) => void; @@ -1002,7 +1000,7 @@ const McpEntryRow = memo(function McpEntryRowImpl({ const isDottedLegacy = entryKey.includes('.'); const isReadOnly = !!disabled || isDottedLegacy; /** Scope mode can't tombstone an inherited entry through per-leaf saves alone, so rename/delete are unreachable until the backend grows tombstone support; field edits still produce per-leaf scope overrides as expected. */ - const isLockedIdentity = isYamlSource || isDottedLegacy || !!scopeMode; + const isLockedIdentity = isYamlSource || isDottedLegacy; const lockedKeys = isYamlSource && !isDottedLegacy ? YAML_LOCKED_FIELDS : undefined; const entryOnChange = useCallback( diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 2688a2d..bc96b05 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -448,7 +448,7 @@ describe('validateMcpCrossField', () => { }); describe('McpServersRenderer — scope mode', () => { - it('disables create and hides rename/delete in scope mode', () => { + it('enables create and shows rename/delete affordances in scope mode (tombstone-backed)', () => { const baseRecord = { adminOnly: { type: 'sse', url: 'https://admin.example.com' }, }; @@ -460,10 +460,11 @@ describe('McpServersRenderer — scope mode', () => { const createBtn = screen.getByText('com_config_create_mcp_server') .closest('button') as HTMLButtonElement; - expect(createBtn.hasAttribute('disabled')).toBe(true); + expect(createBtn.hasAttribute('disabled')).toBe(false); - expect(container.querySelector('button[aria-label^="com_ui_delete"]')).toBeNull(); - expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).toBeNull(); + expect(container.querySelector('button[aria-label^="com_ui_delete"]')).not.toBeNull(); + fireEvent.click(screen.getByText('adminOnly')); + expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).not.toBeNull(); }); it('still allows field-level edits to flow as per-leaf scope overrides', () => { @@ -488,6 +489,96 @@ describe('McpServersRenderer — scope mode', () => { const urlCalls = onChange.mock.calls.filter(([p]) => p === 'mcpServers.adminOnly.url'); expect(urlCalls.length).toBeGreaterThan(0); }); + + it('writes null at entry-path on delete in scope mode (tombstone, not undefined)', () => { + const onChange = vi.fn(); + const baseRecord = { + inherited: { type: 'sse', url: 'https://base.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + scopeMode: true, + onChange, + }); + + const deleteBtn = container.querySelector('button[aria-label^="com_ui_delete"]') as HTMLButtonElement | null; + expect(deleteBtn).not.toBeNull(); + fireEvent.click(deleteBtn!); + + const entryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.inherited'); + expect(entryWrite).toBeDefined(); + expect(entryWrite![1]).toBeNull(); + }); + + it('writes null at old entry-path on rename in scope mode', () => { + const onChange = vi.fn(); + const baseRecord = { + inherited: { type: 'sse', url: 'https://base.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + scopeMode: true, + onChange, + }); + fireEvent.click(screen.getByText('inherited')); + const pencil = container.querySelector( + 'button[aria-label^="com_a11y_rename_entry"]', + ) as HTMLButtonElement | null; + expect(pencil).not.toBeNull(); + fireEvent.click(pencil!); + const renameInput = container.querySelector( + 'input.config-input-ghost', + ) as HTMLInputElement | null; + expect(renameInput).not.toBeNull(); + fireEvent.change(renameInput!, { target: { value: 'renamed' } }); + fireEvent.blur(renameInput!); + + const oldEntryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.inherited'); + expect(oldEntryWrite).toBeDefined(); + expect(oldEntryWrite![1]).toBeNull(); + const newLeafWrite = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.renamed.url' && v === 'https://base.example.com', + ); + expect(newLeafWrite).toBeDefined(); + }); + + it('hides an entry from the rendered list when editedValues holds a null tombstone at its entry path', () => { + const baseRecord = { + inherited: { type: 'sse', url: 'https://base.example.com' }, + other: { type: 'sse', url: 'https://other.example.com' }, + }; + const editedValues = { 'mcpServers.inherited': null as unknown as t.ConfigValue }; + renderRenderer({ + baseRecord, + editedValues, + yamlBaseKeys: new Set(), + scopeMode: true, + }); + expect(screen.queryByText('inherited')).toBeNull(); + expect(screen.getByText('other')).toBeTruthy(); + }); + + it('writes undefined (not null) at entry-path on delete in base mode', () => { + const onChange = vi.fn(); + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + const deleteBtn = container.querySelector('button[aria-label^="com_ui_delete"]') as HTMLButtonElement | null; + expect(deleteBtn).not.toBeNull(); + fireEvent.click(deleteBtn!); + + const entryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.adminOnly'); + expect(entryWrite).toBeDefined(); + expect(entryWrite![1]).toBeUndefined(); + }); }); describe('McpServersRenderer — entry order', () => { diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index 0cdf77a..0e7ffec 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -78,7 +78,6 @@ "com_config_server_name_no_dots": "Server name cannot contain dots", "com_config_server_name_invalid": "Server name is not allowed", "com_config_server_missing_required": "Missing required field: {{field}}", - "com_config_mcp_scope_structural_disabled": "Renaming, creating, and removing MCP servers from a role or group scope is not supported. Make these changes in the base configuration; field-level overrides still work in this scope.", "com_config_mcp_invalid_after_edit": "Cannot save: MCP server \"{{entry}}\" is missing required field \"{{field}}\" for its transport type.", "com_config_group_connection": "Connection", "com_config_group_authentication": "Authentication", From 4d9167f6743c5fe3be8f2785919b1859ed1af5c8 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 13:24:02 -0700 Subject: [PATCH 21/28] =?UTF-8?q?=E2=86=A9=EF=B8=8F=20revert:=20Drop=20Sco?= =?UTF-8?q?pe-Mode=20Tombstone=20Writes=20Pending=20Backend=20Tombstone=20?= =?UTF-8?q?Support?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit wrote null at the MCP entry path in scope mode on delete and rename, on the assumption a tombstone-aware merge layer would interpret null as "hide this inherited entry for this scope." That LibreChat-side change is not landing right now, so the null writes reach a backend that stores them as literal null values in the override document and surface in the effective config as mcpServers.foo = null. That is worse than the prior silent no-op because downstream code now sees a null where it expects an entry shape or absence. Revert handleRemove and handleRename to write undefined at the entry path in scope mode the same as base mode. The existing DELETE field-path path then $unsets the scope-tier override row cleanly, which is the right behavior for scope-only entries (the only case reachable through the UI today, since inherited entries are not surfaced in scope mode). Keep the broader scope-mode lock removal from the prior commit so operators can add, rename, and delete scope-only MCP servers as a normal per-scope policy lever. The inherited-entry delete path remains unimplemented and is unreachable in the UI until inherited entries are exposed in scope view and a tombstone-aware merge layer ships behind it. Tests adjusted to assert the undefined-write shape and the scope-only-entry round trip; the null-tombstone view test is removed since the path no longer fires. --- .../sections/McpServersRenderer.tsx | 17 ++---- .../__tests__/McpServersRenderer.test.tsx | 56 ++++--------------- 2 files changed, 15 insertions(+), 58 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index a7937e0..3a68619 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -657,7 +657,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { editedValues, yamlBaseKeys, onValidationError, - scopeMode, } = props; const localize = useLocalize(); const [createOpen, setCreateOpen] = useState(false); @@ -698,8 +697,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { for (let i = 0; i < leafEdits.length; i++) { const edit = leafEdits[i]; if (edit.segments.length === 0) { - /** `null` is the scope-mode tombstone shape; semantically equivalent to a delete from the view perspective. */ - if (edit.value === undefined || edit.value === null) { + if (edit.value === undefined) { seed = {}; seedFromDelete = true; } else { @@ -781,11 +779,6 @@ export function McpServersRenderer(props: t.FieldRendererProps) { localizeRef.current = localize; }, [localize]); - const scopeModeRef = useRef(scopeMode); - useEffect(() => { - scopeModeRef.current = scopeMode; - }, [scopeMode]); - const handleCreate = useCallback( (serverName: string, entry: Record) => { if (serverName.includes('.')) { @@ -843,9 +836,9 @@ export function McpServersRenderer(props: t.FieldRendererProps) { if (!seen.has(leafPath)) onChange(leafPath, undefined); } } - /** In scope mode write `null` instead of `undefined`: undefined routes through the DELETE-field-path endpoint and silently no-ops for inherited entries that have no scope override, whereas `null` PATCHes a tombstone the merge layer recognizes as "hide this entry for this scope." Base mode keeps `undefined` so MongoDB's $unset can collapse the subtree. */ + /** Entry-path delete is needed to make MongoDB's $unset collapse the subtree; per-leaf $unset alone leaves an empty parent that refetches as a phantom entry. */ if (!seen.has(entryPath)) { - onChange(entryPath, scopeModeRef.current ? null : undefined); + onChange(entryPath, undefined); } }, [onChange, path], @@ -906,8 +899,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { } onChange(`${oldPrefix}${segments.join('.')}`, undefined); } - /** See tombstone note in handleRemove for why scope mode writes null instead of undefined. */ - onChange(`${path}.${oldKey}`, scopeModeRef.current ? null : undefined); + /** See $unset note in handleRemove. */ + onChange(`${path}.${oldKey}`, undefined); }, [onChange, path], ); diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index bc96b05..033aaa3 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -490,10 +490,10 @@ describe('McpServersRenderer — scope mode', () => { expect(urlCalls.length).toBeGreaterThan(0); }); - it('writes null at entry-path on delete in scope mode (tombstone, not undefined)', () => { + it('writes undefined at entry-path on delete in scope mode (the existing DELETE field-path cleanly $unsets a scope-tier entry)', () => { const onChange = vi.fn(); const baseRecord = { - inherited: { type: 'sse', url: 'https://base.example.com' }, + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, }; const { container } = renderRenderer({ baseRecord, @@ -506,15 +506,15 @@ describe('McpServersRenderer — scope mode', () => { expect(deleteBtn).not.toBeNull(); fireEvent.click(deleteBtn!); - const entryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.inherited'); + const entryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.adminOnly'); expect(entryWrite).toBeDefined(); - expect(entryWrite![1]).toBeNull(); + expect(entryWrite![1]).toBeUndefined(); }); - it('writes null at old entry-path on rename in scope mode', () => { + it('writes undefined at old entry-path on rename in scope mode', () => { const onChange = vi.fn(); const baseRecord = { - inherited: { type: 'sse', url: 'https://base.example.com' }, + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, }; const { container } = renderRenderer({ baseRecord, @@ -522,7 +522,7 @@ describe('McpServersRenderer — scope mode', () => { scopeMode: true, onChange, }); - fireEvent.click(screen.getByText('inherited')); + fireEvent.click(screen.getByText('adminOnly')); const pencil = container.querySelector( 'button[aria-label^="com_a11y_rename_entry"]', ) as HTMLButtonElement | null; @@ -535,50 +535,14 @@ describe('McpServersRenderer — scope mode', () => { fireEvent.change(renameInput!, { target: { value: 'renamed' } }); fireEvent.blur(renameInput!); - const oldEntryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.inherited'); + const oldEntryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.adminOnly'); expect(oldEntryWrite).toBeDefined(); - expect(oldEntryWrite![1]).toBeNull(); + expect(oldEntryWrite![1]).toBeUndefined(); const newLeafWrite = onChange.mock.calls.find( - ([p, v]) => p === 'mcpServers.renamed.url' && v === 'https://base.example.com', + ([p, v]) => p === 'mcpServers.renamed.url' && v === 'https://admin.example.com', ); expect(newLeafWrite).toBeDefined(); }); - - it('hides an entry from the rendered list when editedValues holds a null tombstone at its entry path', () => { - const baseRecord = { - inherited: { type: 'sse', url: 'https://base.example.com' }, - other: { type: 'sse', url: 'https://other.example.com' }, - }; - const editedValues = { 'mcpServers.inherited': null as unknown as t.ConfigValue }; - renderRenderer({ - baseRecord, - editedValues, - yamlBaseKeys: new Set(), - scopeMode: true, - }); - expect(screen.queryByText('inherited')).toBeNull(); - expect(screen.getByText('other')).toBeTruthy(); - }); - - it('writes undefined (not null) at entry-path on delete in base mode', () => { - const onChange = vi.fn(); - const baseRecord = { - adminOnly: { type: 'sse', url: 'https://admin.example.com' }, - }; - const { container } = renderRenderer({ - baseRecord, - yamlBaseKeys: new Set(), - onChange, - }); - - const deleteBtn = container.querySelector('button[aria-label^="com_ui_delete"]') as HTMLButtonElement | null; - expect(deleteBtn).not.toBeNull(); - fireEvent.click(deleteBtn!); - - const entryWrite = onChange.mock.calls.find(([p]) => p === 'mcpServers.adminOnly'); - expect(entryWrite).toBeDefined(); - expect(entryWrite![1]).toBeUndefined(); - }); }); describe('McpServersRenderer — entry order', () => { From 7b73faa6e22819538aa60dad5aa2929ae46e7581 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:18:34 -0700 Subject: [PATCH 22/28] =?UTF-8?q?=F0=9F=A9=BA=20fix:=20Plug=20MCP=20Valida?= =?UTF-8?q?tion=20Gaps=20And=20Drop=20Dead=20Scope=20Plumbing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent fixes from a single review pass. Cross-field MCP validation now falls back to the baseline entry's transport inference when the merged entry loses its discriminator. A YAML server that omits type and relies on stdio-by-default would otherwise sneak past the missing-command check after the user cleared its command field, because inferTransportType collapsed to ''. The base-config save validator's max(100) entries cap is raised to 500 so a single MCP create with a large env or headers map cannot be rejected purely by the transport envelope. The hard limit still prevents a runaway client from spamming arbitrary writes. The scopeMode prop chain is removed. The earlier scope-mode tombstone client got reverted because the backend tombstone support was not shipping, and at that point scopeMode was no longer read anywhere in the renderer but stayed plumbed through ConfigTabContentProps, FieldRendererProps, ConfigTabContent, and ConfigPage. Cleared from the prop types, the forwarders, and the test helper. Tests cover the baseline-fallback transport check and the renamed non-YAML-entry affordance tests (formerly scope-mode tests, since the renderer no longer cares about scopeMode and the behavior is identical regardless). --- src/components/configuration/ConfigPage.tsx | 1 - .../configuration/ConfigTabContent.tsx | 2 -- .../sections/McpServersRenderer.tsx | 9 +++++- .../__tests__/McpServersRenderer.test.tsx | 30 +++++++++++-------- src/server/config.ts | 2 +- src/types/config-ui.ts | 4 --- 6 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 92a588f..f6b1bf2 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -923,7 +923,6 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi showConfiguredOnly={showConfiguredOnly} baseRecordKeys={baseRecordKeys} onValidationError={(message) => showToast({ type: 'error', message }, 5000)} - scopeMode={isEditingScope} /> diff --git a/src/components/configuration/ConfigTabContent.tsx b/src/components/configuration/ConfigTabContent.tsx index f4bb536..20a7e01 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -56,7 +56,6 @@ export function ConfigTabContent({ showConfiguredOnly, baseRecordKeys, onValidationError, - scopeMode, }: t.ConfigTabContentProps) { const localize = useLocalize(); const fieldsDisabled = readOnly; @@ -185,7 +184,6 @@ export function ConfigTabContent({ showConfiguredOnly, yamlBaseKeys: baseRecordKeys?.[dataKey], onValidationError, - scopeMode, }; return ( <> diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 3a68619..ca6f290 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -1143,7 +1143,14 @@ export function validateMcpCrossField( const entry = merged[entryKey]; if (!isPlainObject(entry)) continue; const rawType = typeof entry.type === 'string' ? entry.type : ''; - const transportType = rawType || inferTransportType(entry); + /** When the user clears the field that was inferring transport on an inferred-stdio entry (no explicit `type`), `inferTransportType(entry)` collapses to '' and the validator would skip required-field checks. Fall back to the baseline's inference so a stdio-by-default server still trips the missing-command check after its discriminator gets cleared. */ + const baselineEntry = isPlainObject(baseline[entryKey]) + ? (baseline[entryKey] as Record) + : null; + const transportType = + rawType || + inferTransportType(entry) || + (baselineEntry ? inferTransportType(baselineEntry) : ''); const normalized = transportType === 'http' ? 'streamable-http' : transportType; const required = REQUIRED_BY_TRANSPORT[normalized]; if (!required) continue; diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 033aaa3..171031c 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -175,7 +175,6 @@ function renderRenderer({ yamlBaseKeys, onChange = vi.fn(), onValidationError = vi.fn(), - scopeMode, }: { baseRecord: Record; editedValues?: t.FlatConfigMap; @@ -183,7 +182,6 @@ function renderRenderer({ yamlBaseKeys?: Set; onChange?: (path: string, value: t.ConfigValue) => void; onValidationError?: (message: string) => void; - scopeMode?: boolean; }) { const fields = fieldsForMcp(); const props: t.FieldRendererProps = { @@ -200,7 +198,6 @@ function renderRenderer({ dbOverridePaths, yamlBaseKeys, onValidationError, - scopeMode, }; return { ...render(), @@ -445,17 +442,29 @@ describe('validateMcpCrossField', () => { expect(errors.length).toBe(1); expect(errors[0].missingField).toBe('url'); }); + + it('flags a stdio-by-inference entry whose command is cleared, falling back to the baseline transport when the merged entry loses its discriminator', () => { + /** YAML can omit `type` for stdio (the backend Zod schema defaults it). The renderer's transport inference relies on `command` to identify stdio in that case. When the user clears `command`, the merged entry has no discriminator and inferTransportType(entry) returns ''. Without a baseline fallback the validator skipped the required-field check and let the broken entry save. */ + const baseline = { + foo: { command: 'node', args: ['index.js'] }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo.command', ''], + ]); + expect(errors.length).toBe(1); + expect(errors[0].entryKey).toBe('foo'); + expect(errors[0].missingField).toBe('command'); + }); }); -describe('McpServersRenderer — scope mode', () => { - it('enables create and shows rename/delete affordances in scope mode (tombstone-backed)', () => { +describe('McpServersRenderer — non-YAML entry identity affordances', () => { + it('enables create and shows rename/delete affordances for a non-YAML entry', () => { const baseRecord = { adminOnly: { type: 'sse', url: 'https://admin.example.com' }, }; const { container } = renderRenderer({ baseRecord, yamlBaseKeys: new Set(), - scopeMode: true, }); const createBtn = screen.getByText('com_config_create_mcp_server') @@ -467,7 +476,7 @@ describe('McpServersRenderer — scope mode', () => { expect(container.querySelector('button[aria-label^="com_a11y_rename_entry"]')).not.toBeNull(); }); - it('still allows field-level edits to flow as per-leaf scope overrides', () => { + it('flows field-level edits as per-leaf onChange writes', () => { const onChange = vi.fn(); const baseRecord = { adminOnly: { type: 'sse', url: 'https://admin.example.com' }, @@ -475,7 +484,6 @@ describe('McpServersRenderer — scope mode', () => { const { container } = renderRenderer({ baseRecord, yamlBaseKeys: new Set(), - scopeMode: true, onChange, }); @@ -490,7 +498,7 @@ describe('McpServersRenderer — scope mode', () => { expect(urlCalls.length).toBeGreaterThan(0); }); - it('writes undefined at entry-path on delete in scope mode (the existing DELETE field-path cleanly $unsets a scope-tier entry)', () => { + it('writes undefined at the entry path on delete (so the DELETE field-path can $unset the entry cleanly)', () => { const onChange = vi.fn(); const baseRecord = { adminOnly: { type: 'sse', url: 'https://admin.example.com' }, @@ -498,7 +506,6 @@ describe('McpServersRenderer — scope mode', () => { const { container } = renderRenderer({ baseRecord, yamlBaseKeys: new Set(), - scopeMode: true, onChange, }); @@ -511,7 +518,7 @@ describe('McpServersRenderer — scope mode', () => { expect(entryWrite![1]).toBeUndefined(); }); - it('writes undefined at old entry-path on rename in scope mode', () => { + it('writes undefined at the old entry path on rename and per-leaf writes for the new key', () => { const onChange = vi.fn(); const baseRecord = { adminOnly: { type: 'sse', url: 'https://admin.example.com' }, @@ -519,7 +526,6 @@ describe('McpServersRenderer — scope mode', () => { const { container } = renderRenderer({ baseRecord, yamlBaseKeys: new Set(), - scopeMode: true, onChange, }); fireEvent.click(screen.getByText('adminOnly')); diff --git a/src/server/config.ts b/src/server/config.ts index aef5275..9c6d4dd 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -926,7 +926,7 @@ export const saveBaseConfigFn = createServerFn({ method: 'POST' }) entries: z .array(z.object({ fieldPath: safeFieldPath, value: z.unknown() })) .min(1) - .max(100), + .max(500), }), ) .handler(async ({ data }) => { diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index 0d72e7b..21e4e26 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -87,8 +87,6 @@ export interface ConfigTabContentProps { /** YAML-defined entry keys per section, keyed by parent path. */ baseRecordKeys?: Record>; onValidationError?: (message: string) => void; - /** True when editing a role/group scope rather than the base config; renderers may suppress structural actions (rename/delete of inherited entries) that can't be expressed without a scope-level tombstone. */ - scopeMode?: boolean; } export interface ConfigTableOfContentsProps { @@ -233,8 +231,6 @@ export interface FieldRendererProps { /** YAML-defined entry keys for the section being rendered. */ yamlBaseKeys?: Set; onValidationError?: (message: string) => void; - /** True when editing a role/group scope. Renderers that manage record entries should suppress rename/delete affordances because per-leaf saves alone can't tombstone an inherited entry. */ - scopeMode?: boolean; } export interface ImportYamlDialogProps { From d0e3c575a42d654af0152eaed43483b861c08c0f Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:36:51 -0700 Subject: [PATCH 23/28] =?UTF-8?q?=E2=9A=A1=20perf:=20Stabilize=20MCP=20Row?= =?UTF-8?q?=20Memo=20And=20Base=20Record=20Reference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit McpServersRenderer now passes a per-row stable boolean \`justAdded={key === justAddedKey}\` to McpEntryRow instead of the raw \`justAddedKey\` string. Rows that were not just added receive \`false\` on every render; the shallow memo bails and only the newly-created row re-renders. With the old shape, creating a server re-rendered every existing row because the shared string prop changed for all of them. \`baseRecord\` now falls back to a module-level frozen \`EMPTY_RECORD\` sentinel instead of a fresh \`{}\` literal whenever \`parentValue\` is missing or \`getValue\` returns a non-object. The \`editsByEntry\` and \`record\` useMemos are keyed off \`baseRecord\`, so the previous fresh-literal pattern forced the whole memo chain to recompute on every parent re-render even when no MCP data had actually changed. Both fixes are perf-only and behavior-neutral; existing tests cover the surrounding correctness. --- .../sections/McpServersRenderer.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index ca6f290..6d08221 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -41,6 +41,12 @@ const TRANSPORT_TYPE_OPTIONS: { label: string; value: string }[] = [ const ALWAYS_REQUIRED = new Set(['type']); +/** Stable empty record used as the fallback for `baseRecord`/`parentValue` when no data is available, so the downstream `useMemo` chain on `editsByEntry`/`record` does not re-fire on every render with a fresh `{}` literal. */ +const EMPTY_RECORD: Record = Object.freeze({}) as Record< + string, + t.ConfigValue +>; + /** * YAML configs can omit `type` because each transport schema (except * streamable-http) defaults from the discriminating fields. Mirror the @@ -664,8 +670,8 @@ export function McpServersRenderer(props: t.FieldRendererProps) { const path = parentPath; const entryPrefix = `${path}.`; - const baseValue = getValue(path, parentValue ?? {}); - const baseRecord: Record = isPlainObject(baseValue) ? baseValue : {}; + const baseValue = getValue(path, parentValue ?? EMPTY_RECORD); + const baseRecord: Record = isPlainObject(baseValue) ? baseValue : EMPTY_RECORD; const editsByEntry = useMemo(() => { const map = new Map>(); @@ -930,7 +936,7 @@ export function McpServersRenderer(props: t.FieldRendererProps) { onChange={onChange} onRemove={handleRemove} onRename={handleRename} - justAddedKey={justAddedKey} + justAdded={key === justAddedKey} /> ))} {entries.length === 0 && ( @@ -968,7 +974,7 @@ const McpEntryRow = memo(function McpEntryRowImpl({ onChange, onRemove, onRename, - justAddedKey, + justAdded, }: { entryKey: string; entryValue: t.ConfigValue; @@ -979,7 +985,7 @@ const McpEntryRow = memo(function McpEntryRowImpl({ onChange: (path: string, value: t.ConfigValue) => void; onRemove: (key: string) => void; onRename: (oldKey: string, newKey: string) => void; - justAddedKey: string | null; + justAdded: boolean; }) { const entryObj = isPlainObject(entryValue) ? entryValue : {}; const rawType = typeof entryObj.type === 'string' ? entryObj.type : ''; @@ -1037,7 +1043,7 @@ const McpEntryRow = memo(function McpEntryRowImpl({ onRemove={isReadOnly || isLockedIdentity ? undefined : () => onRemove(entryKey)} onRename={isReadOnly || isLockedIdentity ? undefined : (renamed) => onRename(entryKey, renamed)} disabled={isReadOnly} - defaultExpanded={entryKey === justAddedKey} + defaultExpanded={justAdded} renderFields={renderEntryFields} /> ); From 97e2d226819126a4287583c876420cf8b7d7f68c Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:47:37 -0700 Subject: [PATCH 24/28] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Trust=20baseOnly=20?= =?UTF-8?q?Response=20Instead=20Of=20Byte-Comparing=20Against=20Merged=20C?= =?UTF-8?q?onfig?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy-backend fallback in getBaseConfigFn discarded the baseOnly response whenever an admin override on mcpServers happened to be byte-identical to the merged config. That false negative dropped yamlMcpKeys for entries that should stay locked, re-exposing rename/delete affordances on YAML-defined MCP servers in exactly the no-op-override scenario (admin set title to a value that already matched the YAML value, etc.). The byte-equality guard was a defensive heuristic against a LibreChat that ignored ?baseOnly entirely. The deployed backend supports it directly so the heuristic is no longer earning the risk it introduces. Drop the guard and trust the baseOnly response when it parses to a valid mcpServers object. The admin panel still falls back to "lock nothing" when baseOnly errors or returns a malformed shape (the early-return at the isPlainObject check), which is the right behavior on actual backend failure. --- src/server/config.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/server/config.ts b/src/server/config.ts index 9c6d4dd..21dc69d 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -848,17 +848,8 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( const baseOnly = normalizeAppServiceKeys(baseOnlyRaw); const mcp = baseOnly.mcpServers; if (mcp && typeof mcp === 'object' && !Array.isArray(mcp)) { - /** A LibreChat that predates the companion baseOnly support returns the same merged payload for both endpoints, so the response itself is not a reliable signal. When admin overrides on mcpServers exist (so we _expect_ baseOnly to be a strict subset of regular) but the two responses are byte-identical, treat the backend as legacy and fall back to "lock nothing" rather than locking admin-only servers. */ - const mcpOverrides = - dbOverrides && typeof dbOverrides.mcpServers === 'object' && dbOverrides.mcpServers !== null - ? (dbOverrides.mcpServers as Record) - : null; - const hasMcpOverrides = !!mcpOverrides && Object.keys(mcpOverrides).length > 0; - const baseOnlyIsAuthoritative = - !hasMcpOverrides || JSON.stringify(mcp) !== JSON.stringify(config.mcpServers); - if (baseOnlyIsAuthoritative) { - yamlMcpKeys = Object.keys(mcp as Record); - } + /** Trust the baseOnly response when it has a valid mcpServers shape. The previous byte-equality fallback against `config.mcpServers` was a defensive heuristic for hypothetical legacy backends that ignore `?baseOnly`, but it false-negatived whenever an admin override happened to be a no-op (e.g. an admin set `title` to a value that already matched YAML), causing the YAML lock affordances to disappear for entries that should stay locked. The deployed LibreChat supports `?baseOnly` directly, so the heuristic is no longer earning its keep. */ + yamlMcpKeys = Object.keys(mcp as Record); } } From f22f2e704d0efe07cdf579f4d0f7582480775f93 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:06:02 -0700 Subject: [PATCH 25/28] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Accept=20Empty=20Ar?= =?UTF-8?q?ray=20For=20Required=20MCP=20Array=20Fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stdio servers can legitimately ship with command and an empty args array (a self-contained shell wrapper, an executable that needs no arguments). The Zod schema models this as args: z.array(z.string()) which requires the field be present but accepts []. Three call sites were rejecting empty arrays as "missing" anyway: - The create dialog stripped empty arrays out of the draft before validating, so args: [] was lost and the required-field check then complained args was undefined. - The dialog's required-field check itself flagged Array.isArray && length === 0 as missing. - validateMcpCrossField applied the same Array.isArray && length === 0 missing predicate before save. Drop the empty-array clause from all three. The field is still required (presence-wise) so an entry that omits args entirely still errors, matching the Zod schema. The change just lines our validators up with what Zod actually accepts. Regression test exercises an empty-args edit on a stdio entry. --- .../sections/McpServersRenderer.tsx | 16 ++++------------ .../__tests__/McpServersRenderer.test.tsx | 10 ++++++++++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index 6d08221..c8a1c7d 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -576,10 +576,9 @@ function CreateMcpServerDialog({ const entry: Record = {}; for (const [key, val] of Object.entries(draft)) { if (val === '' || val === undefined || val === null) continue; - if (Array.isArray(val) && val.length === 0) continue; entry[key] = val; } - /** Per-leaf saves bypass whole-object Zod validation, so a partial create (e.g. transport `sse` with no url) would persist as an invalid server. Validate transport-specific required fields here while we still hold the dialog draft. */ + /** Per-leaf saves bypass whole-object Zod validation, so a partial create (e.g. transport `sse` with no url) would persist as an invalid server. Validate transport-specific required fields here while we still hold the dialog draft. An empty array is a valid Zod value for required array fields like stdio `args` (the schema requires presence but accepts `[]`), so do not flag it as missing. */ const rawType = typeof entry.type === 'string' ? entry.type : ''; const transportType = rawType || inferTransportType(entry); const normalizedTransport = transportType === 'http' ? 'streamable-http' : transportType; @@ -587,11 +586,7 @@ function CreateMcpServerDialog({ if (required) { for (const field of required) { const val = entry[field]; - const missing = - val === undefined || - val === null || - val === '' || - (Array.isArray(val) && val.length === 0); + const missing = val === undefined || val === null || val === ''; if (missing) { setError(localize('com_config_server_missing_required', { field })); return; @@ -1162,11 +1157,8 @@ export function validateMcpCrossField( if (!required) continue; for (const field of required) { const v = entry[field]; - const missing = - v === undefined || - v === null || - v === '' || - (Array.isArray(v) && v.length === 0); + /** Empty array is a valid Zod value for required array fields like stdio `args` (the schema requires presence, not non-empty). Don't flag it as missing. */ + const missing = v === undefined || v === null || v === ''; if (missing) { errors.push({ entryKey, missingField: field }); break; diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 171031c..2e1e1aa 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -443,6 +443,16 @@ describe('validateMcpCrossField', () => { expect(errors[0].missingField).toBe('url'); }); + it('accepts an empty array for stdio args (the Zod schema requires presence but allows []) without flagging it as missing', () => { + const baseline = { + foo: { type: 'stdio', command: 'node', args: ['index.js'] }, + }; + const errors = validateMcpCrossField(baseline, [ + ['mcpServers.foo.args', []], + ]); + expect(errors).toEqual([]); + }); + it('flags a stdio-by-inference entry whose command is cleared, falling back to the baseline transport when the merged entry loses its discriminator', () => { /** YAML can omit `type` for stdio (the backend Zod schema defaults it). The renderer's transport inference relies on `command` to identify stdio in that case. When the user clears `command`, the merged entry has no discriminator and inferTransportType(entry) returns ''. Without a baseline fallback the validator skipped the required-field check and let the broken entry save. */ const baseline = { From 275a6c5ab5718e83f1c754c6819a89539e1bf929 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:13:32 -0700 Subject: [PATCH 26/28] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Preserve=20Empty=20?= =?UTF-8?q?Arrays=20In=20handleCreate=20Per-Leaf=20Writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit relaxed the validators to accept empty arrays as valid for required MCP array fields like stdio args, but missed the symmetric trim in handleCreate's per-leaf write loop. A user creating a stdio server with command and empty args passed dialog validation, then handleCreate skipped the args entry as "empty", so the per-leaf save never wrote mcpServers..args. The cross-field check at save time then saw the merged entry without args and reported it as missing, blocking the otherwise-valid create. Drop the empty-array skip both at the top level and inside the nested-object branch so an empty array reaches the per-leaf write path. The other degenerate values (undefined, null, '') stay filtered because those are invalid Zod values. The validateMcpCrossField regression test from the previous commit already covers the validator side; this commit completes the write side so the two halves agree. --- src/components/configuration/sections/McpServersRenderer.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx index c8a1c7d..0aa9630 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -794,14 +794,13 @@ export function McpServersRenderer(props: t.FieldRendererProps) { onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_invalid')); return; } + /** Empty arrays are valid Zod values for required array fields (e.g. stdio `args: []`); skipping them here would drop the per-leaf write and leave the entry incomplete, failing the cross-field check at save time. Only undefined / null / empty string get filtered, matching what the dialog draft-trim already does. */ for (const [fieldKey, fieldValue] of Object.entries(entry)) { if (fieldValue === undefined || fieldValue === null) continue; if (fieldValue === '') continue; - if (Array.isArray(fieldValue) && fieldValue.length === 0) continue; if (isPlainObject(fieldValue)) { for (const { segments, value } of enumerateLeafPaths(fieldValue, [fieldKey])) { if (value === undefined || value === null || value === '') continue; - if (Array.isArray(value) && value.length === 0) continue; onChange(`${path}.${serverName}.${segments.join('.')}`, value); } } else { From 9c5c9ec4578cf4fd20581299045229f47dad27a1 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:22:17 -0700 Subject: [PATCH 27/28] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Feed=20YAML=20Fallb?= =?UTF-8?q?ack=20Into=20MCP=20Cross-Field=20Check=20For=20Base-Mode=20Rese?= =?UTF-8?q?ts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-field check in handleConfirmSave only fed a resetFallback in scope mode, where a DELETE on a scope-tier override row reveals the inherited base value. In base mode the admin panel writes to the base config override doc layered on top of the YAML config, so a DELETE on a field path reveals the underlying YAML value rather than removing the field entirely. Without a YAML fallback the validator modeled the post-reset field as undefined and blocked the save even though the merged post-save config would inherit a valid YAML value. getBaseConfigFn now also returns the full yamlMcpServers record alongside yamlMcpKeys so the client has the inherited values available. handleConfirmSave's mcpResetFallback now uses configValues for scope mode and baseConfigData.yamlMcpServers for base mode, matching whichever layer the reset is exposing. Regression test covers the base-mode reset-on-YAML case at the validateMcpCrossField boundary. --- src/components/configuration/ConfigPage.tsx | 10 +++++----- .../__tests__/McpServersRenderer.test.tsx | 16 ++++++++++++++++ src/server/config.ts | 13 +++++++++++-- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index f6b1bf2..0233f94 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -521,12 +521,11 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const mcpEdits: Array<[string, t.ConfigValue]> = touched .filter((p) => p.startsWith('mcpServers.')) .map((p) => [p, editedValues[p]] as [string, t.ConfigValue]); - /** In scope mode, a leaf reset (undefined write) only removes the scope override and reveals the inherited base value, so feed configValues as the resetFallback. In base mode there is nothing to inherit from. */ + /** A leaf reset (undefined write) removes the override and reveals the value of the next-lower layer. In scope mode that next layer is the base config; in base mode it is the un-merged YAML config (the baseOnly response). Feed whichever layer applies as the resetFallback so the cross-field validator does not falsely flag a reset-but-still-valid field as missing. */ const mcpResetFallback = (() => { - if (!isEditingScope) return undefined; - const v = configValues?.mcpServers; - if (v && typeof v === 'object' && !Array.isArray(v)) { - return v as Record; + const source = isEditingScope ? configValues?.mcpServers : baseConfigData?.yamlMcpServers; + if (source && typeof source === 'object' && !Array.isArray(source)) { + return source as Record; } return undefined; })(); @@ -612,6 +611,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi saving, baseActiveConfigValues, configValues, + baseConfigData, localize, ]); diff --git a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx index 2e1e1aa..2d250ac 100644 --- a/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -443,6 +443,22 @@ describe('validateMcpCrossField', () => { expect(errors[0].missingField).toBe('url'); }); + it('treats a base-mode leaf reset on a YAML-defined entry as revealing the YAML value when the YAML map is supplied as resetFallback', () => { + /** In base mode the admin panel writes to the base config override doc; a DELETE on a field path reveals the underlying YAML value. With the YAML map fed in as resetFallback the validator should see the inherited value and not flag the field as missing. */ + const baseline = { + kapa: { type: 'stdio', command: 'node-overridden', args: ['index.js'] }, + }; + const yamlFallback = { + kapa: { type: 'stdio', command: 'node', args: ['index.js'] }, + }; + const errors = validateMcpCrossField( + baseline, + [['mcpServers.kapa.command', undefined]], + yamlFallback, + ); + expect(errors).toEqual([]); + }); + it('accepts an empty array for stdio args (the Zod schema requires presence but allows []) without flagging it as missing', () => { const baseline = { foo: { type: 'stdio', command: 'node', args: ['index.js'] }, diff --git a/src/server/config.ts b/src/server/config.ts index 21dc69d..274ce12 100644 --- a/src/server/config.ts +++ b/src/server/config.ts @@ -841,6 +841,7 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( } let yamlMcpKeys: string[] | undefined; + let yamlMcpServers: Record | undefined; if (baseOnlyResponse.ok) { const { config: baseOnlyRaw } = (await baseOnlyResponse.json()) as { config: Record; @@ -849,11 +850,19 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( const mcp = baseOnly.mcpServers; if (mcp && typeof mcp === 'object' && !Array.isArray(mcp)) { /** Trust the baseOnly response when it has a valid mcpServers shape. The previous byte-equality fallback against `config.mcpServers` was a defensive heuristic for hypothetical legacy backends that ignore `?baseOnly`, but it false-negatived whenever an admin override happened to be a no-op (e.g. an admin set `title` to a value that already matched YAML), causing the YAML lock affordances to disappear for entries that should stay locked. The deployed LibreChat supports `?baseOnly` directly, so the heuristic is no longer earning its keep. */ - yamlMcpKeys = Object.keys(mcp as Record); + yamlMcpServers = mcp as Record; + yamlMcpKeys = Object.keys(yamlMcpServers); } } - return { config, dbOverrides, configuredFromBase, schemaDefaults: flatDefaults, yamlMcpKeys }; + return { + config, + dbOverrides, + configuredFromBase, + schemaDefaults: flatDefaults, + yamlMcpKeys, + yamlMcpServers, + }; }); export const baseConfigOptions = queryOptions({ From 99c6fa6af10a7bf07e62412983ee0fa06d9032f1 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:40:56 -0700 Subject: [PATCH 28/28] =?UTF-8?q?=E2=9A=A1=20perf:=20Walk=20Ancestors=20Di?= =?UTF-8?q?rectly=20In=20hasPendingAncestorDelete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleFieldChange used Object.keys(prev).some(...) to look up whether any ancestor of the new path held an undefined value. Rename and remove emit one onChange call per leaf, and each call re-scanned the full edited-values map, compounding to O(n*m) work per multi-leaf event. For a server with a large env or headers map this produced noticeable jank. Walk the dotted path components upward instead. The set of candidate ancestors is bounded by the path depth (4-6 levels in practice for MCP), independent of the pending-edit count. Each ancestor key lookup is O(1) via in/prev[ancestor]. Pure perf change; the boolean output is unchanged for every input. --- src/components/configuration/ConfigPage.tsx | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 0233f94..546729f 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -403,11 +403,16 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi const isContainerDelete = value === undefined && (baselineIntermediates.has(path) || baselineContainerPaths.has(path)); - /** When the user deleted a container entry and is now writing descendants under it (delete-then-recreate of an MCP server), the new leaf must persist even if it matches baseline so the post-DELETE recreate is not missing required fields, and the ancestor-undefined must outlive the descendant write so handleConfirmSave can DELETE the entry before PATCHing the new leaves. */ - const hasPendingAncestorDelete = Object.keys(prev).some( - (existing) => - existing !== path && path.startsWith(`${existing}.`) && prev[existing] === undefined, - ); + /** When the user deleted a container entry and is now writing descendants under it (delete-then-recreate of an MCP server), the new leaf must persist even if it matches baseline so the post-DELETE recreate is not missing required fields, and the ancestor-undefined must outlive the descendant write so handleConfirmSave can DELETE the entry before PATCHing the new leaves. Walk ancestors directly instead of scanning every pending edit; rename/remove emit one onChange per leaf and the prior O(n)-per-call scan compounded to O(n*m) work per event. */ + const hasPendingAncestorDelete = (() => { + let lastDot = path.lastIndexOf('.'); + while (lastDot > 0) { + const ancestor = path.slice(0, lastDot); + if (ancestor in prev && prev[ancestor] === undefined) return true; + lastDot = ancestor.lastIndexOf('.'); + } + return false; + })(); if (match && !isContainerDelete && !hasPendingAncestorDelete) { const next = { ...prev }; delete next[path];