Skip to content

Commit b8f0d86

Browse files
feat(ui): prefill MCP instructions default + Reset-to-default button (MCP-2484) (#685)
* feat(MCP-2484): prefill instructions textarea + Reset to default button - Export isBlankInstructions() from fields.ts for whitespace-aware blank check - Add resetDefault?: string to SettingField interface - SettingField.vue: render compact Reset button when field.resetDefault is set - Settings.vue: inject resetDefault + updated placeholder from /api/v1/status into the instructions field; maybePrefillInstructions() uses isBlankInstructions so whitespace-only saved values also get prefilled - Dual-trigger prefill: runs from loadConfig() and loadDefaultInstructions() completion + watcher so whichever resolves last wins - 9 new unit tests in instructions-prefill.spec.ts (all passing) - All 186 frontend tests pass Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(ui): persist prefilled instructions on Save without editing (MCP-2484) Codex REQUEST_CHANGES on #685: maybePrefillInstructions() writes the built-in default straight onto state.working, but SettingsSection only PATCHed its locally-tracked dirty keys (set via onChange). A user who opened Settings and clicked Save without editing kept nothing. Derive dirtiness in SettingsSection from working != original (union with the explicit onChange-tracked dirty ref), so any value set outside a control — notably the instructions prefill — is treated as dirty and saved. Reset already worked (it goes through onChange); user edits and discard/save semantics are unchanged. - frontend/tests/unit/settings-section-prefill-dirty.spec.ts: prefilled value (working!=original) is dirty, Save PATCHes instructions, clears after save; equal working/original stays non-dirty. - specs/060-settings-page/verification/instructions-prefill-persist.spec.ts: Playwright — fresh install, Save-without-editing persists, survives reload, Reset repopulates. All 185 frontend unit tests pass; vue-tsc clean; Playwright green. --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
1 parent 3a34c8e commit b8f0d86

7 files changed

Lines changed: 305 additions & 7 deletions

File tree

frontend/src/components/settings/SettingField.vue

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,14 @@
137137

138138
<!-- textarea (multi-line text, e.g. MCP server instructions) -->
139139
<div v-else-if="field.control === 'textarea'" class="flex flex-col items-stretch w-full sm:w-96">
140+
<div v-if="field.resetDefault != null" class="flex justify-end mb-1">
141+
<button
142+
type="button"
143+
class="btn btn-ghost btn-xs text-base-content/50 hover:text-base-content"
144+
:data-test="`setting-reset-${field.key}`"
145+
@click="emitText(field.resetDefault ?? '')"
146+
>↩ Reset to default</button>
147+
</div>
140148
<textarea
141149
class="textarea textarea-bordered textarea-sm w-full font-mono leading-snug"
142150
:class="{ 'textarea-error': validationError }"

frontend/src/components/settings/SettingsSection.vue

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
:key="f.key"
66
:field="f"
77
:model-value="getPath(working, f.key)"
8-
:dirty="!!dirty[f.key]"
8+
:dirty="isFieldDirty(f.key)"
99
@update:model-value="onChange(f, $event)"
1010
/>
1111
<p v-if="!fields.length" class="text-sm text-base-content/50 py-4">No settings match your search.</p>
@@ -98,7 +98,25 @@ const confirmEl = ref<HTMLDialogElement | null>(null)
9898
const pendingMessages = ref<string[]>([])
9999
const pendingInfoOnly = ref(false)
100100
101-
const dirtyKeys = computed(() => Object.keys(dirty.value))
101+
// A field is dirty if the user changed it through a control (tracked in the
102+
// `dirty` ref by onChange) OR its working value diverges from the last-saved
103+
// original. The latter catches values written onto `state.working` from OUTSIDE
104+
// a control — notably the instructions prefill in Settings.vue (MCP-2484).
105+
// Without it, "Save without editing" after a prefill would PATCH nothing because
106+
// the field was never marked dirty.
107+
const dirtyKeys = computed(() => {
108+
const keys = new Set(Object.keys(dirty.value))
109+
for (const f of props.fields) {
110+
if (!eq(getPath(props.working, f.key), getPath(props.original, f.key))) keys.add(f.key)
111+
}
112+
return [...keys]
113+
})
114+
115+
function isFieldDirty(key: string): boolean {
116+
if (key in dirty.value) return true
117+
const f = props.fields.find((x) => x.key === key)
118+
return f != null && !eq(getPath(props.working, key), getPath(props.original, key))
119+
}
102120
103121
// Block Save only when a CHANGED field is invalid — a pre-existing value the
104122
// user hasn't touched must never block saving unrelated edits.

frontend/src/views/Settings.vue

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@
174174
</template>
175175

176176
<script setup lang="ts">
177-
import { ref, reactive, computed, onMounted, onUnmounted } from 'vue'
177+
import { ref, reactive, computed, watch, onMounted, onUnmounted } from 'vue'
178178
import { RouterLink } from 'vue-router'
179179
import { VueMonacoEditor } from '@guolao/vue-monaco-editor'
180180
import { useServersStore } from '@/stores/servers'
@@ -192,6 +192,7 @@ import {
192192
SERVER_EDITION_SECTION_TITLE,
193193
DOCS_BASE,
194194
docsUrl,
195+
isBlankInstructions,
195196
type SettingField,
196197
type SettingsAccordion,
197198
} from '@/views/settings/fields'
@@ -209,21 +210,35 @@ const serverEditionTitle = SERVER_EDITION_SECTION_TITLE
209210
// backend so the `instructions` textarea placeholder never drifts from Go.
210211
const defaultInstructions = ref<string>('')
211212
212-
// Inject the live default as the `instructions` field placeholder. Until the
213-
// fetch resolves (or against an older core that doesn't expose it) the static
214-
// catalogue placeholder ("Loading built-in default…") is used.
213+
// Inject the live default as the `instructions` field placeholder AND as the
214+
// resetDefault value (drives the "Reset to default" button). Until the fetch
215+
// resolves the static catalogue placeholder is used.
215216
const advancedAccordions = computed<SettingsAccordion[]>(() =>
216217
ADVANCED_ACCORDIONS.map((acc) => {
217218
if (!defaultInstructions.value || !acc.fields.some((f) => f.key === 'instructions')) return acc
218219
return {
219220
...acc,
220221
fields: acc.fields.map((f) =>
221-
f.key === 'instructions' ? { ...f, placeholder: defaultInstructions.value } : f
222+
f.key === 'instructions'
223+
? { ...f, placeholder: defaultInstructions.value, resetDefault: defaultInstructions.value }
224+
: f
222225
),
223226
}
224227
})
225228
)
226229
230+
// Prefill the instructions textarea with the built-in default when the config
231+
// field is empty (fresh install or blank override). Only runs when BOTH the
232+
// config AND the default are loaded; whichever resolves second triggers it.
233+
// Never overwrites a user's saved value (non-empty instructions stay as-is).
234+
function maybePrefillInstructions() {
235+
if (!defaultInstructions.value) return
236+
if (!loaded.value) return
237+
if (isBlankInstructions(state.working.instructions)) {
238+
state.working.instructions = defaultInstructions.value
239+
}
240+
}
241+
227242
// ---- form state ----
228243
const loading = ref(false)
229244
const loaded = ref(false)
@@ -332,6 +347,7 @@ async function loadConfig() {
332347
configJson.value = JSON.stringify(cfg, null, 2)
333348
configStatus.value = { valid: true }
334349
loaded.value = true
350+
maybePrefillInstructions()
335351
} else {
336352
loadError.value = response.error || 'Failed to load configuration'
337353
}
@@ -426,12 +442,20 @@ async function loadDefaultInstructions() {
426442
const resp = await api.getStatus()
427443
if (resp.success && typeof resp.data?.default_instructions === 'string') {
428444
defaultInstructions.value = resp.data.default_instructions
445+
// In case loadConfig already ran and set loaded=true, prefill now.
446+
maybePrefillInstructions()
429447
}
430448
} catch {
431449
/* leave placeholder as the static fallback */
432450
}
433451
}
434452
453+
// Also trigger prefill if defaultInstructions resolves after config is loaded
454+
// (handles the common case where /api/v1/status is slower than /api/v1/config).
455+
watch(defaultInstructions, () => {
456+
maybePrefillInstructions()
457+
})
458+
435459
onMounted(() => {
436460
loadConfig()
437461
loadDefaultInstructions()

frontend/src/views/settings/fields.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export interface SettingField {
4949
docs?: string // doc page path on docs.mcpproxy.app, e.g. "/features/docker-isolation"
5050
valueKind?: ValueKind // extra format validation for text/secret fields
5151
optional?: boolean // when true, an empty value is valid (skips kind validation)
52+
resetDefault?: string // when set, render an inline "Reset to default" button that emits this value
5253
}
5354

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

281+
// isBlankInstructions returns true when a saved instructions value is empty /
282+
// whitespace-only / null / undefined — i.e. eligible for prefill from the built-in default.
283+
export function isBlankInstructions(v: string | null | undefined): boolean {
284+
return !v || v.trim() === ''
285+
}
286+
280287
// ---- Section 3: Advanced (subsystem accordions) ----
281288
export const ADVANCED_ACCORDIONS: SettingsAccordion[] = [
282289
{
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
import { describe, it, expect } from 'vitest'
2+
import { isBlankInstructions, type SettingField } from '../../src/views/settings/fields'
3+
4+
// MCP-2484: instructions textarea prefill + reset-to-default
5+
6+
// Unit: SettingField interface accepts resetDefault (compile-time test via import)
7+
describe('SettingField.resetDefault (MCP-2484)', () => {
8+
it('allows a resetDefault property on SettingField without TS error', () => {
9+
const f: SettingField = {
10+
key: 'instructions',
11+
label: 'Server instructions',
12+
control: 'textarea',
13+
optional: true,
14+
resetDefault: 'built-in default text',
15+
}
16+
expect(f.resetDefault).toBe('built-in default text')
17+
})
18+
19+
it('resetDefault is optional — omitting it does not cause a TS error', () => {
20+
const f: SettingField = {
21+
key: 'instructions',
22+
label: 'Server instructions',
23+
control: 'textarea',
24+
optional: true,
25+
}
26+
expect(f.resetDefault).toBeUndefined()
27+
})
28+
})
29+
30+
// Unit: isBlankInstructions helper
31+
describe('isBlankInstructions (MCP-2484)', () => {
32+
it('treats unset/empty/whitespace as blank (eligible for prefill)', () => {
33+
expect(isBlankInstructions(undefined)).toBe(true)
34+
expect(isBlankInstructions(null)).toBe(true)
35+
expect(isBlankInstructions('')).toBe(true)
36+
expect(isBlankInstructions(' \n ')).toBe(true)
37+
})
38+
39+
it('treats a real saved value as non-blank (never overwrite)', () => {
40+
expect(isBlankInstructions('my custom instructions')).toBe(false)
41+
})
42+
})
43+
44+
// Unit: maybePrefillInstructions logic (isolated, not mounting the full component)
45+
describe('maybePrefillInstructions logic (MCP-2484)', () => {
46+
function prefill(working: Record<string, any>, defaultVal: string, isLoaded: boolean) {
47+
if (!defaultVal) return
48+
if (!isLoaded) return
49+
if (isBlankInstructions(working.instructions)) {
50+
working.instructions = defaultVal
51+
}
52+
}
53+
54+
it('prefills instructions when config is empty and default is available', () => {
55+
const w: Record<string, any> = { instructions: '' }
56+
prefill(w, 'built-in default', true)
57+
expect(w.instructions).toBe('built-in default')
58+
})
59+
60+
it('prefills when instructions is null', () => {
61+
const w: Record<string, any> = { instructions: null }
62+
prefill(w, 'built-in default', true)
63+
expect(w.instructions).toBe('built-in default')
64+
})
65+
66+
it('prefills when instructions is undefined', () => {
67+
const w: Record<string, any> = {}
68+
prefill(w, 'built-in default', true)
69+
expect(w.instructions).toBe('built-in default')
70+
})
71+
72+
it('prefills when instructions is whitespace-only', () => {
73+
const w: Record<string, any> = { instructions: ' \n ' }
74+
prefill(w, 'built-in default', true)
75+
expect(w.instructions).toBe('built-in default')
76+
})
77+
78+
it('does NOT overwrite a saved custom instruction', () => {
79+
const w: Record<string, any> = { instructions: 'my custom instructions' }
80+
prefill(w, 'built-in default', true)
81+
expect(w.instructions).toBe('my custom instructions')
82+
})
83+
84+
it('does NOT prefill when config is not yet loaded', () => {
85+
const w: Record<string, any> = { instructions: '' }
86+
prefill(w, 'built-in default', false)
87+
expect(w.instructions).toBe('')
88+
})
89+
90+
it('does NOT prefill when defaultInstructions is empty (API not yet resolved)', () => {
91+
const w: Record<string, any> = { instructions: '' }
92+
prefill(w, '', true)
93+
expect(w.instructions).toBe('')
94+
})
95+
})
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest'
2+
import { mount, flushPromises } from '@vue/test-utils'
3+
import { reactive } from 'vue'
4+
5+
// MCP-2484 (Codex REQUEST_CHANGES on #685): the instructions prefill writes the
6+
// built-in default directly onto `state.working` in Settings.vue. SettingsSection
7+
// PATCHes only its dirty keys, so a value set OUTSIDE a control must still be
8+
// treated as dirty — otherwise "Save without editing" after a prefill persists
9+
// nothing. SettingsSection now derives dirtiness from working≠original.
10+
11+
const patchConfig = vi.fn(() =>
12+
Promise.resolve({ success: true, data: { changed_fields: ['instructions'], requires_restart: false } })
13+
)
14+
15+
vi.mock('@/services/api', () => ({
16+
default: { patchConfig: (...args: unknown[]) => patchConfig(...args) },
17+
}))
18+
vi.mock('@/stores/system', () => ({ useSystemStore: () => ({ addToast: vi.fn() }) }))
19+
20+
import SettingsSection from '@/components/settings/SettingsSection.vue'
21+
import type { SettingField } from '@/views/settings/fields'
22+
23+
const instructionsField: SettingField = {
24+
key: 'instructions',
25+
label: 'Server instructions',
26+
control: 'textarea',
27+
optional: true,
28+
resetDefault: 'BUILT-IN DEFAULT',
29+
}
30+
31+
function mountSection(working: Record<string, unknown>, original: Record<string, unknown>) {
32+
return mount(SettingsSection, {
33+
props: { sectionId: 'mcp', fields: [instructionsField], working: reactive(working), original: reactive(original) },
34+
})
35+
}
36+
37+
describe('SettingsSection prefill-is-dirty (MCP-2484)', () => {
38+
beforeEach(() => patchConfig.mockClear())
39+
40+
it('treats an externally-prefilled value (working≠original) as dirty and saves it', async () => {
41+
// working got the prefilled default written directly; original is still the
42+
// saved (empty) value — exactly the post-prefill state from Settings.vue.
43+
const w = mountSection({ instructions: 'BUILT-IN DEFAULT' }, { instructions: '' })
44+
45+
const save = w.find('[data-test="settings-apply-mcp"]')
46+
expect(save.exists()).toBe(true)
47+
expect((save.element as HTMLButtonElement).disabled).toBe(false)
48+
49+
// the field itself is flagged dirty in the UI
50+
expect(w.find('[data-test="setting-row-instructions"]').classes()).toContain('border-l-warning')
51+
52+
await save.trigger('click')
53+
await flushPromises()
54+
55+
expect(patchConfig).toHaveBeenCalledTimes(1)
56+
expect(patchConfig.mock.calls[0][0]).toMatchObject({ instructions: 'BUILT-IN DEFAULT' })
57+
})
58+
59+
it('is NOT dirty when working equals original (no spurious save)', () => {
60+
const w = mountSection({ instructions: 'same' }, { instructions: 'same' })
61+
expect((w.find('[data-test="settings-apply-mcp"]').element as HTMLButtonElement).disabled).toBe(true)
62+
})
63+
64+
it('clears dirty after a save commits original = working', async () => {
65+
const working = reactive({ instructions: 'BUILT-IN DEFAULT' })
66+
const original = reactive({ instructions: '' })
67+
const w = mount(SettingsSection, {
68+
props: { sectionId: 'mcp', fields: [instructionsField], working, original },
69+
})
70+
await w.find('[data-test="settings-apply-mcp"]').trigger('click')
71+
await flushPromises()
72+
// doSave copies working → original on success; section is no longer dirty
73+
expect((w.find('[data-test="settings-apply-mcp"]').element as HTMLButtonElement).disabled).toBe(true)
74+
})
75+
})

0 commit comments

Comments
 (0)