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
8 changes: 8 additions & 0 deletions frontend/src/components/settings/SettingField.vue
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,14 @@

<!-- textarea (multi-line text, e.g. MCP server instructions) -->
<div v-else-if="field.control === 'textarea'" class="flex flex-col items-stretch w-full sm:w-96">
<div v-if="field.resetDefault != null" class="flex justify-end mb-1">
<button
type="button"
class="btn btn-ghost btn-xs text-base-content/50 hover:text-base-content"
:data-test="`setting-reset-${field.key}`"
@click="emitText(field.resetDefault ?? '')"
>↩ Reset to default</button>
</div>
<textarea
class="textarea textarea-bordered textarea-sm w-full font-mono leading-snug"
:class="{ 'textarea-error': validationError }"
Expand Down
22 changes: 20 additions & 2 deletions frontend/src/components/settings/SettingsSection.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
:key="f.key"
:field="f"
:model-value="getPath(working, f.key)"
:dirty="!!dirty[f.key]"
:dirty="isFieldDirty(f.key)"
@update:model-value="onChange(f, $event)"
/>
<p v-if="!fields.length" class="text-sm text-base-content/50 py-4">No settings match your search.</p>
Expand Down Expand Up @@ -98,7 +98,25 @@ const confirmEl = ref<HTMLDialogElement | null>(null)
const pendingMessages = ref<string[]>([])
const pendingInfoOnly = ref(false)

const dirtyKeys = computed(() => Object.keys(dirty.value))
// A field is dirty if the user changed it through a control (tracked in the
// `dirty` ref by onChange) OR its working value diverges from the last-saved
// original. The latter catches values written onto `state.working` from OUTSIDE
// a control — notably the instructions prefill in Settings.vue (MCP-2484).
// Without it, "Save without editing" after a prefill would PATCH nothing because
// the field was never marked dirty.
const dirtyKeys = computed(() => {
const keys = new Set(Object.keys(dirty.value))
for (const f of props.fields) {
if (!eq(getPath(props.working, f.key), getPath(props.original, f.key))) keys.add(f.key)
}
return [...keys]
})

function isFieldDirty(key: string): boolean {
if (key in dirty.value) return true
const f = props.fields.find((x) => x.key === key)
return f != null && !eq(getPath(props.working, key), getPath(props.original, key))
}

// Block Save only when a CHANGED field is invalid — a pre-existing value the
// user hasn't touched must never block saving unrelated edits.
Expand Down
34 changes: 29 additions & 5 deletions frontend/src/views/Settings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
</template>

<script setup lang="ts">
import { ref, reactive, computed, onMounted, onUnmounted } from 'vue'
import { ref, reactive, computed, watch, onMounted, onUnmounted } from 'vue'
import { RouterLink } from 'vue-router'
import { VueMonacoEditor } from '@guolao/vue-monaco-editor'
import { useServersStore } from '@/stores/servers'
Expand All @@ -192,6 +192,7 @@ import {
SERVER_EDITION_SECTION_TITLE,
DOCS_BASE,
docsUrl,
isBlankInstructions,
type SettingField,
type SettingsAccordion,
} from '@/views/settings/fields'
Expand All @@ -209,21 +210,35 @@ const serverEditionTitle = SERVER_EDITION_SECTION_TITLE
// backend so the `instructions` textarea placeholder never drifts from Go.
const defaultInstructions = ref<string>('')

// Inject the live default as the `instructions` field placeholder. Until the
// fetch resolves (or against an older core that doesn't expose it) the static
// catalogue placeholder ("Loading built-in default…") is used.
// Inject the live default as the `instructions` field placeholder AND as the
// resetDefault value (drives the "Reset to default" button). Until the fetch
// resolves the static catalogue placeholder is used.
const advancedAccordions = computed<SettingsAccordion[]>(() =>
ADVANCED_ACCORDIONS.map((acc) => {
if (!defaultInstructions.value || !acc.fields.some((f) => f.key === 'instructions')) return acc
return {
...acc,
fields: acc.fields.map((f) =>
f.key === 'instructions' ? { ...f, placeholder: defaultInstructions.value } : f
f.key === 'instructions'
? { ...f, placeholder: defaultInstructions.value, resetDefault: defaultInstructions.value }
: f
),
}
})
)

// Prefill the instructions textarea with the built-in default when the config
// field is empty (fresh install or blank override). Only runs when BOTH the
// config AND the default are loaded; whichever resolves second triggers it.
// Never overwrites a user's saved value (non-empty instructions stay as-is).
function maybePrefillInstructions() {
if (!defaultInstructions.value) return
if (!loaded.value) return
if (isBlankInstructions(state.working.instructions)) {
state.working.instructions = defaultInstructions.value
}
}

// ---- form state ----
const loading = ref(false)
const loaded = ref(false)
Expand Down Expand Up @@ -332,6 +347,7 @@ async function loadConfig() {
configJson.value = JSON.stringify(cfg, null, 2)
configStatus.value = { valid: true }
loaded.value = true
maybePrefillInstructions()
} else {
loadError.value = response.error || 'Failed to load configuration'
}
Expand Down Expand Up @@ -426,12 +442,20 @@ async function loadDefaultInstructions() {
const resp = await api.getStatus()
if (resp.success && typeof resp.data?.default_instructions === 'string') {
defaultInstructions.value = resp.data.default_instructions
// In case loadConfig already ran and set loaded=true, prefill now.
maybePrefillInstructions()
}
} catch {
/* leave placeholder as the static fallback */
}
}

// Also trigger prefill if defaultInstructions resolves after config is loaded
// (handles the common case where /api/v1/status is slower than /api/v1/config).
watch(defaultInstructions, () => {
maybePrefillInstructions()
})

onMounted(() => {
loadConfig()
loadDefaultInstructions()
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/views/settings/fields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export interface SettingField {
docs?: string // doc page path on docs.mcpproxy.app, e.g. "/features/docker-isolation"
valueKind?: ValueKind // extra format validation for text/secret fields
optional?: boolean // when true, an empty value is valid (skips kind validation)
resetDefault?: string // when set, render an inline "Reset to default" button that emits this value
}

export interface SettingsAccordion {
Expand Down Expand Up @@ -277,6 +278,12 @@ export const SERVER_EDITION_FIELDS: SettingField[] = [
{ key: 'server_edition.max_user_servers', label: 'Max servers per user', control: 'number', min: 0 },
]

// isBlankInstructions returns true when a saved instructions value is empty /
// whitespace-only / null / undefined — i.e. eligible for prefill from the built-in default.
export function isBlankInstructions(v: string | null | undefined): boolean {
return !v || v.trim() === ''
}

// ---- Section 3: Advanced (subsystem accordions) ----
export const ADVANCED_ACCORDIONS: SettingsAccordion[] = [
{
Expand Down
95 changes: 95 additions & 0 deletions frontend/tests/unit/instructions-prefill.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { describe, it, expect } from 'vitest'
import { isBlankInstructions, type SettingField } from '../../src/views/settings/fields'

// MCP-2484: instructions textarea prefill + reset-to-default

// Unit: SettingField interface accepts resetDefault (compile-time test via import)
describe('SettingField.resetDefault (MCP-2484)', () => {
it('allows a resetDefault property on SettingField without TS error', () => {
const f: SettingField = {
key: 'instructions',
label: 'Server instructions',
control: 'textarea',
optional: true,
resetDefault: 'built-in default text',
}
expect(f.resetDefault).toBe('built-in default text')
})

it('resetDefault is optional — omitting it does not cause a TS error', () => {
const f: SettingField = {
key: 'instructions',
label: 'Server instructions',
control: 'textarea',
optional: true,
}
expect(f.resetDefault).toBeUndefined()
})
})

// Unit: isBlankInstructions helper
describe('isBlankInstructions (MCP-2484)', () => {
it('treats unset/empty/whitespace as blank (eligible for prefill)', () => {
expect(isBlankInstructions(undefined)).toBe(true)
expect(isBlankInstructions(null)).toBe(true)
expect(isBlankInstructions('')).toBe(true)
expect(isBlankInstructions(' \n ')).toBe(true)
})

it('treats a real saved value as non-blank (never overwrite)', () => {
expect(isBlankInstructions('my custom instructions')).toBe(false)
})
})

// Unit: maybePrefillInstructions logic (isolated, not mounting the full component)
describe('maybePrefillInstructions logic (MCP-2484)', () => {
function prefill(working: Record<string, any>, defaultVal: string, isLoaded: boolean) {
if (!defaultVal) return
if (!isLoaded) return
if (isBlankInstructions(working.instructions)) {
working.instructions = defaultVal
}
}

it('prefills instructions when config is empty and default is available', () => {
const w: Record<string, any> = { instructions: '' }
prefill(w, 'built-in default', true)
expect(w.instructions).toBe('built-in default')
})

it('prefills when instructions is null', () => {
const w: Record<string, any> = { instructions: null }
prefill(w, 'built-in default', true)
expect(w.instructions).toBe('built-in default')
})

it('prefills when instructions is undefined', () => {
const w: Record<string, any> = {}
prefill(w, 'built-in default', true)
expect(w.instructions).toBe('built-in default')
})

it('prefills when instructions is whitespace-only', () => {
const w: Record<string, any> = { instructions: ' \n ' }
prefill(w, 'built-in default', true)
expect(w.instructions).toBe('built-in default')
})

it('does NOT overwrite a saved custom instruction', () => {
const w: Record<string, any> = { instructions: 'my custom instructions' }
prefill(w, 'built-in default', true)
expect(w.instructions).toBe('my custom instructions')
})

it('does NOT prefill when config is not yet loaded', () => {
const w: Record<string, any> = { instructions: '' }
prefill(w, 'built-in default', false)
expect(w.instructions).toBe('')
})

it('does NOT prefill when defaultInstructions is empty (API not yet resolved)', () => {
const w: Record<string, any> = { instructions: '' }
prefill(w, '', true)
expect(w.instructions).toBe('')
})
})
75 changes: 75 additions & 0 deletions frontend/tests/unit/settings-section-prefill-dirty.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { mount, flushPromises } from '@vue/test-utils'
import { reactive } from 'vue'

// MCP-2484 (Codex REQUEST_CHANGES on #685): the instructions prefill writes the
// built-in default directly onto `state.working` in Settings.vue. SettingsSection
// PATCHes only its dirty keys, so a value set OUTSIDE a control must still be
// treated as dirty — otherwise "Save without editing" after a prefill persists
// nothing. SettingsSection now derives dirtiness from working≠original.

const patchConfig = vi.fn(() =>
Promise.resolve({ success: true, data: { changed_fields: ['instructions'], requires_restart: false } })
)

vi.mock('@/services/api', () => ({
default: { patchConfig: (...args: unknown[]) => patchConfig(...args) },
}))
vi.mock('@/stores/system', () => ({ useSystemStore: () => ({ addToast: vi.fn() }) }))

import SettingsSection from '@/components/settings/SettingsSection.vue'
import type { SettingField } from '@/views/settings/fields'

const instructionsField: SettingField = {
key: 'instructions',
label: 'Server instructions',
control: 'textarea',
optional: true,
resetDefault: 'BUILT-IN DEFAULT',
}

function mountSection(working: Record<string, unknown>, original: Record<string, unknown>) {
return mount(SettingsSection, {
props: { sectionId: 'mcp', fields: [instructionsField], working: reactive(working), original: reactive(original) },
})
}

describe('SettingsSection prefill-is-dirty (MCP-2484)', () => {
beforeEach(() => patchConfig.mockClear())

it('treats an externally-prefilled value (working≠original) as dirty and saves it', async () => {
// working got the prefilled default written directly; original is still the
// saved (empty) value — exactly the post-prefill state from Settings.vue.
const w = mountSection({ instructions: 'BUILT-IN DEFAULT' }, { instructions: '' })

const save = w.find('[data-test="settings-apply-mcp"]')
expect(save.exists()).toBe(true)
expect((save.element as HTMLButtonElement).disabled).toBe(false)

// the field itself is flagged dirty in the UI
expect(w.find('[data-test="setting-row-instructions"]').classes()).toContain('border-l-warning')

await save.trigger('click')
await flushPromises()

expect(patchConfig).toHaveBeenCalledTimes(1)
expect(patchConfig.mock.calls[0][0]).toMatchObject({ instructions: 'BUILT-IN DEFAULT' })
})

it('is NOT dirty when working equals original (no spurious save)', () => {
const w = mountSection({ instructions: 'same' }, { instructions: 'same' })
expect((w.find('[data-test="settings-apply-mcp"]').element as HTMLButtonElement).disabled).toBe(true)
})

it('clears dirty after a save commits original = working', async () => {
const working = reactive({ instructions: 'BUILT-IN DEFAULT' })
const original = reactive({ instructions: '' })
const w = mount(SettingsSection, {
props: { sectionId: 'mcp', fields: [instructionsField], working, original },
})
await w.find('[data-test="settings-apply-mcp"]').trigger('click')
await flushPromises()
// doSave copies working → original on success; section is no longer dirty
expect((w.find('[data-test="settings-apply-mcp"]').element as HTMLButtonElement).disabled).toBe(true)
})
})
Loading
Loading