diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 085b4a0..546729f 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'; @@ -130,6 +131,15 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return new Set(Object.keys(flattenObject(dbOverrides))); }, [dbOverrides]); + 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 +354,93 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi return scopeResolvedValues ?? {}; }, [isEditingScope, flatBaseline, scopeResolvedValues]); + /** 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)) { + const parts = leaf.split('.'); + for (let i = 1; i < parts.length; i++) { + set.add(parts.slice(0, i).join('.')); + } + } + 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) => { - 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)); + /** 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)); + /** 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]; 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]]; + /** 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; + const newIsDescendant = path.startsWith(`${existing}.`); + const newIsAncestor = existing.startsWith(`${path}.`); + if (newIsDescendant && next[existing] === undefined) continue; + if (newIsDescendant || newIsAncestor) { + delete next[existing]; } - const indexMatch = /^(.+)\.\d+$/.exec(path); - if (indexMatch) delete next[indexMatch[1]]; - return next; - }); + } + return next; }); }, - [scopeBaseline], + [scopeBaseline, baselineIntermediates, baselineContainerPaths], ); const isDirty = Object.keys(editedValues).length > 0; @@ -453,11 +515,46 @@ 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. 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 = baseActiveConfigValues?.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]); + /** 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 = (() => { + const source = isEditingScope ? configValues?.mcpServers : baseConfigData?.yamlMcpServers; + if (source && typeof source === 'object' && !Array.isArray(source)) { + return source as Record; + } + return undefined; + })(); + if (mcpEdits.length > 0) { + const mcpErrors = validateMcpCrossField(mcpBaseline, mcpEdits, mcpResetFallback); + 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) => ({ 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); @@ -466,42 +563,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 { @@ -522,6 +614,10 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi invalidateAndResetBase, invalidateAndResetScope, saving, + baseActiveConfigValues, + configValues, + baseConfigData, + localize, ]); const serializedEditedValues = useMemo(() => { @@ -540,7 +636,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 +926,8 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi sectionPermissions={sectionPermissions} 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 7ce6520..20a7e01 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -54,6 +54,8 @@ export function ConfigTabContent({ sectionPermissions, schemaDefaults, showConfiguredOnly, + baseRecordKeys, + onValidationError, }: t.ConfigTabContentProps) { const localize = useLocalize(); const fieldsDisabled = readOnly; @@ -164,6 +166,7 @@ export function ConfigTabContent({ getValue, onChange: onFieldChange, onResetField, + editedValues, disabled: sectionDisabled, profileMap, previewMode, @@ -179,6 +182,8 @@ export function ConfigTabContent({ pendingResets, schemaDefaults, showConfiguredOnly, + yamlBaseKeys: baseRecordKeys?.[dataKey], + onValidationError, }; return ( <> @@ -232,7 +237,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..0aa9630 100644 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ b/src/components/configuration/sections/McpServersRenderer.tsx @@ -1,17 +1,8 @@ -/** - * 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 - */ - 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'; @@ -21,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'], @@ -34,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']), @@ -51,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' }, @@ -59,17 +39,18 @@ const TRANSPORT_TYPE_OPTIONS: { label: string; value: string }[] = [ { label: 'websocket', value: 'websocket' }, ]; -/** The `type` field is always required. */ 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 +>; + /** - * 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; @@ -79,7 +60,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'; } @@ -97,15 +78,85 @@ function withFieldOverrides(field: t.SchemaField, transportType: string): t.Sche return field; } -// --------------------------------------------------------------------------- -// Semantic field groups -// --------------------------------------------------------------------------- +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]; + 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; +} + +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 { + return JSON.parse(JSON.stringify(value)) as Record; +} + +function isPlainObject(value: t.ConfigValue): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** New entry names are blocked from containing dots, but legacy data may still hold dotted server names; longest-prefix match against known keys keeps those edits attributed to the real entry instead of carving off the first segment. */ +function resolveEntryKey( + rest: string, + knownKeys: Iterable, +): { 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[] = [], + 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; +} interface FieldGroupDef { labelKey: string; fields: string[]; defaultExpanded: boolean; - /** Nested sub-groups rendered inside this group (fields should be empty when using children). */ children?: FieldGroupDef[]; } @@ -149,10 +200,6 @@ const MCP_FIELD_GROUPS: FieldGroupDef[] = [ }, ]; -// --------------------------------------------------------------------------- -// FieldGroup — collapsible group within a card (replicates EndpointsRenderer) -// --------------------------------------------------------------------------- - function flattenGroupFields( fields: t.SchemaField[], parentValue: t.ConfigValue, @@ -162,17 +209,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) { - // 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. + const fieldDisabled = disabled || (lockedKeys?.has(field.key) ?? false); if (field.key === 'type') { const fieldId = `${parentPath}-${field.key}`; const label = localize(`com_config_field_${field.key}`); @@ -194,7 +237,7 @@ function flattenGroupFields( value={displayValue} options={TRANSPORT_TYPE_OPTIONS} onChange={(v) => onChange(field.key, v)} - disabled={disabled} + disabled={fieldDisabled} aria-label={label} /> @@ -205,10 +248,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 +259,7 @@ function flattenGroupFields( onChange(field.key, { ...nestedObj, [childKey]: childValue }); }, localize, - disabled, + fieldDisabled, collectionRenderOverrides, true, ), @@ -233,7 +273,7 @@ function flattenGroupFields( parentPath, onChange, localize, - disabled, + fieldDisabled, collectionRenderOverrides, true, ), @@ -243,7 +283,6 @@ function flattenGroupFields( return nodes; } -/** Parent-level collapsible section that wraps child sub-groups. */ function FieldGroupSection({ labelKey, defaultExpanded, @@ -298,6 +337,7 @@ function FieldGroup({ disabled, defaultExpanded, transportType, + lockedKeys, }: { labelKey: string; fields: t.SchemaField[]; @@ -307,6 +347,7 @@ function FieldGroup({ disabled?: boolean; defaultExpanded: boolean; transportType: string; + lockedKeys?: Set; }) { const localize = useLocalize(); const { isExpanded, hasEverExpanded, sectionRef, toggle } = useCollapsibleSection({ @@ -349,6 +390,8 @@ function FieldGroup({ localize, transportType, disabled, + undefined, + lockedKeys, )} , )} @@ -356,42 +399,35 @@ function FieldGroup({ ); } -// --------------------------------------------------------------------------- -// McpEntryFields — dynamic visibility based on transport type -// --------------------------------------------------------------------------- - function McpEntryFields({ fields, parentValue, 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); - // 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(); 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 +437,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 +452,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 +467,12 @@ function McpEntryFields({ groupFields, parentValue, parentPath, - handleChange, + onChange, localize, currentType, disabled, + undefined, + lockedKeys, )} )} @@ -456,10 +483,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 +501,11 @@ function McpEntryFields({ fields={groupFields} parentValue={parentValue} parentPath={parentPath} - onChange={handleChange} + onChange={onChange} disabled={disabled} defaultExpanded={group.defaultExpanded} transportType={currentType} + lockedKeys={lockedKeys} /> ); }; @@ -490,20 +519,17 @@ function McpEntryFields({ fields={ungrouped} parentValue={parentValue} parentPath={parentPath} - onChange={handleChange} + onChange={onChange} disabled={disabled} defaultExpanded={false} transportType={currentType} + lockedKeys={lockedKeys} /> )} ); } -// --------------------------------------------------------------------------- -// CreateMcpServerDialog -// --------------------------------------------------------------------------- - function CreateMcpServerDialog({ open, onClose, @@ -535,6 +561,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; @@ -542,9 +576,23 @@ 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. 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; + const required = REQUIRED_BY_TRANSPORT[normalizedTransport]; + if (required) { + for (const field of required) { + const val = entry[field]; + const missing = val === undefined || val === null || val === ''; + if (missing) { + setError(localize('com_config_server_missing_required', { field })); + return; + } + } + } onSave(name, entry); setServerName(''); setDraft({}); @@ -598,76 +646,263 @@ function CreateMcpServerDialog({ ); } -// --------------------------------------------------------------------------- -// McpServersRenderer — main export -// --------------------------------------------------------------------------- export function McpServersRenderer(props: t.FieldRendererProps) { - const { fields, parentPath, parentValue, getValue, onChange, disabled } = props; + const { + fields, + parentPath, + parentValue, + getValue, + onChange, + disabled, + editedValues, + yamlBaseKeys, + onValidationError, + } = 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 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 ?? EMPTY_RECORD); + const baseRecord: Record = isPlainObject(baseValue) ? baseValue : EMPTY_RECORD; + + 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 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, baseRecord]); + + 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. 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 => { + 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; + } + } + 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, + subsequentLeaves.map((e) => [e.segments, e.value] as [string[], t.ConfigValue]), + ); + }; + + /** 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]); + + const entries = useMemo(() => Object.entries(record), [record]); + + const existingKeys = useMemo(() => new Set(Object.keys(record)), [record]); + + /** + * 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 keep the callbacks referentially stable so memo(McpEntryRow) can bail. */ + 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]); + + /** 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) => { - const next: Record = { [serverName]: entry }; - for (const [k, v] of entries) { - next[k] = v; + if (serverName.includes('.')) { + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_no_dots')); + return; + } + if ( + serverName === '__proto__' || + serverName === 'constructor' || + serverName === 'prototype' + ) { + 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 (isPlainObject(fieldValue)) { + for (const { segments, value } of enumerateLeafPaths(fieldValue, [fieldKey])) { + if (value === undefined || value === null || value === '') continue; + onChange(`${path}.${serverName}.${segments.join('.')}`, value); + } + } else { + onChange(`${path}.${serverName}.${fieldKey}`, fieldValue); + } } - onChange(path, next); setJustAddedKey(serverName); }, - [entries, onChange, path], + [onChange, path], ); const handleRemove = useCallback( (key: string) => { - const next = { ...record }; - delete next[key]; - onChange(path, next); + /** 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}.`; + 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); + } + } + /** 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); + } }, - [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) { + return; } - onChange(path, next); - }, - [record, onChange, path], - ); + /** 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; + if (newKey.includes('.')) { + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_no_dots')); + return; + } + if (newKey === '__proto__' || newKey === 'constructor' || newKey === 'prototype') { + onValidationErrorRef.current?.(localizeRef.current('com_config_server_name_invalid')); + return; + } + if (Object.hasOwn(record, newKey)) { + onValidationErrorRef.current?.(localizeRef.current('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]; + + /** 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) : []; + + 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('.'); + if (overlayBySeg.has(segKey)) { + onChange(`${newPrefix}${segments.join('.')}`, overlayBySeg.get(segKey)); + } + onChange(`${oldPrefix}${segments.join('.')}`, undefined); + } + /** See $unset note in handleRemove. */ + onChange(`${path}.${oldKey}`, undefined); }, - [record, onChange, path], + [onChange, path], ); return ( @@ -683,37 +918,21 @@ 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} - /> - ); - })} + {entries.map(([key, entryValue]) => ( + + ))} {entries.length === 0 && (

{localize('com_config_no_entries')} @@ -725,8 +944,227 @@ export function McpServersRenderer(props: t.FieldRendererProps) { onSave={handleCreate} fields={fields} existingKeys={existingKeys} - renderFields={renderGroupedMcpFields} + renderFields={(entryFields, ev, ep, eoc) => ( + + )} /> ); } + +const McpEntryRow = memo(function McpEntryRowImpl({ + entryKey, + entryValue, + fields, + path, + disabled, + isYamlSource, + onChange, + onRemove, + onRename, + justAdded, +}: { + 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; + justAdded: boolean; +}) { + 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}`; + /** 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; + /** 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; + 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, isDottedLegacy], + ); + + const renderEntryFields: t.CollectionRenderFields = useCallback( + (entryFields, ev, ep) => ( + + ), + [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, isDottedLegacy], + ); + + return ( + onRemove(entryKey)} + onRename={isReadOnly || isLockedIdentity ? undefined : (renamed) => onRename(entryKey, renamed)} + disabled={isReadOnly} + defaultExpanded={justAdded} + renderFields={renderEntryFields} + /> + ); +}); + +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). */ +function mergeMcpEdits( + baseline: Record, + edits: Array<[string, t.ConfigValue]>, + resetFallback?: Record, +): 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) { + if (!path.startsWith('mcpServers.')) continue; + const parsed = resolveEntryKey(path.slice('mcpServers.'.length), knownKeys); + if (!parsed) continue; + const { entryKey, segments } = parsed; + knownKeys.add(entryKey); + if (segments.length === 0) { + if (value === undefined) { + 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); + } + continue; + } + if (deletedEntries.has(entryKey) && value !== undefined) { + deletedEntries.delete(entryKey); + } + if (!upsertEntries[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) : {}; + } + 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 = {}; + 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. `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, resetFallback); + const knownKeys = new Set(Object.keys(baseline)); + const affected = new Set(); + for (const [path] of edits) { + 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) { + const entry = merged[entryKey]; + if (!isPlainObject(entry)) continue; + const rawType = typeof entry.type === 'string' ? entry.type : ''; + /** 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; + for (const field of required) { + const v = entry[field]; + /** 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; + } + } + } + return errors; +} + +export { YAML_LOCKED_FIELDS, INSPECTOR_DERIVED, 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..2d250ac --- /dev/null +++ b/src/components/configuration/sections/__tests__/McpServersRenderer.test.tsx @@ -0,0 +1,1042 @@ +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, + validateMcpCrossField, +} 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(), + onValidationError = vi.fn(), +}: { + baseRecord: Record; + 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 = { + 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, + onValidationError, + }; + return { + ...render(), + onChange, + onValidationError, + 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('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([]); + }); + + 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'); + }); + + 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'] }, + }; + 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 = { + 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 — 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(), + }); + + const createBtn = screen.getByText('com_config_create_mcp_server') + .closest('button') as HTMLButtonElement; + expect(createBtn.hasAttribute('disabled')).toBe(false); + + 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('flows field-level edits as per-leaf onChange writes', () => { + const onChange = vi.fn(); + const baseRecord = { + adminOnly: { type: 'sse', url: 'https://admin.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + 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); + }); + + 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' }, + }; + 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(); + }); + + 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' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + fireEvent.click(screen.getByText('adminOnly')); + 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.adminOnly'); + expect(oldEntryWrite).toBeDefined(); + expect(oldEntryWrite![1]).toBeUndefined(); + const newLeafWrite = onChange.mock.calls.find( + ([p, v]) => p === 'mcpServers.renamed.url' && v === 'https://admin.example.com', + ); + expect(newLeafWrite).toBeDefined(); + }); +}); + +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 = { + 'legacy.dotted': { type: 'sse', url: 'https://new.example.com' }, + }; + const { container } = renderRenderer({ + baseRecord, + 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('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' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + }); + + 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); + + 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(dottedWrites).toEqual([]); + }); +}); + +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 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' }, + }; + const { container } = renderRenderer({ + baseRecord, + yamlBaseKeys: new Set(), + onChange, + onValidationError, + }); + + 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(onValidationError).toHaveBeenCalledWith('com_config_server_name_exists'); + }); +}); + +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'); + }); + + 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', () => { + 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..a949ecc --- /dev/null +++ b/src/components/configuration/sections/mcpFieldMeta.ts @@ -0,0 +1,13 @@ +/** Changing `type` breaks the MCPOptionsSchema Zod union and produces inspectionFailed stubs that cannot connect. */ +export const YAML_LOCKED_FIELDS = new Set(['type']); + +/** Inspector-populated fields; admin overrides for these are silently ignored. */ +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..0e7ffec 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -75,6 +75,10 @@ "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_server_missing_required": "Missing required field: {{field}}", + "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", diff --git a/src/server/config.test.ts b/src/server/config.test.ts index fddc4bf..60426d0 100644 --- a/src/server/config.test.ts +++ b/src/server/config.test.ts @@ -621,6 +621,92 @@ 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(';'); + 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); + } + }); + + 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); + }); }); /* --------------------------------------------------------------------------- @@ -857,9 +943,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..274ce12 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,52 @@ export function getZodTypeName( return typeName || 'unknown'; } +/** 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; + 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); + } + /** 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) => { + 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: [] }] }, + }; + }; + /** _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. * Returns the schema **with wrappers intact** so `.safeParse()` runs the * full validation chain (refine, transform, pipe). Returns `null` if the @@ -484,16 +529,18 @@ 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; - } + /** 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) { + current = candidates[0]; + } else { + current = anyOfSchema(candidates); } - if (!found) return null; - current = found; } else if (typeName === 'ZodIntersection') { const resolved = resolveIntersection(unwrapped); if (!resolved?.shape?.[segment]) return null; @@ -761,8 +808,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 +840,29 @@ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler(async ( dbOverrides = dbConfig.overrides as Record; } - return { config, dbOverrides, configuredFromBase, schemaDefaults: flatDefaults }; + let yamlMcpKeys: string[] | undefined; + let yamlMcpServers: Record | 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)) { + /** 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. */ + yamlMcpServers = mcp as Record; + yamlMcpKeys = Object.keys(yamlMcpServers); + } + } + + return { + config, + dbOverrides, + configuredFromBase, + schemaDefaults: flatDefaults, + yamlMcpKeys, + yamlMcpServers, + }; }); export const baseConfigOptions = queryOptions({ @@ -833,7 +903,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] : []; @@ -853,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 b13e873..21e4e26 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -84,6 +84,9 @@ export interface ConfigTabContentProps { sectionPermissions?: Record; schemaDefaults?: FlatConfigMap; showConfiguredOnly?: boolean; + /** YAML-defined entry keys per section, keyed by parent path. */ + baseRecordKeys?: Record>; + onValidationError?: (message: string) => void; } export interface ConfigTableOfContentsProps { @@ -206,6 +209,7 @@ export interface FieldRendererProps { getValue: (path: string, fallback: ConfigValue) => ConfigValue; onChange: (path: string, value: ConfigValue) => void; onResetField?: (path: string) => void; + editedValues?: FlatConfigMap; disabled?: boolean; profileMap?: Record; previewMode?: boolean; @@ -224,6 +228,9 @@ 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; + /** YAML-defined entry keys for the section being rendered. */ + yamlBaseKeys?: Set; + onValidationError?: (message: string) => void; } export interface ImportYamlDialogProps {