fix: modifying a presentation failed whenever illustration was ticked
Some checks failed
Forgejo Android APK / Root app tests (push) Successful in 53s
Forgejo Docker Build / Root app tests (push) Successful in 54s
Forgejo Android APK / Build signed APK (push) Successful in 1m56s
Forgejo Docker Build / Build Docker image (push) Successful in 22s
Forgejo Docker Build / Deploy to the host (push) Failing after 0s
Some checks failed
Forgejo Android APK / Root app tests (push) Successful in 53s
Forgejo Docker Build / Root app tests (push) Successful in 54s
Forgejo Android APK / Build signed APK (push) Successful in 1m56s
Forgejo Docker Build / Build Docker image (push) Successful in 22s
Forgejo Docker Build / Deploy to the host (push) Failing after 0s
existingDeck was declared below the branch that reads it, so `var` hoisting made it undefined there. With illustration on, a deck modification therefore appended the *markdown* instruction — "Returning the markdown is still required; a tool call is not a substitute for it" — to a prompt whose body asked for deck JSON. The model was told to produce two different artifacts in one reply, the reply parsed as neither, and deckBuild.parse returned null, so the handler answered 502 "That change could not be applied." Moved the declaration above its first reader. Same class of fault as the savedFigureIds one, in the same file. Two things made it hard to see, both fixed: - The library row read created_at, so a modification that did apply left the visible timestamp on the generation time. That timestamp is what led to "modification doesn't work" — it was the only signal available, and it was reading the wrong column. Rows now show the modified time when there is one. - A model can also return the document back unchanged. That was logged server-side and answered "Applied. Download it to see the result", which sent people to download an identical file. The response now carries `unchanged` and the UI says so, keeping the instruction in the box so it can be reworded. Also surfaced has_deck on the library list: 28 of 38 stored presentations have no deck and go through the weaker flat-markdown path, and nothing in the UI distinguished them. They now read "plain text, no slide layout". test/my-resources-refine.test.js runs the handler rather than reading it, since all three faults were invisible to source reading. Verified against a mutation: putting the declaration back where it was fails test 1 and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU
This commit is contained in:
parent
cfb8aab77d
commit
e69eb9a9f7
3 changed files with 222 additions and 18 deletions
|
|
@ -317,7 +317,8 @@
|
|||
var option = document.createElement('option');
|
||||
option.value = String(row.id);
|
||||
option.textContent = (row.title || 'Untitled') +
|
||||
' \u2014 ' + (row.kind === 'article' ? 'article' : 'presentation');
|
||||
' \u2014 ' + (row.kind === 'article' ? 'article'
|
||||
: row.has_deck === false ? 'presentation, plain text' : 'presentation');
|
||||
select.appendChild(option);
|
||||
});
|
||||
if (previous && library.some(function (row) { return String(row.id) === previous; })) {
|
||||
|
|
@ -361,12 +362,20 @@
|
|||
// Same reporting as generating: what it was written from, what was
|
||||
// searched, and any figure that came back.
|
||||
var g = data.grounding || {};
|
||||
say('Applied' + (g.used ? ', using ' + g.count + ' library excerpt' + (g.count === 1 ? '' : 's') : '') +
|
||||
'. Download it to see the result.', 'good');
|
||||
// The model can return the document back unchanged. That is a failed
|
||||
// modification, and saying "Applied" for it sent people off to download
|
||||
// an identical file and conclude the feature was broken.
|
||||
if (data.unchanged) {
|
||||
say('The model returned it unchanged — nothing was modified. ' +
|
||||
'Try naming the slide or section to change, and what to change about it.', 'bad');
|
||||
} else {
|
||||
say('Applied' + (g.used ? ', using ' + g.count + ' library excerpt' + (g.count === 1 ? '' : 's') : '') +
|
||||
'. Download it to see the result.', 'good');
|
||||
}
|
||||
reportSearches(data.searches);
|
||||
showIllustrations(data.imageJobs || []);
|
||||
reportImageFailures(data.imageFailures);
|
||||
if (box) box.value = '';
|
||||
if (box && !data.unchanged) box.value = '';
|
||||
loadLibrary();
|
||||
})
|
||||
.catch(function (err) { say(err.message, 'bad'); })
|
||||
|
|
@ -390,11 +399,21 @@
|
|||
title.style.cssText = 'font-weight:600;font-size:13px;';
|
||||
title.textContent = row.title || 'Untitled';
|
||||
|
||||
// The modified time, when there is one. Showing only the creation time meant
|
||||
// a modification that did apply still looked like nothing had happened, so
|
||||
// the timestamp was the thing people used to conclude modify was broken.
|
||||
var created = new Date(row.created_at);
|
||||
var updated = row.updated_at ? new Date(row.updated_at) : created;
|
||||
var edited = updated - created > 2000;
|
||||
|
||||
var meta = document.createElement('div');
|
||||
meta.style.cssText = 'font-size:11px;color:var(--g500);';
|
||||
meta.textContent = (row.kind === 'article' ? 'Article' : 'Presentation') +
|
||||
' · ' + new Date(row.created_at).toLocaleString() +
|
||||
(row.grounded_count ? ' · ' + row.grounded_count + ' library excerpts' : ' · not grounded');
|
||||
' · ' + (edited ? 'modified ' + updated.toLocaleString()
|
||||
: created.toLocaleString()) +
|
||||
(row.grounded_count ? ' · ' + row.grounded_count + ' library excerpts' : ' · not grounded') +
|
||||
// Only worth saying when it is the weaker kind. A deck is the normal case.
|
||||
(row.kind !== 'article' && row.has_deck === false ? ' · plain text, no slide layout' : '');
|
||||
|
||||
body.appendChild(title);
|
||||
body.appendChild(meta);
|
||||
|
|
|
|||
|
|
@ -439,7 +439,12 @@ router.post('/my-resources/generate', async function (req, res) {
|
|||
router.get('/my-resources', async function (req, res) {
|
||||
try {
|
||||
var rows = await db.all(
|
||||
'SELECT id, title, kind, topic, grounded_count, created_at, updated_at ' +
|
||||
// has_deck, not the deck itself: the list does not need the slides, but a
|
||||
// presentation without one behaves differently enough — flat layout, a
|
||||
// weaker modification path — that the owner should be able to see which
|
||||
// kind they have.
|
||||
'SELECT id, title, kind, topic, grounded_count, created_at, updated_at, ' +
|
||||
"(deck IS NOT NULL AND jsonb_array_length(COALESCE(deck->'slides', '[]'::jsonb)) > 0) AS has_deck " +
|
||||
'FROM user_resources WHERE user_id = ? ORDER BY created_at DESC LIMIT ?',
|
||||
[req.user.id, MAX_PER_USER]
|
||||
);
|
||||
|
|
@ -520,6 +525,19 @@ router.post('/my-resources/:id/refine', async function (req, res) {
|
|||
'a URL to fill the gap: leave the References section as it is.';
|
||||
}
|
||||
|
||||
// A presentation with a stored deck is edited as a deck. Editing its
|
||||
// markdown instead changed only the markdown: export renders from the deck,
|
||||
// so a modification succeeded, said so, and produced an identical download.
|
||||
//
|
||||
// Declared here, above its first reader, and not further down where it used
|
||||
// to sit. `var` hoisted it, so the illustration branch below read it as
|
||||
// undefined and composed the markdown instruction for a deck: the model was
|
||||
// told to return markdown and deck JSON in the same reply, the reply parsed
|
||||
// as neither, and modifying a deck with illustration on failed outright.
|
||||
var existingDeck = existing.deck
|
||||
? (typeof existing.deck === 'string' ? JSON.parse(existing.deck) : existing.deck) : null;
|
||||
if (existingDeck && !(existingDeck.slides || []).length) existingDeck = null;
|
||||
|
||||
// How a figure is asked for depends on which thing is being edited. A deck
|
||||
// places figures by declaring them on a slide; markdown has nowhere to put
|
||||
// one, so it uses the tool. Offering the tool while editing a deck queued a
|
||||
|
|
@ -536,13 +554,6 @@ router.post('/my-resources/:id/refine', async function (req, res) {
|
|||
'never a real patient. Returning the markdown is still required; a tool call is not a ' +
|
||||
'substitute for it, and no image tag or URL goes into the markdown.';
|
||||
|
||||
// A presentation with a stored deck is edited as a deck. Editing its
|
||||
// markdown instead changed only the markdown: export renders from the deck,
|
||||
// so a modification succeeded, said so, and produced an identical download.
|
||||
var existingDeck = existing.deck
|
||||
? (typeof existing.deck === 'string' ? JSON.parse(existing.deck) : existing.deck) : null;
|
||||
if (existingDeck && !(existingDeck.slides || []).length) existingDeck = null;
|
||||
|
||||
var messages = [{ role: 'user', content: existingDeck
|
||||
? 'Revise the following slide deck according to the instruction. Return ONLY the ' +
|
||||
'complete revised JSON object, in exactly the same shape, no commentary and no code ' +
|
||||
|
|
@ -612,22 +623,27 @@ router.post('/my-resources/:id/refine', async function (req, res) {
|
|||
// Said out loud, every time. A modification that changes nothing is the
|
||||
// failure worth catching, and it is invisible from the response: the row
|
||||
// updates, the title updates, and the file is identical.
|
||||
// Computed once, because the caller is told this too. Logging it server-side
|
||||
// while answering "Applied" left the one person who could do something about
|
||||
// it — the person who asked for the change — reading a success message next
|
||||
// to an identical file.
|
||||
var unchanged = revisedDeck
|
||||
? JSON.stringify(existingDeck.slides) === JSON.stringify(revisedDeck.slides)
|
||||
: revised.trim() === String(existing.markdown || '').trim();
|
||||
logRefine({
|
||||
id: existing.id,
|
||||
path: revisedDeck ? 'deck' : 'markdown',
|
||||
outcome: 'applied',
|
||||
before: existingDeck ? (existingDeck.slides || []).length : existing.markdown.length,
|
||||
after: revisedDeck ? (revisedDeck.slides || []).length : revised.length,
|
||||
unchanged: revisedDeck
|
||||
? JSON.stringify(existingDeck.slides) === JSON.stringify(revisedDeck.slides)
|
||||
: revised.trim() === String(existing.markdown || '').trim(),
|
||||
unchanged: unchanged,
|
||||
figures: added.length,
|
||||
model: ai && ai.model,
|
||||
instructions: instructions
|
||||
});
|
||||
|
||||
res.json({
|
||||
success: true, resource: row, markdown: revised,
|
||||
success: true, resource: row, markdown: revised, unchanged: unchanged,
|
||||
grounding: { used: Boolean(sources.corpus.context), count: sources.corpus.sources.length,
|
||||
reason: sources.corpus.reason || null },
|
||||
searches: sources.searches,
|
||||
|
|
|
|||
169
test/my-resources-refine.test.js
Normal file
169
test/my-resources-refine.test.js
Normal file
|
|
@ -0,0 +1,169 @@
|
|||
// Behavioural cover for modifying a resource. The rest of my-resources.test.js
|
||||
// reads the source; these run the handler, because the three faults they pin
|
||||
// were all invisible to source reading — the code said the right thing and did
|
||||
// the wrong one.
|
||||
const test = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const vm = require('node:vm');
|
||||
|
||||
const read = file => fs.readFileSync(path.join(__dirname, '..', file), 'utf8');
|
||||
|
||||
// Normalised, because that is the shape the route stores. Comparing a
|
||||
// hand-written deck with a parsed reply would compare normalisation, not content.
|
||||
const deckBuild = require('../src/utils/deckBuild');
|
||||
const DECK = Object.assign(
|
||||
{ title: 'Croup', subtitle: 'Teaching deck', date: '2026-01-01' },
|
||||
deckBuild.parse(JSON.stringify({
|
||||
slides: [
|
||||
{ type: 'title', title: 'Croup' },
|
||||
{ type: 'bullets', title: 'Features', bullets: ['Barking cough', 'Stridor'] }
|
||||
]
|
||||
}))
|
||||
);
|
||||
|
||||
function router(t, overrides = {}) {
|
||||
const module = { exports: {} };
|
||||
const aiCalls = [];
|
||||
const updates = [];
|
||||
const row = Object.assign({
|
||||
id: 5, user_id: 7, title: 'Croup', kind: 'presentation', topic: 'croup',
|
||||
markdown: '# Croup\n\n- Barking cough\n- Stridor\n', deck: JSON.stringify(DECK)
|
||||
}, overrides.row || {});
|
||||
const quiet = { log() {}, error() {}, warn() {}, info() {} };
|
||||
|
||||
const mocks = {
|
||||
express: require('express'),
|
||||
os: require('os'),
|
||||
path: require('path'),
|
||||
'fs/promises': require('fs/promises'),
|
||||
'../db/database': {
|
||||
get: async (sql, params) => {
|
||||
if (/^UPDATE user_resources/.test(sql)) {
|
||||
updates.push({ sql, params });
|
||||
return { id: row.id, title: row.title, updated_at: '2026-01-02T00:00:00Z' };
|
||||
}
|
||||
return row;
|
||||
},
|
||||
all: async () => [],
|
||||
run: async () => ({}),
|
||||
getSetting: async key => (key === 'clinical_assistant.image_model' ? 'synthetic-image' : '')
|
||||
},
|
||||
'../middleware/auth': { authMiddleware: (req, res, next) => next() },
|
||||
'../utils/ai': {
|
||||
callAI: async (messages, options) => {
|
||||
aiCalls.push({ messages, options });
|
||||
return { content: overrides.reply, model: 'synthetic' };
|
||||
},
|
||||
discoverModels: async () => []
|
||||
},
|
||||
'../utils/deckBuild': deckBuild,
|
||||
'../utils/deckSchema': require('../src/utils/deckSchema'),
|
||||
'../utils/deckReview': { enabled: async () => false, review: async d => d },
|
||||
'../utils/documentExport': { FORMATS: {}, isSupported: () => false, filename: () => 'x', mimeFor: () => '', render: async () => ({}), renderDeck: async () => ({}) },
|
||||
'../utils/generatedImages': { service: () => ({ get: async () => null, asset: async () => null }), workflows: ['my_resources'] },
|
||||
'../utils/learningRetrieval': { retrieve: async () => ({ context: '', sources: [], reason: null }) },
|
||||
'../utils/metrics': { resourceRefines: { inc() {} }, resourceVocabularyGaps: { inc() {} } },
|
||||
'../utils/pubmedSearch': { isAvailable: async () => false, search: async () => ({ results: [] }), formatForPrompt: () => '' },
|
||||
'../utils/webSearch': { isAvailable: async () => false, search: async () => ({ results: [] }), formatForPrompt: () => '' },
|
||||
'../utils/resourceImages': { tools: [{ name: 'draw' }], dispatch: async x => x, guidance: () => 'GUIDANCE.', requestedCount: () => 0, MAX_IMAGES: 6 }
|
||||
};
|
||||
|
||||
vm.runInNewContext(read('src/routes/myResources.js'), {
|
||||
module, exports: module.exports, console: quiet, Buffer, JSON, Date, Math,
|
||||
process: { env: {} }, setTimeout() {},
|
||||
require(name) { assert.ok(Object.hasOwn(mocks, name), 'Unexpected import: ' + name); return mocks[name]; }
|
||||
});
|
||||
|
||||
async function request(method, routePath, body) {
|
||||
const layer = module.exports.stack.find(l => l.route && l.route.path === routePath && l.route.methods[method]);
|
||||
assert.ok(layer, 'route missing: ' + method + ' ' + routePath);
|
||||
const stack = layer.route.stack;
|
||||
const handler = stack[stack.length - 1].handle;
|
||||
const res = {
|
||||
statusCode: 200, body: null,
|
||||
status(code) { this.statusCode = code; return this; },
|
||||
json(payload) { this.body = payload; return this; },
|
||||
setHeader() {}, send() { return this; }
|
||||
};
|
||||
await handler({ body, params: { id: '5' }, query: {}, user: { id: 7 } }, res);
|
||||
return res;
|
||||
}
|
||||
return { request, aiCalls, updates };
|
||||
}
|
||||
|
||||
test('modifying a deck asks for a deck, even when illustration is on', async () => {
|
||||
// The fault: existingDeck was declared below the branch that read it, so var
|
||||
// hoisting made it undefined there. Illustration on a deck therefore appended
|
||||
// the markdown instruction — "Returning the markdown is still required" —
|
||||
// to a prompt whose body asked for deck JSON. The model was told to produce
|
||||
// two different artifacts at once and the reply parsed as neither, so every
|
||||
// modification of a presentation with illustration ticked failed with 502.
|
||||
const r = router(null, {
|
||||
reply: JSON.stringify({ slides: [DECK.slides[0], { type: 'bullets', title: 'Features', bullets: ['Barking cough', 'Stridor', 'Hoarse voice'] }] })
|
||||
});
|
||||
const res = await r.request('post', '/my-resources/:id/refine', {
|
||||
instructions: 'add a third feature', withImages: 'true'
|
||||
});
|
||||
|
||||
assert.equal(res.statusCode, 200, 'a deck modification with images on must not 502');
|
||||
const prompt = r.aiCalls[0].messages[0].content;
|
||||
assert.match(prompt, /Revise the following slide deck/);
|
||||
assert.match(prompt, /Add "image_prompt" to the slides/, 'the deck illustration instruction');
|
||||
assert.doesNotMatch(prompt, /Returning the markdown is still required/,
|
||||
'the markdown instruction must never reach a deck prompt');
|
||||
assert.doesNotMatch(prompt, /An illustration tool is available/,
|
||||
'a deck declares its figures; it is not given the tool');
|
||||
assert.equal(r.aiCalls[0].options.tools, undefined, 'and no tool schema is attached');
|
||||
});
|
||||
|
||||
test('a modification the model returned unchanged says so instead of claiming success', async () => {
|
||||
// Answering "Applied" for a reply identical to the original sent people off
|
||||
// to download the same file and conclude the feature was broken. It was
|
||||
// logged server-side, where the person who could reword the instruction
|
||||
// could not see it.
|
||||
const r = router(null, { reply: JSON.stringify({ slides: DECK.slides }) });
|
||||
const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'make it better' });
|
||||
|
||||
assert.equal(res.statusCode, 200);
|
||||
assert.equal(res.body.unchanged, true, 'the caller is told the deck came back identical');
|
||||
});
|
||||
|
||||
test('a modification that did change the deck reports itself as changed', async () => {
|
||||
const r = router(null, {
|
||||
reply: JSON.stringify({ slides: [DECK.slides[0], { type: 'bullets', title: 'Features', bullets: ['Barking cough', 'Stridor', 'Hoarse voice'] }] })
|
||||
});
|
||||
const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'add a third feature' });
|
||||
|
||||
assert.equal(res.body.unchanged, false);
|
||||
assert.equal(r.updates.length, 1, 'and the row is written');
|
||||
assert.match(r.updates[0].sql, /updated_at = NOW\(\)/);
|
||||
});
|
||||
|
||||
test('a presentation with no deck still modifies, through the markdown path', async () => {
|
||||
const r = router(null, { row: { deck: null }, reply: '# Croup\n\n- Barking cough\n- Stridor\n- Hoarse voice\n' });
|
||||
const res = await r.request('post', '/my-resources/:id/refine', {
|
||||
instructions: 'add a third feature', withImages: 'true'
|
||||
});
|
||||
|
||||
assert.equal(res.statusCode, 200);
|
||||
assert.equal(res.body.unchanged, false);
|
||||
const prompt = r.aiCalls[0].messages[0].content;
|
||||
assert.match(prompt, /Revise the following Pandoc markdown/);
|
||||
assert.match(prompt, /An illustration tool is available/, 'markdown has nowhere to declare a figure');
|
||||
});
|
||||
|
||||
test('the library says which presentations carry a deck, and the row shows when it was modified', () => {
|
||||
// Both halves of "modify does nothing": the list read created_at, so a
|
||||
// modification that did apply left the visible timestamp untouched, and
|
||||
// nothing distinguished a real deck from a flat one.
|
||||
const route = read('src/routes/myResources.js');
|
||||
assert.match(route, /AS has_deck/);
|
||||
|
||||
const ui = read('public/js/myResources.js');
|
||||
assert.match(ui, /row\.updated_at \? new Date\(row\.updated_at\)/);
|
||||
assert.match(ui, /edited \? 'modified '/);
|
||||
assert.match(ui, /has_deck === false \? ' · plain text, no slide layout' : ''/);
|
||||
assert.match(ui, /if \(data\.unchanged\) \{/);
|
||||
});
|
||||
Loading…
Reference in a new issue