From 259ad65b3f6fed09c50f4d7763baa9f8bda9dd45 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 31 Oct 2026 18:28:55 -0700 Subject: [PATCH] fix(tagging): refuse non-text default ids and keep one listener per list (#431) A default instructions id that is not text is a 400 on save and falls back with a warning on load; a damaged list warns once and migrates the old template afresh. Option clicks use one listener on the list, the reset note clears once the defaults are saved, and a duplicated long name is cut to fit. --- py/config.py | 19 +++++++--- py/tagging_defaults.py | 2 +- .../js/workspace-system-instructions.test.js | 35 ++++++++++++++++++- tests/test_saved_instructions.py | 30 +++++++++++++++- web/js/workspace/system-instructions.mjs | 22 ++++++++++-- 5 files changed, 98 insertions(+), 10 deletions(-) diff --git a/py/config.py b/py/config.py index a7fcd8a3..4af38a5a 100644 --- a/py/config.py +++ b/py/config.py @@ -775,17 +775,23 @@ class AutotagConfig: f"autotag.defaults.template {error}; using the built-in one" ) entries: List[Dict[str, str]] = [] + damaged = False if "instructions" in defaults: try: entries = validate_instructions(defaults["instructions"]) except ValueError as error: + damaged = True config_logger.warning( f"autotag.defaults.instructions {_reason(error)}; " "using Built-in only" ) chosen = {engine: BUILTIN_INSTRUCTIONS_ID for engine in INSTRUCTION_ENGINES} stored = defaults.get("default_instructions") - if "default_instructions" in defaults and ( + if damaged: + # Both defaults fall back with the list (its one warning says so); + # the old template is migrated as on a first upgrade. + pass + elif "default_instructions" in defaults and ( not isinstance(stored, dict) or set(stored) != set(INSTRUCTION_ENGINES) ): config_logger.warning( @@ -795,8 +801,11 @@ class AutotagConfig: elif isinstance(stored, dict): ids = {item["id"] for item in entries} for engine in INSTRUCTION_ENGINES: - if stored[engine] == BUILTIN_INSTRUCTIONS_ID or stored[engine] in ids: - chosen[engine] = stored[engine] + value = stored[engine] + if isinstance(value, str) and ( + value == BUILTIN_INSTRUCTIONS_ID or value in ids + ): + chosen[engine] = value else: config_logger.warning( f"autotag.defaults.default_instructions.{engine} names no " @@ -805,8 +814,8 @@ class AutotagConfig: cls._migrate_template( entries, chosen, - first="instructions" not in defaults, - keep_defaults="default_instructions" in defaults, + first=damaged or "instructions" not in defaults, + keep_defaults=not damaged and "default_instructions" in defaults, ) cls.INSTRUCTIONS = tuple(entries) cls.DEFAULT_INSTRUCTIONS = chosen diff --git a/py/tagging_defaults.py b/py/tagging_defaults.py index 16446e75..9d00cd96 100644 --- a/py/tagging_defaults.py +++ b/py/tagging_defaults.py @@ -314,7 +314,7 @@ def validate_default_instructions(value: Any, entries) -> Dict[str, str]: raise ValueError("Default instructions must name GGUF and HuggingFace.") ids = {BUILTIN_INSTRUCTIONS_ID} | {item["id"] for item in entries} for engine in INSTRUCTION_ENGINES: - if value[engine] not in ids: + if not isinstance(value[engine], str) or value[engine] not in ids: raise ValueError( f"Default for {ENGINE_LABELS[engine]} must be one of the saved " "instructions." diff --git a/tests/js/workspace-system-instructions.test.js b/tests/js/workspace-system-instructions.test.js index 0954b950..0a1a7814 100644 --- a/tests/js/workspace-system-instructions.test.js +++ b/tests/js/workspace-system-instructions.test.js @@ -83,7 +83,9 @@ async function mounted({ host = HOST } = {}) { const options = () => byId('instruction-list').children.map(option => [option.children[0].textContent, option.children[1]?.children.map(badge => badge.textContent) ?? [], option.getAttribute('aria-selected')]); const click = id => byId(id).dispatch('click'); - const pick = name => byId('instruction-list').children.find(option => option.children[0].textContent === name).dispatch('click'); + // A click on an option reaches the list's one listener (as it bubbles in a browser). + const pick = name => byId('instruction-list').dispatch('click', { target: byId('instruction-list').children + .find(option => option.children[0].textContent === name).children[0] }); return { dom, drafts, scope, block, byId, options, click, pick, root }; } @@ -222,3 +224,34 @@ test('arrow keys move the selection; before the read everything is disabled', as assert.equal(unread.byId('instruction-default-gguf').disabled, true); } finally { unread.scope.dispose(); unread.dom.restore(); } }); + +test('the reset note goes once the defaults are saved; a long name is cut to fit its copy suffix', async () => { + const view = await mounted(); + try { + await view.pick('Custom template'); + await view.click('instruction-delete'); + assert.equal(view.byId('instruction-reset').textContent, 'GGUF and HuggingFace now use Built-in.'); + await view.drafts.save(); + assert.equal(view.byId('instruction-reset').textContent, ''); + await view.pick('Anime'); + view.byId('instruction-name').value = 'x'.repeat(60); + await view.byId('instruction-name').dispatch('input'); + await view.click('instruction-duplicate'); + const copy = view.drafts.get('host', 'autotag_instructions').at(-1).name; + assert.equal(copy, `${'x'.repeat(55)} copy`); + await view.click('instruction-duplicate'); + assert.equal([...view.drafts.get('host', 'autotag_instructions').at(-1).name].length <= 60, true); + } finally { view.scope.dispose(); view.dom.restore(); } +}); + +test('option clicks are one listener on the list, not one per repaint', async () => { + const view = await mounted(); + try { + const list = view.byId('instruction-list'); + const before = list.listeners.length; + for (let i = 0; i < 5; i += 1) await view.pick('Anime'); + assert.equal(list.listeners.length, before); + assert.equal(view.byId('instruction-list').children.every(option => option.listeners.length === 0), true); + assert.equal(view.byId('instruction-name').value, 'Anime'); + } finally { view.scope.dispose(); view.dom.restore(); } +}); diff --git a/tests/test_saved_instructions.py b/tests/test_saved_instructions.py index 3bdbeb8b..f7f4b09f 100644 --- a/tests/test_saved_instructions.py +++ b/tests/test_saved_instructions.py @@ -128,6 +128,10 @@ class TestValidation(unittest.TestCase): {"gguf": "builtin"}, "Default instructions must name GGUF and HuggingFace.", ), + ( + {"gguf": [], "hf": "builtin"}, + "Default for GGUF JoyCaption must be one of the saved instructions.", + ), ("builtin", "Default instructions must name GGUF and HuggingFace."), ): with self.subTest(value=value): @@ -232,7 +236,7 @@ class TestLoad(ConfigTestCase): ], "default_instructions": {"gguf": "0123456789ab", "hf": "builtin"}, }, - warnings=2, + warnings=1, # one warning; the defaults follow the list silently ) self.assertEqual(self.state(), ([], {"gguf": "builtin", "hf": "builtin"})) self.assertEqual( @@ -347,5 +351,29 @@ class TestGetConfig(ConfigTestCase): self.assertNotIn("template", AutotagConfig.get_config()["defaults"]) +class TestDamagedValues(ConfigTestCase): + def test_a_non_text_default_falls_back_with_a_warning(self): + self.load({"default_instructions": {"gguf": [], "hf": "builtin"}}, warnings=1) + self.assertEqual(self.state(), ([], {"gguf": "builtin", "hf": "builtin"})) + + def test_a_damaged_list_warns_once_and_migrates_the_template_afresh(self): + custom = "My own tags, please." + self.load( + { + "template": custom, + "instructions": "broken", + "default_instructions": {"gguf": "0123456789ab", "hf": "builtin"}, + }, + warnings=1, + ) + self.assertEqual( + self.state(), + ( + [entry(sha12(custom), "Custom template", custom)], + {"gguf": sha12(custom), "hf": sha12(custom)}, + ), + ) + + if __name__ == "__main__": unittest.main() diff --git a/web/js/workspace/system-instructions.mjs b/web/js/workspace/system-instructions.mjs index edde28b2..ee3e6bfc 100644 --- a/web/js/workspace/system-instructions.mjs +++ b/web/js/workspace/system-instructions.mjs @@ -74,6 +74,16 @@ export function freeName(base, entries) { for (let n = 2; ; n += 1) if (!taken.has(fold(`${base} (${n})`))) return `${base} (${n})`; } +/** " copy" (then "(2)"…), the name cut so the whole stays within the 60-character limit. */ +export function copyName(name, entries) { + for (let room = MAX_NAME_CHARS - ' copy'.length; room > 0; room -= 1) { + const base = [...String(name ?? '').trim()].slice(0, room).join('').trimEnd(); + const candidate = freeName(INSTRUCTIONS_COPY.copyName(base), entries); + if (characters(candidate) <= MAX_NAME_CHARS) return candidate; + } + return freeName(INSTRUCTIONS_COPY.copyName(BUILTIN_NAME), entries); +} + /** A new entry id: 12 lowercase hex characters from `random` (crypto.getRandomValues), never one already used. */ export function newId(entries, random = bytes => globalThis.crypto.getRandomValues(bytes)) { const used = new Set(entries.map(entry => entry.id)); @@ -249,6 +259,7 @@ export function savedInstructionsBlock({ scope, drafts, random }) { list.replaceChildren(...entries.map(entry => { const option = el('div'); option.id = `instruction-option-${entry.id}`; + option.dataset.id = entry.id; option.setAttribute('role', 'option'); option.setAttribute('aria-selected', String(entry.id === selectedId)); option.append(el('span', entry.name)); @@ -258,13 +269,14 @@ export function savedInstructionsBlock({ scope, drafts, random }) { if (chosen[engine] === entry.id) badges.append(el('span', INSTRUCTIONS_COPY.defaultFor(short), 'chip blue')); } if (badges.children.length) option.append(badges); - scope.listen(option, 'click', () => select(entry.id)); return option; })); list.setAttribute('aria-activedescendant', `instruction-option-${selectedId}`); } function repaint() { + // The note a deleted default left goes once the defaults are saved (or set back). + if (!Object.hasOwn(drafts.changes('host'), DEFAULTS_KEY)) reset.textContent = ''; const loaded = ready(); for (const control of [create, duplicate, remove, nameInput, textArea, ...selects.map(item => item.select)]) { control.disabled = !loaded; @@ -310,9 +322,15 @@ export function savedInstructionsBlock({ scope, drafts, random }) { scope.listen(duplicate, 'click', () => { const entries = allEntries(values()); const source = selected(); - add({ id: newId(entries, random), name: freeName(INSTRUCTIONS_COPY.copyName(source.name), entries), text: source.text }); + add({ id: newId(entries, random), name: copyName(source.name, entries), text: source.text }); }); scope.listen(remove, 'click', deleteSelected); + // One listener for every option, however often the list is rebuilt. + scope.listen(list, 'click', event => { + let node = event.target; + while (node && node !== list && node.getAttribute?.('role') !== 'option') node = node.parentNode; + if (node && node !== list) select(node.dataset.id); + }); scope.listen(nameInput, 'input', () => edit('name', nameInput.value)); scope.listen(textArea, 'input', () => edit('text', textArea.value)); scope.listen(list, 'keydown', event => {