diff --git a/TESTING.md b/TESTING.md index 214eb50..aa86024 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2624,13 +2624,18 @@ Recorded deviations (plan D60–D70, D74, D75, D79–D82 renumbered D92–D102, ComfyUI and keeps this page open." Host compatibility adds a `p.small[role=status]` with the renderer the open ComfyUI windows report over the bridge (`renderer`: `classic` or `nodes2` on each Prompt Manager destination, - `web/js/prompt-bridge-adapter.js`): "Detected: Nodes 2.0 (1 ComfyUI window).", "… + `web/js/prompt-bridge-adapter.js`; `#renderer-status`): "Detected: Nodes 2.0 (1 ComfyUI window).", "… Classic and Nodes 2.0 (2 ComfyUI windows).", "ComfyUI is open; add a Prompt Manager node to see which renderer it uses." or "No ComfyUI window is open in this browser, so - its renderer is not known."; it looks again when the page becomes visible. "LoraManager + its renderer is not known."; it looks again when the page becomes visible, and only the + newest check paints. The two selects never shrink for the reason lines and, at phone + width, may grow to 160px (the concept caps them at 120px and cuts "Follow host + renderer"), so each shows its chosen option whole (`selectsFit` in the comparison). "LoraManager integration" replaces the concept's sample line with the detection ("Detected: custom_nodes/ComfyUI-Lora-Manager." / "Not detected in ComfyUI’s custom_nodes - folders."); the legacy read-only "LoraManager path" row follows it; "Automatically + folders." / "Found at …, but outside ComfyUI’s custom_nodes folders, so it can’t be + enabled." for a linked install the server would refuse, from `/lora/status`'s + `detected_usable`); the legacy read-only "LoraManager path" row follows it; "Automatically include trigger words" adds "Trigger words are (not) loaded." (the trigger cache, a separate fact). The notes read as the row description (`.setting-row p`). Pinned by `integrations matches the concept` (both themes and sizes) and its mutation check @@ -2656,12 +2661,20 @@ Recorded deviations (plan D60–D70, D74, D75, D79–D82 renumbered D92–D102, - **D114** (Integrations, S10): the CivitAI key field is never filled in (the page never receives the key). Typing drafts a new key, a blank field keeps the saved one (no draft), and "Clear saved key" (shown only while a key is saved) drafts its removal. - A `p.small` says "A key is saved." / "No key saved." / "The new key replaces the saved - one when you save." / "The saved key is removed when you save."; the concept's + A `p.small#civitai-key-status[role=status]` (it also describes the field) says "A key + is saved." / "No key saved." / "The new key replaces the saved one when you save." / + "The saved key is removed when you save."; pressing "Clear saved key" keeps the + keyboard on the key field. A typed key is dropped when the section is left (the field + comes back empty). The field is `autocomplete=new-password`, so browsers never offer it + as a login; the concept's prototype warning is dropped. The LoRA rows save through the one Save row: `POST /lora/enable` with `expected_revision` (a config.json changed elsewhere is `settings-changed`), then `/settings` for any other host change with the revision the - first save returned. Pinned by `key never prefilled; blank keeps; Clear saved key + first save returned (`writeHostInParts`). When only one part is written, the store keeps + what saved, adopts its revision and highlights only the rest: the status is the + server's reason (an unusable source) or "Some changes were saved to the server + configuration; the highlighted ones were not.", and the next Save needs no conflict + (`a save where only the LoRA part is written says so; the next save needs no conflict`). Pinned by `key never prefilled; blank keeps; Clear saved key clears; saved through Save preferences (D114)` and `a LoRA save over settings changed elsewhere is a conflict, never a blind write` (workspace-system-integrations.spec). - **D113** (Sources): the Worker threads range ends at the server's `cpu_count` (the @@ -3106,7 +3119,10 @@ workflow (README changelog). The LoRA import is staged (`py/lora_import.py`): discover the LoraManager `*.metadata.json` files and fingerprint them (path, size, modification time); stage each record (CivitAI example images go into the image cache first, best effort, counted in -`downloaded` / `download_errors`); then `PromptDatabase.replace_category_prompts` +`downloaded` / `download_errors`; each image is written to a temp file beside its cache +entry and moved in place, so a half-written image is never seen as cached); the files are +listed and fingerprinted again (a file changed, removed or added while staging aborts with +nothing written); then `PromptDatabase.replace_category_prompts` (`database/category_import.py`) replaces the `lora-manager` category in one `BEGIN IMMEDIATE` transaction. Any failure leaves the previous prompts, tags and image links exactly as they were; images already downloaded stay in the cache and the job says @@ -3123,11 +3139,18 @@ size, rows, no cache folder, the downloader never called) and answers the mainte /prompt_manager/lora/import/apply {token}` starts the `lora_import` job (202): title "LoRA reference import", admission `records`, cancellable only while it stages, the safety backup the policy promises before the replace. Refusals: `plan-changed` (409, the files -changed since the review), `plan-expired`, `plan-used`, `storage-changed`, `busy`, +changed since the review, or the integration was turned off or moved to another install), +`plan-expired`, `plan-used`, `storage-changed`, `busy`, `insufficient-space`, `apply-failed`. The job fails with `plan-changed` (the files or the imports changed meanwhile), `blocked-by-pairs`, `nothing-to-import`, `backup-failed` or -`import-failed`, each with nothing written. The legacy `POST /lora/scan` runs the same -stages under its SSE event shape (`test_legacy_scan_events_unchanged_on_success`), holds +`import-failed`, each with nothing written; the job keeps what staging skipped, by name, +whatever happened after. The tests change real files behind a held download +(`test_files_changed_while_staging_fail_the_job`, removed and added files too). The +replace binds the category, not one parameter per prompt, so SQLite's variable limit +never caps a collection. The legacy `POST /lora/scan` runs the same stages under its SSE +event shape; a page that closes its connection stops staging before its next LoRA, and the +scan keeps its records hold until staging has stopped +(`test_a_disconnected_legacy_scan_stops_and_holds_until_it_stopped`) (`test_legacy_scan_events_unchanged_on_success`), holds the registry as "LoRA import (admin page)" and takes no safety backup. `POST /lora/enable` accepts the optional `expected_revision`. LoraManager detection also searches every `custom_nodes` folder ComfyUI registers through `folder_paths` (#144), main folder first. diff --git a/tests/browser/system-integrations-concept.spec.mjs b/tests/browser/system-integrations-concept.spec.mjs index 1ba0752..3990e72 100644 --- a/tests/browser/system-integrations-concept.spec.mjs +++ b/tests/browser/system-integrations-concept.spec.mjs @@ -71,6 +71,20 @@ function measureIntegrations(mainSelector) { // The concept's `.small` text (its Save row status); production's row notes use the same class. small: css(layout.querySelector('#settings-status'), 'fontSize', 'color'), note: note && css(note, 'fontSize', 'color'), + // Whether each select shows its whole chosen option (its text within the box, less the native arrow). + // Whether each select is as wide as the browser lays out a copy holding only its chosen option, i.e. it shows + // that option whole (the browser's own select sizing, arrow included). + selectsFit: [...layout.querySelectorAll('.setting-row > select')].map(select => { + const copy = select.cloneNode(false); + copy.removeAttribute('id'); + copy.append(new Option(select.selectedOptions[0]?.textContent ?? '')); + Object.assign(copy.style, { position: 'absolute', visibility: 'hidden', width: 'max-content', minWidth: '0', + maxWidth: 'none', flex: 'none' }); + select.parentElement.append(copy); + const needed = copy.getBoundingClientRect().width; + copy.remove(); + return select.getBoundingClientRect().width + 0.5 >= needed; + }), pageScroll: document.documentElement.scrollWidth > document.documentElement.clientWidth, }; } @@ -122,6 +136,10 @@ function mapIntegrationsConcept(reference) { // D107: the import is reviewed first ("Review import"; the concept's button only simulated a job). operations: lora.operations.map(operation => ({ ...operation, actions: [['Review import', '']] })) }, ], + // D106: both selects show their whole chosen option at every width (at 390 the concept cuts "Follow host + // renderer"; production lets the two selects grow to 160px and never shrink for the reason lines). + selectsFit: reference.selectsFit.map(() => true), + select: reference.select.maxWidth === '120px' ? { ...reference.select, maxWidth: '160px' } : reference.select, // The D106/D114 row notes are `p.small` inside the row text, so they read as its description (`.setting-row p`). note: { fontSize: reference.description.fontSize, color: reference.description.color }, }; @@ -139,7 +157,7 @@ async function productionIntegrations(page, theme) { await openProduction(page, 'system/integrations', theme); // Compare once the LoRA state and the renderer check answered. await expect(page.locator('#workspace-main #lora-path')).toHaveValue('custom_nodes/ComfyUI-Lora-Manager'); - await expect(page.locator('#workspace-main .setting-row [role="status"]')).toHaveText(RENDERER_NONE); + await expect(page.locator('#workspace-main #renderer-status')).toHaveText(RENDERER_NONE); await page.mouse.move(0, 0); return page.evaluate(measureIntegrations, '#workspace-main'); } @@ -190,6 +208,10 @@ test('integrations comparison detects mutations', async ({ page, request }) => { await page.evaluate(() => { document.querySelectorAll('#workspace-main #renderer option')[1].disabled = false; }); expect(await paths()).toEqual(['surfaces.0.rows.1.control.options.1.1']); await page.evaluate(() => { document.querySelectorAll('#workspace-main #renderer option')[1].disabled = true; }); + // A select squeezed until its chosen option is cut (S10 review M3). + const undoSqueeze = await mutate(page, { css: '#workspace-main #renderer { max-width: 70px !important; }' }); + expect(await paths()).toEqual(['selectsFit.1']); + await undoSqueeze(); // A swapped token. const undoToken = await mutate(page, { css: '#workspace-main .settings-layout { --line: rgb(255, 0, 0); }' }); expect(await paths()).toEqual(expect.arrayContaining(['row.borderTopColor'])); @@ -265,7 +287,7 @@ async function productionReview(page, request, theme) { await openProduction(page, 'system/integrations', theme); await expect(page.locator('#workspace-main #lora-path')).toHaveValue('custom_nodes/ComfyUI-Lora-Manager'); // The renderer line above settles first: its new text moves the rows below (a click must not land mid-shift). - await expect(page.locator('#workspace-main .setting-row [role="status"]')).toHaveText(RENDERER_NONE); + await expect(page.locator('#workspace-main #renderer-status')).toHaveText(RENDERER_NONE); // The import reviews what is enabled: save the integration on first. await page.locator('#workspace-main #lora-enable').check(); await page.getByRole('button', { name: 'Save preferences', exact: true }).click(); diff --git a/tests/browser/workspace-system-integrations.spec.mjs b/tests/browser/workspace-system-integrations.spec.mjs index ace020a..faf78e1 100644 --- a/tests/browser/workspace-system-integrations.spec.mjs +++ b/tests/browser/workspace-system-integrations.spec.mjs @@ -190,6 +190,13 @@ test('key never prefilled; blank keeps; Clear saved key clears; saved through Sa await expect(field).toHaveValue(''); await expect(key.locator('p.small')).toHaveText('No key saved.'); await expect(clear).toHaveCount(0); + // A typed key is dropped when the section is left: the field comes back empty with nothing hidden to save (N4). + await field.fill('sk-dropped'); + await expect(status(page)).toHaveText('Unsaved changes.'); + await page.locator('#workspace-main .section-nav').getByRole('button', { name: 'Appearance', exact: true }).click(); + await page.locator('#workspace-main .section-nav').getByRole('button', { name: 'Integrations', exact: true }).click(); + await expect(status(page)).toHaveText('No unsaved changes.'); + await expect(key.locator('p.small')).toHaveText('No key saved.'); // Typing a key drafts it; Save sends it once through /lora/enable with the settings revision. await field.fill('sk-test-key'); await expect(key.locator('p.small')).toHaveText('The new key replaces the saved one when you save.'); @@ -215,10 +222,15 @@ test('key never prefilled; blank keeps; Clear saved key clears; saved through Sa await field.fill(' '); await expect(status(page)).toHaveText('No unsaved changes.'); await field.fill(''); - // Clear saved key drafts the removal; Save removes it. - await clear.click(); + // Clear saved key drafts the removal; Save removes it. The keyboard stays on the key field, and the line that + // says so is announced (S10 review M2). + await expect(key.locator('p.small')).toHaveAttribute('role', 'status'); + await expect(field).toHaveAttribute('aria-describedby', 'civitai-key-status'); + await clear.focus(); + await page.keyboard.press('Enter'); await expect(key.locator('p.small')).toHaveText('The saved key is removed when you save.'); await expect(clear).toHaveCount(0); + await expect(field).toBeFocused(); await save(page).click(); await expect(status(page)).toHaveText('Saved to the server configuration.'); expect(enables.at(-1)).toMatchObject({ clear_civitai_api_key: true }); @@ -242,6 +254,37 @@ test('a LoRA save over settings changed elsewhere is a conflict, never a blind w expect((await serverJson(request, '/__test__/integrations/state')).trigger_words).toBe(true); }); +test('a save where only the LoRA part is written says so; the next save needs no conflict (S10 review I3)', + async ({ page, request }) => { + await lora(request); + await expect(await request.post('/__test__/system/missing-source', { data: { inside: true } })).toBeOK(); + await openIntegrations(page); + const triggers = settingOf(page, 'Automatically include trigger words').locator('input'); + await triggers.check(); + // In Sources, the drive is unplugged after the page read the list, and another source is removed. + const nav = page.locator('#workspace-main .section-nav'); + await nav.getByRole('button', { name: 'Sources & scans', exact: true }).click(); + const sourceRow = title => layoutOf(page).locator('#source-list .operation').filter({ has: page.locator('h3', { hasText: title }) }); + await expect(sourceRow('unplugged')).toBeVisible(); + await expect(await request.post('/__test__/system/unplug')).toBeOK(); + await sourceRow('ComfyUI input').getByRole('button', { name: 'Remove', exact: true }).click(); + await save(page).click(); + // The LoRA part was saved, the sources were refused: the status gives the server's reason for the source, and + // only the source row is highlighted. + await expect(status(page)).toHaveText("output/unplugged can't be used: Gallery root does not exist. Remove it or reconnect it, then save."); + await expect(sourceRow('unplugged')).toHaveAttribute('aria-invalid', 'true'); + expect((await serverJson(request, '/__test__/integrations/state')).trigger_words).toBe(true); + await nav.getByRole('button', { name: 'Integrations', exact: true }).click(); + await expect(triggers).toBeChecked(); + await expect(triggers).not.toHaveAttribute('aria-invalid', 'true'); + // The next save carries the revision the saved part left: it saves, with no "changed elsewhere". + await nav.getByRole('button', { name: 'Sources & scans', exact: true }).click(); + await sourceRow('unplugged').getByRole('button', { name: 'Remove', exact: true }).click(); + await save(page).click(); + await expect(status(page)).toHaveText('Saved to the server configuration.'); + expect((await serverJson(request, '/prompt_manager/settings')).sources.configured.map(source => source.path)).toEqual(['output']); + }); + test('Review import shows replacement, blocked items and network effects with the backup box; Start opens the job (D107)', async ({ page, request }) => { await lora(request, { loras: 3, civitai: true, previous: 2 }); @@ -341,7 +384,11 @@ test('the import can be cancelled while it downloads; then nothing is written', await expect.poll(async () => (await serverJson(request, '/__test__/integrations/state')).downloads).toBeGreaterThan(0); const cancel = page.locator('#workspace-main').getByRole('button', { name: 'Cancel import', exact: true }); await expect(cancel).toBeVisible(); - await cancel.click(); + // The cancel must have reached the server before the held download goes on (S10 review M1). + await Promise.all([ + page.waitForResponse(response => /\/prompt_manager\/jobs\/[0-9a-f]+\/cancel$/.test(response.url()) && response.ok()), + cancel.click(), + ]); await expect(await request.post('/__test__/integrations/gate', { data: { hold: false } })).toBeOK(); const job = await ended(request, id); expect(job.state).toBe('cancelled'); diff --git a/tests/comfyui/prompt-bridge.spec.mjs b/tests/comfyui/prompt-bridge.spec.mjs index b936409..95a7455 100644 --- a/tests/comfyui/prompt-bridge.spec.mjs +++ b/tests/comfyui/prompt-bridge.spec.mjs @@ -196,7 +196,7 @@ test('each destination reports its window renderer, and System › Integrations const id=await addNode(page,'PromptManager',{text:'Renderer check'}); await withClient(page,context,async clientPage=>{ const name=renderer=>renderer?'Nodes 2.0':'Classic'; - const status=clientPage.locator('#workspace-main .setting-row [role="status"]'); + const status=clientPage.locator('#workspace-main #renderer-status'); expect((await destination(clientPage,id)).target.renderer).toBe(initial?'nodes2':'classic'); await clientPage.evaluate(()=>{location.hash='#/system/integrations';}); await expect(status).toHaveText(`Detected: ${name(initial)} (1 ComfyUI window).`); diff --git a/tests/js/workspace-system-integrations.test.js b/tests/js/workspace-system-integrations.test.js index 881829e..d8eac71 100644 --- a/tests/js/workspace-system-integrations.test.js +++ b/tests/js/workspace-system-integrations.test.js @@ -36,7 +36,7 @@ test('LoRA status becomes host values: drafted flags and read-only facts kept ap has_civitai_api_key: true, detected: true, detected_path: 'custom_nodes/ComfyUI-Lora-Manager', trigger_cache_loaded: true }), { lora_enabled: true, lora_trigger_words_enabled: false, clear_civitai_api_key: false, lora_path: 'custom_nodes/ComfyUI-Lora-Manager', lora_detected: true, lora_detected_path: 'custom_nodes/ComfyUI-Lora-Manager', - lora_has_key: true, lora_cache_loaded: true, + lora_detected_usable: true, lora_has_key: true, lora_cache_loaded: true, }); const empty = loraValues({}); assert.equal(empty.lora_enabled, false); @@ -84,6 +84,10 @@ test('LoRA rows show detected / enabled / cache-loaded separately', async () => assert.equal(detectionText({ lora_detected: true, lora_detected_path: '' }), 'Detected.'); assert.equal(detectionText({ lora_detected: false }), 'Not detected in ComfyUI’s custom_nodes folders.'); assert.equal(detectionText({}), null); + // S10 review N5: a linked install outside custom_nodes is found but can't be enabled. + assert.equal(detectionText({ lora_detected: true, lora_detected_path: 'LoRA-Manager', lora_detected_usable: false }), + 'Found at LoRA-Manager, but outside ComfyUI’s custom_nodes folders, so it can’t be enabled.'); + assert.equal((await integrations).loraValues({ detected: true, detected_usable: false }).lora_detected_usable, false); assert.equal(cacheText({ lora_cache_loaded: true }), 'Trigger words are loaded.'); assert.equal(cacheText({ lora_cache_loaded: false }), 'Trigger words are not loaded.'); assert.equal(cacheText({}), null); @@ -152,3 +156,42 @@ test('a failed import the files or saved pairs overtook is reviewed again in Int assert.equal(reviewAgainOf(failed('import-failed')), null); assert.equal(reviewAgainOf({ ...failed('plan-changed'), state: 'cancelled' }), null); }); + +test('a host save in two parts reports exactly which part saved (S10 review I3)', async () => { + const { writeHostInParts } = await integrations; + const saved = { lora_enabled: true, lora_trigger_words_enabled: false, lora_path: 'custom_nodes/LM' }; + const calls = []; + const ok = revision => async body => { calls.push(body); return { success: true, persisted: true, revision }; }; + const refuse = (code, message) => async body => { calls.push(body); throw Object.assign(new Error(message), { code }); }; + const run = (changes, postLora, postSettings) => writeHostInParts({ changes, saved, expected_revision: 'r0', postLora, postSettings }); + + // Both parts: the second carries the revision the first returned. + assert.equal((await run({ lora_trigger_words_enabled: true, result_timeout: 5 }, ok('r1'), ok('r2'))).revision, 'r2'); + assert.deepEqual(calls.map(body => body.expected_revision), ['r0', 'r1']); + // LoRA saved, settings refused: the LoRA keys and the new revision ride on the settings error. + calls.length = 0; + const later = await run({ lora_trigger_words_enabled: true, gallery_root_paths: ['x'] }, ok('r1'), + refuse('invalid-source', 'x cannot be used')).catch(error => error); + assert.equal(later.code, 'invalid-source'); + assert.deepEqual(later.partial, { saved: ['lora_trigger_words_enabled'], revision: 'r1' }); + // LoRA refused (not detected), settings still sent with the unchanged revision and saved. + calls.length = 0; + const first = await run({ lora_enabled: true, result_timeout: 5 }, refuse(null, 'LoraManager not found'), ok('r2')) + .catch(error => error); + assert.equal(first.message, 'LoraManager not found'); + assert.deepEqual(first.partial, { saved: ['result_timeout'], revision: 'r2' }); + assert.deepEqual(calls.map(body => body.expected_revision), ['r0', 'r0']); + // A conflict on the first write stops there; a lone part fails as itself. + calls.length = 0; + const conflict = await run({ lora_enabled: false, result_timeout: 5 }, refuse('settings-changed', 'changed'), ok('r2')) + .catch(error => error); + assert.equal(conflict.code, 'settings-changed'); + assert.equal(conflict.partial, undefined); + assert.equal(calls.length, 1); + const alone = await run({ result_timeout: 5 }, ok('r1'), refuse('busy', 'scan running')).catch(error => error); + assert.equal(alone.code, 'busy'); + assert.equal(alone.partial, undefined); + const both = await run({ lora_enabled: true, result_timeout: 5 }, refuse(null, 'a'), refuse(null, 'b')).catch(error => error); + assert.equal(both.message, 'b'); + assert.equal(both.partial, undefined); +}); diff --git a/tests/js/workspace-system-state.test.js b/tests/js/workspace-system-state.test.js index f7e611e..130bc67 100644 --- a/tests/js/workspace-system-state.test.js +++ b/tests/js/workspace-system-state.test.js @@ -190,6 +190,7 @@ test('status text per outcome', async () => { host: 'Saved to the server configuration.', runtime: 'Applied until the server restarts.', partial: 'Some changes were not saved. Review the highlighted settings.', + partialSaved: 'Some changes were saved to the server configuration; the highlighted ones were not.', tabOnly: 'Saved for this tab only; browser storage is unavailable.', conflict: 'Settings changed elsewhere. Reload this section to see them.', restart: 'Restart ComfyUI to apply:', @@ -364,3 +365,42 @@ test('a host save refused for one source says the server reason and names it unt assert.equal(drafts.invalidSource(), null); assert.equal(drafts.statusText(), 'Unsaved changes.'); }); + +test('a partial host save keeps what saved, adopts its revision and fails only the rest (S10 review I3)', async () => { + const { SETTINGS_STATUS } = await source; + const domains = adapters({ host: { lora_trigger_words_enabled: false, gallery_root_paths: ['a'] } }); + const drafts = await loaded(domains); + // The LoRA part is written (config.json moves to sha256:lora), then /settings refuses one source. + domains.outcome.host = () => { + throw Object.assign(new Error('missing can\'t be used: not found. Remove it or reconnect it, then save.'), { + code: 'invalid-source', source: 'missing', partial: { saved: ['lora_trigger_words_enabled'], revision: 'sha256:lora' } }); + }; + drafts.set('host', 'lora_trigger_words_enabled', true); + drafts.set('host', 'gallery_root_paths', ['a', 'missing']); + const first = await drafts.save(); + assert.equal(first.host, 'partial'); + assert.deepEqual([...drafts.failed('host')], ['gallery_root_paths']); + assert.equal(drafts.saved('host').lora_trigger_words_enabled, true); + assert.deepEqual(drafts.changes('host'), { gallery_root_paths: ['a', 'missing'] }); + assert.equal(drafts.invalidSource(), 'missing'); + assert.equal(drafts.statusText(), 'missing can\'t be used: not found. Remove it or reconnect it, then save.'); + // The next save carries the revision the saved part produced: no false "changed elsewhere". + domains.outcome.host = null; + drafts.set('host', 'gallery_root_paths', ['a']); + const second = await drafts.save(); + assert.equal(second.host, 'saved'); + assert.equal(domains.calls.at(-1)[2].expected_revision, 'sha256:lora'); + assert.deepEqual(domains.calls.at(-1)[1], { gallery_root_paths: ['a'] }); + // Without a server reason, the status says some were saved and the highlighted ones were not. + domains.outcome.host = () => { throw Object.assign(new Error('boom'), { partial: { saved: ['x'], revision: 'sha256:three' } }); }; + drafts.set('host', 'x', 1); + drafts.set('host', 'y', 2); + assert.equal((await drafts.save()).host, 'partial'); + assert.equal(drafts.statusText(), SETTINGS_STATUS.partialSaved); + assert.deepEqual([...drafts.failed('host')], ['y']); + // A partial result that saved nothing is a plain failure. + domains.outcome.host = () => { throw Object.assign(new Error('boom'), { partial: { saved: [] } }); }; + drafts.set('host', 'z', 3); + assert.equal((await drafts.save()).host, 'failed'); + assert.equal(drafts.statusText(), SETTINGS_STATUS.partial); +}); diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index ceb6869..09d8f8b 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -505,7 +505,7 @@ }, "workspace/system-integrations.mjs": { "lines": 96, - "branches": 86, + "branches": 88, "functions": 91, "statements": 96 }, diff --git a/web/js/workspace/system-integrations.mjs b/web/js/workspace/system-integrations.mjs index e8333a7..1313d5d 100644 --- a/web/js/workspace/system-integrations.mjs +++ b/web/js/workspace/system-integrations.mjs @@ -74,6 +74,7 @@ export function loraValues(status) { lora_path: typeof status?.path === 'string' ? status.path : '', lora_detected: status?.detected === true, lora_detected_path: typeof status?.detected_path === 'string' ? status.detected_path : '', + lora_detected_usable: status?.detected_usable !== false, lora_has_key: status?.has_civitai_api_key === true, lora_cache_loaded: status?.trigger_cache_loaded === true, }; @@ -100,6 +101,45 @@ export function loraPayload(changes, saved) { return body; } +/** + * Write host `changes` in their two parts (S10 review I3): the LoRA keys through `postLora` (`/lora/enable`), then the + * other keys through `postSettings` (`/settings`), each with the revision the previous write left. Resolves with the + * last answer when everything saved. When only one part saved, rejects with the other part's error carrying + * `partial: {saved: [keys], revision}`, so the store keeps what saved and adopts the new revision; a + * conflict on the first write (another page changed config.json) stops before the second. + */ +export async function writeHostInParts({ changes, saved, expected_revision, postLora, postSettings }) { + const { lora, settings } = splitHostChanges(changes); + let revision = expected_revision; + let result = null; + const written = []; + let refused = null; + if (lora) { + try { + result = await postLora({ ...loraPayload(lora, saved), expected_revision: revision }); + if (typeof result?.revision === 'string') revision = result.revision; + written.push(...Object.keys(lora)); + } catch (error) { + if (!settings || error?.code === 'settings-changed') throw error; + refused = error; + } + } + if (settings) { + try { + result = await postSettings({ ...settings, expected_revision: revision }); + if (typeof result?.revision === 'string') revision = result.revision; + written.push(...Object.keys(settings)); + } catch (error) { + if (!written.length) throw error; + throw Object.assign(error, { partial: { saved: written, revision } }); + } + } + if (refused) { + throw Object.assign(refused, { partial: { saved: written, revision } }); + } + return result; +} + /** The renderer line for discovered bridge hosts (D106). */ export function rendererText(hosts) { if (!Array.isArray(hosts) || hosts.length === 0) return RENDERER_COPY.none; @@ -114,6 +154,10 @@ export function rendererText(hosts) { export function detectionText(values) { if (values.lora_detected === undefined) return null; if (!values.lora_detected) return 'Not detected in ComfyUI’s custom_nodes folders.'; + if (values.lora_detected_usable === false) { + // A linked install outside custom_nodes is found but cannot be enabled (S10 review N5). + return `Found at ${values.lora_detected_path || 'a linked folder'}, but outside ComfyUI’s custom_nodes folders, so it can’t be enabled.`; + } return values.lora_detected_path ? `Detected: ${values.lora_detected_path}.` : 'Detected.'; } @@ -260,17 +304,23 @@ function comfySurface({ scope, clientFactory }) { setNote(returnRow, REASONS.returnCanvas); const status = el('p', RENDERER_COPY.looking, 'small'); status.setAttribute('role', 'status'); + status.id = 'renderer-status'; renderer.children[0].append(el('p', REASONS.renderer, 'small'), status); surface.append(send, renderer, returnRow); let client = null; + // Overlapping checks (two quick visibility changes) paint only the newest answer (S10 review N2). + let latest = 0; const detect = async () => { + const check = ++latest; + let text; try { client ??= clientFactory(); - status.textContent = rendererText(await client.discover({ waitMs: 300 })); + text = rendererText(await client.discover({ waitMs: 300 })); } catch { - if (!scope.signal.aborted) status.textContent = RENDERER_COPY.unavailable; + text = RENDERER_COPY.unavailable; } + if (check === latest && !scope.signal.aborted) status.textContent = text; }; scope.cleanup(() => client?.dispose()); // Another tab may open or switch ComfyUI meanwhile: look again when this page is shown again. @@ -309,8 +359,16 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli const key = settingRow({ id: 'civitai-key', label: 'CivitAI API key', description: 'Credentials stay in the server configuration.', control: { type: 'password' } }); const keyInput = controlOf(key, 'civitai-key'); - keyInput.autocomplete = 'off'; + // Never a login the browser offers to save or fill (S10 review N3). + keyInput.autocomplete = 'new-password'; + keyInput.spellcheck = false; keyInput.placeholder = 'Optional'; + // D114: the saved-key line is announced (a polite live region) and describes the field (S10 review M2). + const keyNote = el('p', '', 'small'); + keyNote.id = 'civitai-key-status'; + keyNote.setAttribute('role', 'status'); + key.children[0].append(keyNote); + keyInput.setAttribute('aria-describedby', keyNote.id); const clear = createButton('Clear saved key'); clear.dataset.setting = 'clear_civitai_api_key'; clear.dataset.key = 'clear_civitai_api_key'; @@ -324,6 +382,8 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli }); scope.listen(clear, 'click', () => { keyInput.value = ''; + // The button leaves with the saved key: keep the keyboard on the field it belongs to (S10 review M2). + keyInput.focus(); drafts.set('host', 'civitai_api_key', undefined); drafts.set('host', 'clear_civitai_api_key', true); }); @@ -354,10 +414,13 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli pathInput.value = values.lora_path || values.lora_detected_path || ''; setNote(triggers, cacheText(values)); keyInput.disabled = unread; - setNote(key, keyText(values, changes)); + keyNote.textContent = keyText(values, changes) ?? ''; // D114: "Clear saved key" is offered only while a saved key would stay. if (values.lora_has_key === true && changes.clear_civitai_api_key !== true) key.children[0].append(keyActions); - else keyActions.remove(); + else { + if (keyActions.contains(document.activeElement)) keyInput.focus(); + keyActions.remove(); + } const state = JSON.stringify([values.lora_enabled, values.lora_trigger_words_enabled, values.civitai_api_key, values.clear_civitai_api_key]); const saved = values.civitai_api_key !== undefined || values.clear_civitai_api_key === true; @@ -370,6 +433,9 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli if (!unread) shown = state; }; scope.cleanup(drafts.subscribe(repaint)); + // A typed key is held only while its field shows it: leaving the section drops it, since the field comes back + // empty and a hidden key draft could not be seen or discarded (S10 review N4). + scope.cleanup(() => drafts.set('host', 'civitai_api_key', undefined)); repaint(); return [comfy, surface]; } diff --git a/web/js/workspace/system-state.mjs b/web/js/workspace/system-state.mjs index a0d29ff..10c82c1 100644 --- a/web/js/workspace/system-state.mjs +++ b/web/js/workspace/system-state.mjs @@ -19,6 +19,8 @@ * 'settings-changed'` when another page changed a drafted value (a conflict: the browser values are read again), * `writeHost(changes, {expected_revision})` the settings response (`{success, persisted, revision}`, and the * `restart_required_fields` the status names), `writeRuntime(changes)` anything; a write that throws failed. A host + * write that saved part of its changes throws with `partial: {saved: [keys], revision}`: those keys are saved, the + * revision is adopted and only the rest fail (`partial`, S10). A host * write refused as `invalid-source` (its error's `source` and server message) makes that message the status and * `invalidSource()` that source, so Sources marks its row (S4). */ @@ -32,6 +34,8 @@ export const SETTINGS_STATUS = Object.freeze({ host: 'Saved to the server configuration.', runtime: 'Applied until the server restarts.', partial: 'Some changes were not saved. Review the highlighted settings.', + // S10 review I3: a host save written in two parts where only one was saved. + partialSaved: 'Some changes were saved to the server configuration; the highlighted ones were not.', tabOnly: 'Saved for this tab only; browser storage is unavailable.', conflict: 'Settings changed elsewhere. Reload this section to see them.', // D128: a saved setting the running server applies only after a restart. @@ -55,6 +59,7 @@ export function statusText(outcome) { const results = DRAFT_DOMAINS.map(domain => outcome?.[domain]); if (results.includes('conflict')) return SETTINGS_STATUS.conflict; if (results.includes('failed')) return outcome?.invalid?.message || SETTINGS_STATUS.partial; + if (results.includes('partial')) return outcome?.invalid?.message || SETTINGS_STATUS.partialSaved; const saved = []; if (outcome?.browser === 'saved') saved.push(SETTINGS_STATUS.browser); if (outcome?.browser === 'tab-only') saved.push(SETTINGS_STATUS.tabOnly); @@ -177,10 +182,21 @@ export function createSystemDrafts(adapters) { commit(domain, written); return 'applied'; } catch (error) { - fail(domain, written); if (domain === 'host' && error?.code === INVALID_SOURCE_CODE && typeof error.message === 'string') { hostDetail.invalid = { source: typeof error.source === 'string' ? error.source : null, message: error.message }; } + const partial = domain === 'host' ? error?.partial : null; + if (partial && Array.isArray(partial.saved)) { + // S10 review I3: part of the host save was written (config.json moved on): keep what saved, adopt the + // revision it produced, and fail only the rest, so the next save is not a false conflict. + if (typeof partial.revision === 'string') revision = partial.revision; + const saved = new Map([...written].filter(([key]) => partial.saved.includes(key))); + commit(domain, saved); + fail(domain, new Map([...written].filter(([key]) => !saved.has(key)))); + if (error?.code === CONFLICT_CODE) return 'conflict'; + return saved.size ? 'partial' : 'failed'; + } + fail(domain, written); return error?.code === CONFLICT_CODE ? 'conflict' : 'failed'; } } diff --git a/web/js/workspace/system.mjs b/web/js/workspace/system.mjs index bcf45cf..ccbf1e5 100644 --- a/web/js/workspace/system.mjs +++ b/web/js/workspace/system.mjs @@ -19,7 +19,7 @@ import { mountOverview } from './system-overview.mjs'; import { mountAppearance } from './system-appearance.mjs'; import { mountSources } from './system-sources.mjs'; import { mountMaintenance } from './system-maintenance.mjs'; -import { LORA_ENABLE_URL, LORA_STATUS_URL, loraPayload, loraValues, mountIntegrations, splitHostChanges } from './system-integrations.mjs'; +import { LORA_ENABLE_URL, LORA_STATUS_URL, loraValues, mountIntegrations, writeHostInParts } from './system-integrations.mjs'; import { busyToast } from './activity-scan.mjs'; import { showWorkspaceToast } from './toast.mjs'; import { displayPreferences, readPreferences, reconcilePreferences, subscribePreferences, writePreferences } from './preferences.mjs'; @@ -88,18 +88,11 @@ export function systemDrafts(api) { revision: result.revision }; }, async writeHost(changes, { expected_revision }) { - // S10: the LoRA keys save through /lora/enable first, then the rest through /settings with the revision that - // save returned; both refuse a config.json changed since it was read. - const { lora, settings } = splitHostChanges(changes); - let revision = expected_revision; - let result = null; + // S10: the LoRA keys save through /lora/enable, the rest through /settings, each with the current revision; + // a save where only one part was written reports which (writeHostInParts, review I3). try { - if (lora) { - result = await api.post(LORA_ENABLE_URL, { ...loraPayload(lora, drafts.saved('host')), expected_revision: revision }); - revision = result?.revision ?? revision; - } - if (settings) result = await api.post(SETTINGS_URL, { ...settings, expected_revision: revision }); - return result; + return await writeHostInParts({ changes, saved: drafts.saved('host'), expected_revision, + postLora: body => api.post(LORA_ENABLE_URL, body), postSettings: body => api.post(SETTINGS_URL, body) }); } catch (error) { // A sources change waits for a running record-changing job (S4): name it, as a refused start does (D57). if (error?.code === 'busy') showWorkspaceToast(busyToast(await runningJob(error))); diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index 27c27e7..1739ceb 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -423,3 +423,9 @@ pre.prompt-text { font: inherit; font-size: 12px; line-height: 1.75; margin-bloc /* D101: a duplicate copy's Keep label is its source and path, often long names with no space to break at; it wraps inside its `.compare-item` so the review never scrolls sideways. */ .duplicate-groups .compare-item label { overflow-wrap: anywhere; /* D101 */ } +/* D106: Integrations' two selects keep their chosen option readable: they never shrink for the row's reason lines, and + at phone width they may grow past the concept's 120px cap (which cuts "Follow host renderer") up to 160px. */ +.setting-row :is(#send-destination, #renderer) { flex-shrink: 0; /* D106 */ } +@media (max-width: 700px) { + #workspace-main .setting-row :is(#send-destination, #renderer) { max-width: 160px !important; /* D106 */ } +}