From ae602a1852f33f8bbe4908ffaa5f3f0477e578b6 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 13 Sep 2026 02:37:57 +0200 Subject: [PATCH] fix: My Resources uses the image model you chose for it, and says so on save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "The model is chosen per request, where the deck is generated" described something that happens nowhere. There is no per-request picker, and asking where it was is what exposed the real bug. My Resources read clinical_assistant.image_model while the admin screen saved my_resources.image_model. That setting was stored, returned by the API and rendered into the form — and never used by anything. Somebody noticed the field did nothing and disabled it rather than finding out why, which left a control that could not be changed and a note explaining a mechanism that does not exist. My note repeating it was wrong too. The generator now reads its own setting and falls back to the Assistant's, so an install that only ever set one model keeps working untouched, and the field is enabled again with "leave blank to use the Clinical Assistant's" — which is now true rather than a rationalisation. Saving also says what it saved. "Saved 8:31:59 PM. Decks will be reviewed by ..." answered a different question from the one an admin actually has, which is whether the model they just picked is the one that will draw. It now names each workflow's model and fallbacks back, and spells out the blank case rather than leaving a gap. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- public/js/admin/clinicalAssistant.js | 11 +++++++--- public/js/admin/imageSettings.js | 33 ++++++++++++++++------------ src/routes/myResources.js | 22 +++++++++++++++++-- test/frontend-prompt-env.test.js | 29 ++++++++++++++++++++++-- 4 files changed, 74 insertions(+), 21 deletions(-) diff --git a/public/js/admin/clinicalAssistant.js b/public/js/admin/clinicalAssistant.js index d2e6c837..fa83b777 100644 --- a/public/js/admin/clinicalAssistant.js +++ b/public/js/admin/clinicalAssistant.js @@ -408,12 +408,17 @@ export function initClinicalAssistantAdmin(adminEscapeHtml) { putAssistantConfig('clinical_assistant.allowed_image_models', checkedAssistantModels('assistant-allowed-image-models').join(',')), putAssistantConfig('my_resources.review_model', getValue('mr-review-model')), saveImageSettings() - ]).then(function() { + ]).then(function(results) { var reviewer = getValue('mr-review-model'); + // The image models named back. "Saved" does not tell an administrator + // that the model they chose is the one that will draw — which is the + // whole question, and the reason this card was hard to trust. + var images = Array.isArray(results[results.length - 1]) ? results[results.length - 1] : []; if (status) { status.textContent = 'Saved ' + new Date().toLocaleTimeString() + '. ' + - (reviewer ? 'Decks will be reviewed by ' + reviewer + '.' : 'Slide review is off.') + - ' New image jobs use these settings; existing jobs are unchanged.'; + (images.length ? 'Images — ' + images.join('; ') + '. ' : '') + + (reviewer ? 'Decks reviewed by ' + reviewer + '. ' : 'Slide review is off. ') + + 'New image jobs use these settings; existing jobs are unchanged.'; } showToast('Availability saved', 'success'); }).catch(function(err) { diff --git a/public/js/admin/imageSettings.js b/public/js/admin/imageSettings.js index 07b82de7..ba856f5f 100644 --- a/public/js/admin/imageSettings.js +++ b/public/js/admin/imageSettings.js @@ -50,21 +50,18 @@ async function load() { group.appendChild(legend); // My Resources picks its model per request from the roster, so only the - // Only the Assistant names a primary here. My Resources takes its model - // from the request that asks for the picture, so a disabled, permanently - // empty dropdown labelled "set per request" was a control that could - // never do anything — it read as broken rather than as not applicable. - // Its fallbacks still belong here: those are what runs when the chosen - // model fails, whoever chose it. + // Both workflows name a model. My Resources' was saved and ignored for a + // while — the generator read the Assistant's setting instead — so the + // field was disabled and labelled "set per request", describing something + // that happens nowhere. It works now, and blank means "use the Clinical + // Assistant's", which is what every install did before it worked. const primary = makeSelect([saved.model], discovered); + group.appendChild(row('Model', primary)); if (workflow.key === 'my_resources') { const note = document.createElement('p'); - note.style.cssText = 'margin:2px 0 8px;font-size:12px;color:var(--g500);'; - note.textContent = 'The model is chosen per request, where the deck is generated. ' + - 'The fallbacks below apply to whichever model that request picked.'; + note.style.cssText = 'margin:2px 0 10px;font-size:12px;color:var(--g500);'; + note.textContent = 'Leave blank to use the Clinical Assistant\'s image model above.'; group.appendChild(note); - } else { - group.appendChild(row('Model', primary)); } const fallbacks = []; @@ -100,18 +97,26 @@ async function load() { // settings that were not sent. export async function saveImageSettings() { if (!loaded) throw new Error('Image settings were not loaded, so they were not saved.'); + // What was written, for the card's confirmation. "Saved" alone does not + // answer the question an admin actually has, which is whether the model they + // just picked is the one that will be used. + const written = []; for (const workflow of WORKFLOWS) { const control = controls[workflow.key]; const body = { budget: Number(control.budget.value), fallbacks: control.fallbacks.map(s => s.value).filter(Boolean) }; - // Only the workflows that own a primary send one; My Resources would - // be overwriting a per-request choice with a form field. - if (workflow.key !== 'my_resources') body.model = control.primary.value; + // Every workflow sends its model. My Resources' used to be withheld, + // which is why its field could never be set to anything. + body.model = control.primary.value; await imageJson('/api/admin/image-settings/' + workflow.key, { method: 'PUT', body: JSON.stringify(body) }); + written.push(workflow.label + ': ' + + (body.model || 'the Clinical Assistant\'s model') + + (body.fallbacks.length ? ', then ' + body.fallbacks.join(', ') : ', no fallback')); } + return written; } function row(labelText, control) { diff --git a/src/routes/myResources.js b/src/routes/myResources.js index 7e3a9ac9..e8f11b3c 100644 --- a/src/routes/myResources.js +++ b/src/routes/myResources.js @@ -209,6 +209,24 @@ router.get('/my-resources/image/jobs/:id', async function (req, res) { } }); +// Which model draws the figures in a resource. +// +// Its own setting, with the Clinical Assistant's as the fallback. This used to +// read the Assistant's and only the Assistant's, while the admin screen saved +// my_resources.image_model — so that field was stored, returned by the API, +// shown in the form, and never used. Somebody noticed the field did nothing and +// disabled it rather than finding out why, which left a control that could not +// be changed and a note saying the model was "chosen per request", which is not +// a thing that happens anywhere. +// +// Blank still means "use the Assistant's", so an install that only ever set one +// model keeps working without anyone touching the screen. +async function resourceImageModel() { + var own = String(await db.getSetting('my_resources.image_model', '') || ''); + if (own) return own; + return String(await db.getSetting('clinical_assistant.image_model', '') || ''); +} + router.get('/my-resources/options', async function (req, res) { try { var models = await allowedModels(); @@ -216,7 +234,7 @@ router.get('/my-resources/options', async function (req, res) { success: true, models: models.allowed, defaultModel: models.configured, - imagesAvailable: Boolean(await db.getSetting('clinical_assistant.image_model', '')), + imagesAvailable: Boolean(await resourceImageModel()), // Whether this person has somewhere to send a file. The button is hidden // rather than shown-and-refused: an action that always fails is worse // than an action that is not offered. @@ -275,7 +293,7 @@ async function gatherSources(subject, body, keywords) { // Asking for an illustration when no image model is configured is not an // error, it just cannot happen; the caller reports that rather than failing. - var imageModel = wantsImages ? String(await db.getSetting('clinical_assistant.image_model', '') || '') : ''; + var imageModel = wantsImages ? await resourceImageModel() : ''; return { corpus: corpus, searches: searches, literature: literature, webFindings: webFindings, // A search that was asked for and came back empty is the dangerous case: the diff --git a/test/frontend-prompt-env.test.js b/test/frontend-prompt-env.test.js index 44dd0197..2fd1a38a 100644 --- a/test/frontend-prompt-env.test.js +++ b/test/frontend-prompt-env.test.js @@ -421,9 +421,12 @@ test('image-model dropdowns keep saved selections through discovery failures and 'Learning Hub is gone; saving to it is what broke the card'); assert.deepEqual(puts().find(c => c.url.endsWith('clinical_assistant')).body, { model: 'saved-image', budget: 32000, fallbacks: ['saved-backup'] }); - // My Resources chooses its model per request, so the form must not send one. + // My Resources sends its model like every other workflow. It used to be + // withheld — the generator read the Clinical Assistant's setting instead, so + // my_resources.image_model was saved and ignored, and the field was disabled + // rather than fixed. Blank is meaningful: it means "use the Assistant's". assert.deepEqual(puts().find(c => c.url.endsWith('my_resources')).body, - { budget: 32000, fallbacks: [] }); + { model: '', budget: 32000, fallbacks: [] }); assert.equal(select.value, 'saved-image', 'save never reverts the selection'); }); @@ -513,3 +516,25 @@ test('catalogue loading failure always reaches a visible retry state, never an e assert.doesNotMatch(f.textContent, /Loading/, 'no eternal spinner text remains'); } }); + +test('saving names the image models back, so a choice can be seen to have taken', async t => { + // "Saved 8:31:59 PM" answered a different question from the one an admin has, + // which is whether the model they just picked is the one that will draw. + const ui = await browser(t, 'admin', url => { + if (url === '/api/admin/config') return json(assistantConfig()); + if (url === '/api/admin/image-settings') return json({ success: true, maxModels: 3, workflows: { + clinical_assistant: { model: 'draws-this', fallbacks: ['then-this'], budget: 32000 }, + my_resources: { model: '', fallbacks: [], budget: 32000 } } }); + if (url.endsWith('/image-models/discover')) return json({ models: [{ id: 'draws-this' }, { id: 'then-this' }] }); + if (url.includes('/api/admin/image-settings/')) return json({ success: true }); + }); + for (let i = 0; i < 6; i++) await tick(); + ui.document.getElementById('btn-save-availability').click(); + for (let i = 0; i < 10; i++) await tick(); + + const said = ui.document.getElementById('assistant-availability-status').textContent; + assert.match(said, /Clinical Assistant: draws-this, then then-this/); + // Blank is meaningful and is spelled out rather than shown as an empty gap. + assert.match(said, /My Resources: the Clinical Assistant's model/); + assert.match(said, /existing jobs are unchanged/); +});