Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 11 additions & 51 deletions src/components/configuration/ConfigPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
import {
flattenObject,
unflattenObject,
serializeKVPairs,
deepSerializeKVPairs,
normalizeImportConfig,
hasConfigCapability,
Expand All @@ -31,7 +30,7 @@ import {
} from '@/utils';
import { useLocalize, useHighlightRef, useActiveSection, useCapabilities } from '@/hooks';
import { CONFIG_TABS, OTHER_TAB, SECTION_META, HIDDEN_SECTIONS } from './configMeta';
import { mergeIndexedArrayEdits, partitionScopeResetPaths } from './utils';
import { applyConfigEdit, mergeIndexedArrayEdits, partitionScopeResetPaths } from './utils';
import { validateMcpCrossField } from './sections/McpServersRenderer';
import { ScopeSelector, ScopeTriggerButton } from './ScopeSelector';
import { StickyActionBar } from '@/components/shared';
Expand Down Expand Up @@ -385,51 +384,14 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi
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;
}
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];
}
}
return next;
return applyConfigEdit(
prev,
path,
value,
scopeBaseline,
baselineIntermediates,
baselineContainerPaths,
);
});
},
[scopeBaseline, baselineIntermediates, baselineContainerPaths],
Expand Down Expand Up @@ -574,9 +536,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi
.filter((p) => editedValues[p] !== undefined)
.map((p) => ({
fieldPath: p,
value: /\.\d+$/.test(p)
? deepSerializeKVPairs(editedValues[p])
: serializeKVPairs(editedValues[p]),
value: deepSerializeKVPairs(editedValues[p]),
}));
const resets = touched.filter((p) => editedValues[p] === undefined);
const inheritedMcpKeys = (() => {
Expand Down Expand Up @@ -668,7 +628,7 @@ export function ConfigPage({ initialTab, highlightField, initialScope }: t.Confi
const serializedEditedValues = useMemo(() => {
const result: t.FlatConfigMap = {};
for (const [k, v] of Object.entries(editedValues)) {
result[k] = /\.\d+$/.test(k) ? deepSerializeKVPairs(v) : serializeKVPairs(v);
result[k] = deepSerializeKVPairs(v);
}
return result;
}, [editedValues]);
Expand Down
87 changes: 65 additions & 22 deletions src/components/configuration/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
splitUnionTypes,
partitionScopeResetPaths,
mergeIndexedArrayEdits,
applyConfigEdit,
} from './utils';
import { createField } from '@/test/fixtures';

Expand Down Expand Up @@ -270,10 +271,9 @@ describe('mergeIndexedArrayEdits', () => {
});

it('merges into an existing parent without clobbering its keys', () => {
const merged = mergeIndexedArrayEdits(
{ modelSpecs: { enforce: true, prioritize: false } },
[['modelSpecs.list.0', { name: 'a' }]],
);
const merged = mergeIndexedArrayEdits({ modelSpecs: { enforce: true, prioritize: false } }, [
['modelSpecs.list.0', { name: 'a' }],
]);
expect(merged.modelSpecs).toEqual({
enforce: true,
prioritize: false,
Expand Down Expand Up @@ -325,23 +325,68 @@ describe('mergeIndexedArrayEdits', () => {
});

it('walks deep parent chains, creating each missing level', () => {
const merged = mergeIndexedArrayEdits({}, [
['endpoints.custom.deep.list.0', { name: 'x' }],
]);
const merged = mergeIndexedArrayEdits({}, [['endpoints.custom.deep.list.0', { name: 'x' }]]);
expect(merged).toEqual({
endpoints: { custom: { deep: { list: [{ name: 'x' }] } } },
});
});
});

describe('applyConfigEdit', () => {
it('updates a pending whole-array edit when a newly-added entry is typed into', () => {
const prev = {
'modelSpecs.list': [{}, { name: 'smart-assistant' }],
};
const result = applyConfigEdit(
prev,
'modelSpecs.list.0',
{ name: 'TEST1' },
{},
new Set(),
new Set(),
);
expect(result).toEqual({
'modelSpecs.list': [{ name: 'TEST1' }, { name: 'smart-assistant' }],
});
expect(result).not.toHaveProperty('modelSpecs.list.0');
});

it('keeps per-index edits when no parent array edit is pending', () => {
const result = applyConfigEdit(
{},
'modelSpecs.list.0',
{ name: 'TEST1' },
{},
new Set(),
new Set(),
);
expect(result).toEqual({
'modelSpecs.list.0': { name: 'TEST1' },
});
});

it('drops stale indexed edits when a whole-array edit is queued', () => {
const result = applyConfigEdit(
{ 'modelSpecs.list.0': { name: 'old' } },
'modelSpecs.list',
[{ name: 'new' }],
{},
new Set(),
new Set(),
);
expect(result).toEqual({
'modelSpecs.list': [{ name: 'new' }],
});
});
});

describe('partitionScopeResetPaths', () => {
it('routes whole MCP entry resets to tombstones', () => {
expect(
partitionScopeResetPaths([
'mcpServers.github',
'mcpServers.github.url',
'interface.modelSelect',
], new Set(['github'])),
partitionScopeResetPaths(
['mcpServers.github', 'mcpServers.github.url', 'interface.modelSelect'],
new Set(['github']),
),
).toEqual({
resetPaths: ['mcpServers.github.url', 'interface.modelSelect'],
tombstonePaths: ['mcpServers.github'],
Expand All @@ -350,10 +395,10 @@ describe('partitionScopeResetPaths', () => {

it('routes whole MCP entry resets to unsets when the entry is scope-local', () => {
expect(
partitionScopeResetPaths([
'mcpServers.scopeOnly',
'mcpServers.inherited',
], new Set(['inherited'])),
partitionScopeResetPaths(
['mcpServers.scopeOnly', 'mcpServers.inherited'],
new Set(['inherited']),
),
).toEqual({
resetPaths: ['mcpServers.scopeOnly'],
tombstonePaths: ['mcpServers.inherited'],
Expand All @@ -362,12 +407,10 @@ describe('partitionScopeResetPaths', () => {

it('preserves input order within reset and tombstone groups', () => {
expect(
partitionScopeResetPaths([
'mcpServers.alpha',
'registration.enabled',
'mcpServers.beta',
'endpoints.custom.0',
], new Set(['alpha', 'beta'])),
partitionScopeResetPaths(
['mcpServers.alpha', 'registration.enabled', 'mcpServers.beta', 'endpoints.custom.0'],
new Set(['alpha', 'beta']),
),
).toEqual({
resetPaths: ['registration.enabled', 'endpoints.custom.0'],
tombstonePaths: ['mcpServers.alpha', 'mcpServers.beta'],
Expand Down
68 changes: 68 additions & 0 deletions src/components/configuration/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type * as t from '@/types';

const INDEXED_ARRAY_PATH_RE = /^(.+)\.(\d+)$/;

export function inferKVType(v: t.ConfigValue): t.KVValueType {
if (typeof v === 'boolean') return 'boolean';
if (typeof v === 'number') return 'number';
Expand Down Expand Up @@ -249,6 +251,72 @@ export function partitionScopeResetPaths(
return { resetPaths, tombstonePaths };
}

export function applyConfigEdit(
prev: t.FlatConfigMap,
path: string,
value: t.ConfigValue,
baseline: t.FlatConfigMap,
baselineIntermediates: Set<string>,
baselineContainerPaths: Set<string>,
): t.FlatConfigMap {
const indexMatch = INDEXED_ARRAY_PATH_RE.exec(path);
if (indexMatch) {
const [, arrayPath, indexStr] = indexMatch;
const pendingArray = prev[arrayPath];
if (Array.isArray(pendingArray)) {
const next = { ...prev };
const arr = [...pendingArray];
arr[Number(indexStr)] = value;
next[arrayPath] = arr;
Comment thread
danny-avila marked this conversation as resolved.
for (const existing of Object.keys(next)) {
if (existing.startsWith(`${arrayPath}.`)) delete next[existing];
}
return next;
}
}

const baselineValue = baseline[path];
const match =
value === baselineValue ||
(typeof value === 'object' &&
typeof baselineValue === 'object' &&
JSON.stringify(value) === JSON.stringify(baselineValue));
const isContainerDelete =
value === undefined && (baselineIntermediates.has(path) || baselineContainerPaths.has(path));
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;
}
const next = { ...prev, [path]: value };
if (Array.isArray(value)) {
const prefix = `${path}.`;
for (const k of Object.keys(next)) {
if (k.startsWith(prefix) && INDEXED_ARRAY_PATH_RE.test(k)) delete next[k];
}
}
if (indexMatch) delete next[indexMatch[1]];
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];
}
}
return next;
}

/**
* Merge indexed-array edits (entries whose flat path ends in `.<digit>`) into
* a config tree. Each indexed edit's value replaces the array element at that
Expand Down
Loading