mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-06-28 23:52:09 -04:00
fix: group selection drop-downs recreation and repopulation logic (#3424)
* fix: include in-memory templates in group participant character list _getCharacterList() only fetched user templates from the /api/presets/templates endpoint. When a character was just created in the Character tab, the async auto-save to the templates API might not have completed by the time the Group tab loaded its participant dropdown — causing newly created characters to be missing. Now also merges the in-memory userTemplates array from presets.js as a fallback. These are updated as soon as the async save completes (via the loadUserTemplates callback), so they bridge the gap between character creation and API persistence. Fixes #3207 * fix: optimistic userTemplates update on character save Update the in-memory userTemplates array immediately when saveCustomPreset() succeeds, before the fire-and-forget templates API POST completes. This bridges the timing gap where _getCharacterList() calls getUserTemplates() and gets stale data because loadUserTemplates() hasn't been triggered yet. * test: verify group participant dropdown merges in-memory templates Source-level guards for the #3207 fix: - group.js imports and calls getUserTemplates() to merge in-memory templates - presets.js exports getUserTemplates and does optimistic in-memory update on save 5 tests ensuring the fix can't be silently reverted. * fix: generate client-side id for optimistic update, return shallow copy from getUserTemplates 1. New characters now get a 'user-<hex>' id immediately on save, matching the server's convention (uuid.uuid4().hex[:8]). Previously the id was '' which the merge guard in _getCharacterList filtered as falsy. 2. getUserTemplates() now returns [...userTemplates] so callers cannot accidentally mutate module state. * fix(group.js): fix selection drop-downs behavior - add an identifier to the selection drop-downs based on what type it is. - fix behavior of continuously adding a row when a user clicks the "Group" tab button. - fix behavior of not repopulating existing selection drop-downs whenever a user clicks the "Group" tab button. * fix(#3207): remove duplicate of latest persona - fix the duplication of the latest persona or character being shown in selection drop-downs. - remove unnecessary blocks of code in `_getCharacterList()` - add functionality to show error toast if saving a preset template/character fails. - add functionality to revert optimistic update of preset template/character if saving fails. * chore(group.js,preset.js): fix test & format errors remove trailing whitespaces in lines 230 and 232 in /static/group.js add back the expected syntax from tests/test_group_character_dropdown.py * fix(presets.js,group.js): fix runtime errors as stated in a comment by @alteixeira20, runtime errors exist for the applied fixes. fixes: - missing ending `]` querySelectorAll("select.preset-input[data-selection-type=character") in `group.js` - spelling error in `modelSelection.vale` in `group.js` - fix the ordering logic error in optimistic rollback where `Object.assign` is called first before the clone happens in `saveCustomPreset` in `presets.js`. - add tests for the cloning logic bug with the same format as previous tests by checking the order of LOC in `tests/test_group_character_dropdown.py`. --------- Co-authored-by: michaelxer <michaelxer@users.noreply.github.com> Co-authored-by: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com>
This commit is contained in:
+72
-13
@@ -6,7 +6,7 @@ import markdownModule from './markdown.js';
|
|||||||
import chatRenderer from './chatRenderer.js';
|
import chatRenderer from './chatRenderer.js';
|
||||||
import spinnerModule from './spinner.js';
|
import spinnerModule from './spinner.js';
|
||||||
import { providerLogo } from './providers.js';
|
import { providerLogo } from './providers.js';
|
||||||
import { PROMPT_TEMPLATES, getAllPresets } from './presets.js';
|
import { PROMPT_TEMPLATES, getUserTemplates } from './presets.js';
|
||||||
import { sortModelObjects } from './modelSort.js';
|
import { sortModelObjects } from './modelSort.js';
|
||||||
import Storage from './storage.js';
|
import Storage from './storage.js';
|
||||||
|
|
||||||
@@ -89,12 +89,16 @@ function _initGroupTab() {
|
|||||||
|
|
||||||
const charSel = document.createElement('select');
|
const charSel = document.createElement('select');
|
||||||
charSel.className = 'preset-input';
|
charSel.className = 'preset-input';
|
||||||
|
// add an identifier that this is a character selection
|
||||||
|
charSel.dataset.selectionType = "character"
|
||||||
charSel.style.cssText = 'font-size:11px;flex:1;height:26px;';
|
charSel.style.cssText = 'font-size:11px;flex:1;height:26px;';
|
||||||
charSel.innerHTML = '<option value="">Empty...</option>' +
|
charSel.innerHTML = '<option value="">Empty...</option>' +
|
||||||
characters.map(c => '<option value="' + c.id + '">' + uiModule.esc(c.name) + '</option>').join('');
|
characters.map(c => '<option value="' + c.id + '">' + uiModule.esc(c.name) + '</option>').join('');
|
||||||
|
|
||||||
const modelSel = document.createElement('select');
|
const modelSel = document.createElement('select');
|
||||||
modelSel.className = 'preset-input';
|
modelSel.className = 'preset-input';
|
||||||
|
// add an identifier that this is a model selection
|
||||||
|
modelSel.dataset.selectionType = "model"
|
||||||
modelSel.style.cssText = 'font-size:11px;flex:1;height:26px;';
|
modelSel.style.cssText = 'font-size:11px;flex:1;height:26px;';
|
||||||
modelSel.innerHTML = '<option value="">Model…</option>' +
|
modelSel.innerHTML = '<option value="">Model…</option>' +
|
||||||
models.map(m => '<option value="' + m.mid + '">' + uiModule.esc(m.display) + '</option>').join('');
|
models.map(m => '<option value="' + m.mid + '">' + uiModule.esc(m.display) + '</option>').join('');
|
||||||
@@ -196,15 +200,67 @@ function _initGroupTab() {
|
|||||||
});
|
});
|
||||||
|
|
||||||
const groupTab = document.querySelector('.preset-tab[data-chartab="group"]');
|
const groupTab = document.querySelector('.preset-tab[data-chartab="group"]');
|
||||||
|
// whenever a user navigates to the Group tab
|
||||||
if (groupTab) groupTab.addEventListener('click', () => {
|
if (groupTab) groupTab.addEventListener('click', () => {
|
||||||
_modelsCache = null;
|
_modelsCache = null;
|
||||||
if (startBtn) startBtn.textContent = 'Start Group';
|
if (startBtn) startBtn.textContent = 'Start Group';
|
||||||
_loadGroupPresets();
|
_loadGroupPresets();
|
||||||
if (_groupParticipants.length === 0) {
|
|
||||||
|
const isGroupTabUnInitialized =
|
||||||
|
_groupParticipants.length === 0 && participantsEl.children.length === 0;
|
||||||
|
|
||||||
|
if (isGroupTabUnInitialized) {
|
||||||
setTimeout(() => addBtn.click(), 100);
|
setTimeout(() => addBtn.click(), 100);
|
||||||
|
} else {
|
||||||
|
// queue this asynchronously since repopulating the selection drop-downs
|
||||||
|
// do not need to be visible right away; it can be safely delayed before
|
||||||
|
// the next event loop
|
||||||
|
queueMicrotask(() => {
|
||||||
|
repopulateExistingSelections();
|
||||||
|
})
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
async function repopulateExistingSelections() {
|
||||||
|
const EMPTY = "";
|
||||||
|
|
||||||
|
const characterSelections = participantsEl.querySelectorAll("select.preset-input[data-selection-type=character]");
|
||||||
|
const modelSelections = participantsEl.querySelectorAll("select.preset-input[data-selection-type=model]");
|
||||||
|
|
||||||
|
if (characterSelections.length !== 0) {
|
||||||
|
const characters = await _getCharacterList();
|
||||||
|
|
||||||
|
characterSelections.forEach((characterSelection) => {
|
||||||
|
|
||||||
|
const chosenCharacter = characterSelection.value;
|
||||||
|
const isChosenCharacterExisting = chosenCharacter !== EMPTY
|
||||||
|
&& characters.findIndex((char) => char.id === chosenCharacter) !== -1;
|
||||||
|
|
||||||
|
characterSelection.innerHTML = '<option value="">Empty...</option>' +
|
||||||
|
characters.map(c => '<option value="' + c.id + '">' + uiModule.esc(c.name) + '</option>').join('');
|
||||||
|
if (isChosenCharacterExisting) {
|
||||||
|
characterSelection.value = chosenCharacter;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
if (modelSelections.length !== 0) {
|
||||||
|
const models = await _getModels();
|
||||||
|
|
||||||
|
modelSelections.forEach((modelSelection) => {
|
||||||
|
const chosenModel = modelSelection.value;
|
||||||
|
const isChosenModelExisting = chosenModel !== EMPTY
|
||||||
|
&& models.findIndex((model) => model.mid === chosenModel) !== -1;
|
||||||
|
|
||||||
|
modelSelection.innerHTML = '<option value="">Model…</option>' +
|
||||||
|
models.map(m => '<option value="' + m.mid + '">' + uiModule.esc(m.display) + '</option>').join('');
|
||||||
|
if (isChosenModelExisting) {
|
||||||
|
modelSelection.value = chosenModel;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Load and render saved group presets
|
// Load and render saved group presets
|
||||||
async function _loadGroupPresets() {
|
async function _loadGroupPresets() {
|
||||||
try {
|
try {
|
||||||
@@ -288,17 +344,6 @@ async function _getCharacterList() {
|
|||||||
const chars = PROMPT_TEMPLATES.filter(t => t.isCharacter).map(t => ({
|
const chars = PROMPT_TEMPLATES.filter(t => t.isCharacter).map(t => ({
|
||||||
id: t.id, name: t.name, prompt: t.prompt,
|
id: t.id, name: t.name, prompt: t.prompt,
|
||||||
}));
|
}));
|
||||||
// User-created characters from presets
|
|
||||||
try {
|
|
||||||
const allPresets = getAllPresets();
|
|
||||||
if (allPresets && allPresets.custom && allPresets.custom.character_name) {
|
|
||||||
chars.push({
|
|
||||||
id: 'custom',
|
|
||||||
name: allPresets.custom.character_name,
|
|
||||||
prompt: allPresets.custom.system_prompt || allPresets.custom.prompt || '',
|
|
||||||
});
|
|
||||||
}
|
|
||||||
} catch (e) {}
|
|
||||||
// Load user templates and wait for them before returning.
|
// Load user templates and wait for them before returning.
|
||||||
// The endpoint returns a JSON array directly (not {templates:[...]}).
|
// The endpoint returns a JSON array directly (not {templates:[...]}).
|
||||||
// All user templates are personas by definition — no isCharacter filter needed.
|
// All user templates are personas by definition — no isCharacter filter needed.
|
||||||
@@ -306,12 +351,26 @@ async function _getCharacterList() {
|
|||||||
const r = await fetch(API_BASE + '/api/presets/templates', { credentials: 'same-origin' });
|
const r = await fetch(API_BASE + '/api/presets/templates', { credentials: 'same-origin' });
|
||||||
const data = await r.json();
|
const data = await r.json();
|
||||||
const templates = Array.isArray(data) ? data : (data.templates || []);
|
const templates = Array.isArray(data) ? data : (data.templates || []);
|
||||||
|
|
||||||
templates.forEach(t => {
|
templates.forEach(t => {
|
||||||
if (t.id && t.name && !chars.find(c => c.id === t.id)) {
|
if (t.id && t.name && !chars.find(c => c.id === t.id)) {
|
||||||
chars.push({ id: t.id, name: t.name, prompt: t.system_prompt || t.prompt || '' });
|
chars.push({ id: t.id, name: t.name, prompt: t.system_prompt || t.prompt || '' });
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
} catch (e) {}
|
} catch (e) {}
|
||||||
|
|
||||||
|
// Also merge in-memory templates from presets.js — these may include
|
||||||
|
// newly created characters whose async save-to-API hasn't completed yet.
|
||||||
|
const memTemplates = getUserTemplates();
|
||||||
|
|
||||||
|
if (Array.isArray(memTemplates)) {
|
||||||
|
memTemplates.forEach(t => {
|
||||||
|
if (t.id && t.name && !chars.find(c => c.id === t.id)) {
|
||||||
|
chars.push({ id: t.id, name: t.name, prompt: t.system_prompt || t.prompt || '' });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
return chars;
|
return chars;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+48
-7
@@ -830,15 +830,48 @@ export async function saveCustomPreset(showToast, showError) {
|
|||||||
const _selVal = document.getElementById('char-template-select')?.value || '';
|
const _selVal = document.getElementById('char-template-select')?.value || '';
|
||||||
const isBuiltinPreset = PROMPT_TEMPLATES.some(t => t.isPreset && (t.name === name || t.name === _selVal));
|
const isBuiltinPreset = PROMPT_TEMPLATES.some(t => t.isPreset && (t.name === name || t.name === _selVal));
|
||||||
const saveName = isBuiltinPreset ? null : (name || null);
|
const saveName = isBuiltinPreset ? null : (name || null);
|
||||||
|
|
||||||
if (saveName) {
|
if (saveName) {
|
||||||
fetch(`${API_BASE}/api/presets/templates`, {
|
const _existing = userTemplates.find(t => t.name === saveName);
|
||||||
method: 'POST',
|
let clone;
|
||||||
|
const _entry = {
|
||||||
|
id: _existing && _existing.id
|
||||||
|
|| 'user-' + Math.random().toString(16).slice(2, 10),
|
||||||
|
name: saveName,
|
||||||
|
// use ?? since it's more semantic for null-coalescing
|
||||||
|
system_prompt: system_prompt ?? '',
|
||||||
|
temperature: config.temperature,
|
||||||
|
max_tokens: config.max_tokens,
|
||||||
|
}
|
||||||
|
const ENDPOINT = `${API_BASE}/api/presets/templates`;
|
||||||
|
|
||||||
|
// Optimistically update the in-memory templates list by @michaelxer
|
||||||
|
if (_existing) {
|
||||||
|
// slow but works for now
|
||||||
|
clone = JSON.parse(JSON.stringify(_existing));
|
||||||
|
|
||||||
|
Object.assign(_existing, _entry);
|
||||||
|
} else {
|
||||||
|
userTemplates.push(_entry);
|
||||||
|
}
|
||||||
|
|
||||||
|
fetch(ENDPOINT, {
|
||||||
|
method: "POST",
|
||||||
headers: { 'Content-Type': 'application/json' },
|
headers: { 'Content-Type': 'application/json' },
|
||||||
body: JSON.stringify({
|
body: JSON.stringify(_entry)
|
||||||
id: (userTemplates.find(t => t.name === saveName) || {}).id || '',
|
}).then((r) => {
|
||||||
name: saveName, system_prompt, temperature: config.temperature, max_tokens: config.max_tokens,
|
if (r.ok) {
|
||||||
}),
|
loadUserTemplates();
|
||||||
}).then(r => { if (r.ok) loadUserTemplates(); }).catch(() => {});
|
}
|
||||||
|
}).catch(() => {
|
||||||
|
if (clone) {
|
||||||
|
Object.assign(_existing, clone);
|
||||||
|
}
|
||||||
|
|
||||||
|
if (showError) {
|
||||||
|
showError(_isInjectStart ? "Something went wrong. Saved prompt has been undone." : "Something went wrong. Saved persona has been undone.");
|
||||||
|
}
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
if (showToast) {
|
if (showToast) {
|
||||||
@@ -883,6 +916,13 @@ export function getAllPresets() {
|
|||||||
return presets;
|
return presets;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Get the in-memory user templates list (may be stale; call loadUserTemplates first if freshness matters).
|
||||||
|
*/
|
||||||
|
export function getUserTemplates() {
|
||||||
|
return [...userTemplates];
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Get the character name (if set)
|
* Get the character name (if set)
|
||||||
*/
|
*/
|
||||||
@@ -1099,6 +1139,7 @@ const presetsModule = {
|
|||||||
getSelectedPreset,
|
getSelectedPreset,
|
||||||
getPreset,
|
getPreset,
|
||||||
getAllPresets,
|
getAllPresets,
|
||||||
|
getUserTemplates,
|
||||||
getCharacterName,
|
getCharacterName,
|
||||||
onSessionSwitch,
|
onSessionSwitch,
|
||||||
isPersistentChat,
|
isPersistentChat,
|
||||||
|
|||||||
@@ -0,0 +1,84 @@
|
|||||||
|
"""Issue #3207 — newly created characters missing from Group participant dropdown.
|
||||||
|
|
||||||
|
The fix has two parts:
|
||||||
|
1. group.js _getCharacterList() merges in-memory userTemplates from presets.js
|
||||||
|
as a fallback (covers the gap while the async templates API save is in-flight).
|
||||||
|
2. presets.js saveCustomPreset() does an optimistic in-memory update of
|
||||||
|
userTemplates immediately on success (bridges the timing race where
|
||||||
|
loadUserTemplates hasn't been triggered yet).
|
||||||
|
|
||||||
|
These tests assert the source patterns exist so they can't be silently removed.
|
||||||
|
"""
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
GROUP_JS = Path("static/js/group.js").read_text(encoding="utf-8")
|
||||||
|
PRESETS_JS = Path("static/js/presets.js").read_text(encoding="utf-8")
|
||||||
|
|
||||||
|
|
||||||
|
# --- group.js: in-memory template merge in _getCharacterList ---
|
||||||
|
|
||||||
|
def test_group_imports_getUserTemplates():
|
||||||
|
"""group.js must import getUserTemplates from presets.js."""
|
||||||
|
assert "getUserTemplates" in GROUP_JS
|
||||||
|
assert "from './presets.js'" in GROUP_JS or 'from "./presets.js"' in GROUP_JS
|
||||||
|
|
||||||
|
|
||||||
|
def test_group_merges_in_memory_templates():
|
||||||
|
"""_getCharacterList must call getUserTemplates() and merge results."""
|
||||||
|
assert "getUserTemplates()" in GROUP_JS
|
||||||
|
# The merge loop should check for duplicates by id
|
||||||
|
assert "!chars.find(c => c.id === t.id)" in GROUP_JS
|
||||||
|
|
||||||
|
|
||||||
|
# --- presets.js: optimistic in-memory update on save ---
|
||||||
|
|
||||||
|
def test_presets_exports_getUserTemplates():
|
||||||
|
"""getUserTemplates must be exported from presets.js."""
|
||||||
|
assert "export function getUserTemplates()" in PRESETS_JS
|
||||||
|
|
||||||
|
|
||||||
|
def test_presets_optimistic_update_on_save():
|
||||||
|
"""saveCustomPreset must update userTemplates in-memory before the async POST."""
|
||||||
|
# Find the optimistic update block
|
||||||
|
assert "Optimistically update the in-memory templates list" in PRESETS_JS
|
||||||
|
# Must push to userTemplates for new entries
|
||||||
|
assert "userTemplates.push(_entry)" in PRESETS_JS
|
||||||
|
# Must Object.assign for existing entries
|
||||||
|
assert "Object.assign(_existing, _entry)" in PRESETS_JS
|
||||||
|
|
||||||
|
|
||||||
|
def test_presets_getUserTemplates_returns_array():
|
||||||
|
"""getUserTemplates should return a shallow copy of userTemplates."""
|
||||||
|
assert "return [...userTemplates]" in PRESETS_JS
|
||||||
|
|
||||||
|
|
||||||
|
def test_presets_optimistic_id_not_empty():
|
||||||
|
"""Optimistic update must generate a client-side id for new characters (not empty string)."""
|
||||||
|
# The id generation uses 'user-' prefix matching server's uuid convention
|
||||||
|
assert "user-' + Math.random" in PRESETS_JS
|
||||||
|
# Must NOT use empty string as fallback (that was the bug)
|
||||||
|
assert "(_existing && _existing.id) || ''" not in PRESETS_JS
|
||||||
|
|
||||||
|
def test_presets_clone_happens_before_mutation():
|
||||||
|
"""Rollback snapshot must be taken before Object.assign mutates _existing."""
|
||||||
|
clone_idx = PRESETS_JS.find("clone = JSON.parse(JSON.stringify(_existing))")
|
||||||
|
assign_idx = PRESETS_JS.find("Object.assign(_existing, _entry)")
|
||||||
|
|
||||||
|
assert clone_idx != -1
|
||||||
|
assert assign_idx != -1
|
||||||
|
assert clone_idx < assign_idx
|
||||||
|
|
||||||
|
def test_presets_rollbak_restores_from_clone():
|
||||||
|
"""Failed save must restore the original object from the pre-mutation clone."""
|
||||||
|
assert "if (clone)" in PRESETS_JS
|
||||||
|
assert "Object.assign(_existing, clone)" in PRESETS_JS
|
||||||
|
|
||||||
|
def test_presets_clone_is_deep_copy():
|
||||||
|
"""Rollback snapshot must be a deep clone, not an alias."""
|
||||||
|
assert "clone = JSON.parse(JSON.stringify(_existing))" in PRESETS_JS
|
||||||
|
|
||||||
|
def test_presets_no_alias_clone():
|
||||||
|
"""Prevent accidental rollback breakage via reference assignment."""
|
||||||
|
assert "clone = _existing" not in PRESETS_JS
|
||||||
|
assert "const clone = _existing" not in PRESETS_JS
|
||||||
|
assert "let clone = _existing" not in PRESETS_JS
|
||||||
Reference in New Issue
Block a user