fix(system): flag changes made while System was closed and keep embedded galleries fixed

Final review fixes for System S3 (Appearance):

- I1: the on-demand metadata deviation is D124 (TESTING.md, inspector
  markers); TESTING.md names the spec that pins it, since no concept
  comparison renders that state.
- I2: System checks the display defaults for conflicts on mount, not only
  on storage events, and a browser write refuses a draft another page
  overtook, so a draft is never saved over another tab's value without
  the conflict status first. New two-tab spec.
- I3: the layout and size defaults start the Images page only; embedded
  galleries (a prompt history's Shared gallery and dialogs) keep
  Grid/Comfortable.
- M1: host rows whose /settings read failed say "Server settings could
  not be read."
- M2: a rejected timeout shows the "Some changes were not saved" status.
- M3: the browser loads on mount and on storage events never reject
  unhandled.
- M4: the next accepted write stores the tab-only changes and clears the
  overlay.
- M5: cache off never re-reads a source-changed selection, and refresh()
  never loads a deferred one.
- M6: the System fixture refuses a config.json outside its temp root and
  restores it with os.replace.
- M7: the inspection read counts wait on a request barrier, not frames.
- M8 recorded as an open saved-views question in TESTING.md.

tests/browser/workspace-system.spec.mjs

@@ -11,7 +11,7 @@ const number = value => value.toLocaleString('en-US');
 
 test.beforeEach(async ({ page, request, browserName }) => {
     await startCoverage(page, browserName);
-    await seedConcept(request);
+    page.manifest = await seedConcept(request);
     await expect(await request.post('/__test__/system/reset')).toBeOK();
     await expect(await request.post('/__test__/jobs/restart')).toBeOK();
     page.systemErrors = [];
@@ -279,6 +279,14 @@ async function toggle(page, values) {
     for (const [id, checked] of Object.entries(values)) await page.locator(`#${id}`).setChecked(checked, { force: true });
 }
 
+/**
+ * A request barrier: one request issued and answered after everything the page has asked for so far, so a read the
+ * last action started has been seen (Playwright reports a page's requests in order).
+ */
+async function requestBarrier(page) {
+    expect(await page.evaluate(url => fetch(url, { cache: 'no-store' }).then(response => response.ok), SETTINGS_URL)).toBe(true);
+}
+
 async function openImages(page) {
     await page.goto(`${workspace}#/images`);
     await expect(imageCards(page).first()).toBeVisible();
@@ -324,6 +332,23 @@ test('defaults apply to a new visit but not to the current view or a saved view'
     }
 });
 
+test('the defaults reach the Images page only; embedded galleries keep their own presentation', async ({ page }) => {
+    await openAppearance(page);
+    await page.locator('#default-view').selectOption('list');
+    await page.locator('#density').selectOption('compact');
+    await saveAppearance(page, 'Saved in this browser.');
+    // The prompt history's shared gallery has no toolbar to change them, so it opens as it always did.
+    await page.goto(`${workspace}#/library/${page.manifest.prompts[0].positive_id}`);
+    await expect(imageCards(page).first()).toBeVisible();
+    await expect(page.locator('#workspace-main .image-grid')).toHaveAttribute('data-layout', 'grid');
+    await expect(page.locator('#workspace-main .image-grid')).toHaveAttribute('data-density', 'comfortable');
+    // The Images page starts from them.
+    await page.locator('#primary-nav a[data-route="images"]').click();
+    await expect(imageCards(page).first()).toBeVisible();
+    await expect(page.locator('.image-grid')).toHaveAttribute('data-layout', 'list');
+    await expect(page.locator('.image-grid')).toHaveAttribute('data-density', 'compact');
+});
+
 test('metadata off shows an explicit load and loads once on request', async ({ page }) => {
     await openAppearance(page);
     await toggle(page, { 'metadata-auto': false });
@@ -343,7 +368,7 @@ test('metadata off shows an explicit load and loads once on request', async ({ p
     // The same file again reads nothing more (the cache is on).
     await first.locator('.image-card-actions button').first().click();
     await expect(panel(page).locator('.panel-heading h2')).toBeVisible();
-    await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve))));
+    await requestBarrier(page);
     expect(reads).toHaveLength(1);
     // The viewer's details use the same inspector: another file waits there too, and loads on request.
     await imageCards(page).nth(1).locator('.image-open').click();
@@ -358,7 +383,6 @@ test('metadata off shows an explicit load and loads once on request', async ({ p
 test('cache off rereads the current selection; on reuses the one current record', async ({ page }) => {
     const reads = [];
     page.on('request', item => { if (new URL(item.url()).pathname.startsWith(INSPECT_PATH)) reads.push(item.url()); });
-    const settle = () => page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve))));
     for (const cache of [true, false]) {
         if (!cache) {
             await openAppearance(page);
@@ -371,7 +395,7 @@ test('cache off rereads the current selection; on reuses the one current record'
         await info.click();
         await expect(panel(page).locator('.panel-heading h2')).toBeVisible();
         await expect.poll(() => reads.length).toBeGreaterThan(0);
-        await settle();
+        await requestBarrier(page);
         reads.length = 0;
         // Inspect the selected file twice more.
         for (const _ of [1, 2]) {
@@ -380,7 +404,7 @@ test('cache off rereads the current selection; on reuses the one current record'
             if (answered) await answered;
             await expect(panel(page).locator('.panel-heading h2')).toBeVisible();
         }
-        await settle();
+        await requestBarrier(page);
         expect(reads, `cache ${cache}`).toHaveLength(cache ? 0 : 2);
     }
 });
@@ -435,6 +459,7 @@ test('host fields round-trip through /settings with revision', async ({ page, re
         await page.locator('#timeout').fill(bad);
         await page.locator('#timeout').press('Tab');
         await expect(page.locator('#timeout')).toHaveAttribute('aria-invalid', 'true');
+        await expect(statusOf(page)).toHaveText('Some changes were not saved. Review the highlighted settings.');
         await expect(saveOf(page)).toHaveAttribute('aria-disabled', 'true');
     }
     await page.locator('#timeout').fill('0');
@@ -450,6 +475,21 @@ test('host fields round-trip through /settings with revision', async ({ page, re
     expect(await storedSettings(page)).toBeNull();
 });
 
+test('server settings that cannot be read say so in their rows', async ({ page }) => {
+    await page.route(url => url.pathname === SETTINGS_URL, route => (route.request().method() === 'GET'
+        ? route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ success: false, error: 'Injected failure' }) })
+        : route.continue()));
+    await page.goto(`${workspace}#/system/appearance`);
+    for (const id of ['open-mode', 'timeout']) {
+        const row = page.locator('#workspace-main .setting-row').filter({ has: page.locator(`#${id}`) });
+        await expect(row.locator('p.small')).toHaveText('Server settings could not be read.');
+        await expect(page.locator(`#${id}`)).toBeDisabled();
+    }
+    // The browser rows are this browser's, and work as before.
+    await expect(page.locator('#default-view')).toBeEnabled();
+    await expect(page.locator('#workspace-main .setting-row p.small')).toHaveCount(2);
+});
+
 test('stale revision shows the conflict status', async ({ page, request }) => {
     await openAppearance(page);
     // Another page saves after this one read the settings.
@@ -495,6 +535,31 @@ test('reload keeps saved values; a new tab reads them', async ({ page, context }
     await other.close();
 });
 
+test('a change another tab saved while System was closed is a conflict on return, never saved over silently', async ({ page, context }) => {
+    await openAppearance(page);
+    await page.locator('#density').selectOption('compact');
+    await expect(statusOf(page)).toHaveText('Unsaved changes.');
+    // Leave System for Images: the draft stays for the page session, but System no longer listens.
+    await page.locator('#primary-nav a[data-route="images"]').click();
+    await expect(imageCards(page).first()).toBeVisible();
+    const other = await context.newPage();
+    await openAppearance(other);
+    await other.locator('#density').selectOption('large');
+    await saveAppearance(other, 'Saved in this browser.');
+    await other.close();
+    // Back in System: the draft is kept and flagged, and nothing was written over the other tab's value.
+    await page.evaluate(() => { location.hash = '#/system/appearance'; });
+    await expect(page.locator('#workspace-main .settings-layout > section.surface > h2')).toHaveText('Browsing & display');
+    await expect(statusOf(page)).toHaveText('Settings changed elsewhere. Reload this section to see them.');
+    await expect(page.locator('#density')).toHaveAttribute('aria-invalid', 'true');
+    await expect(page.locator('#density')).toHaveValue('compact');
+    expect((await storedSettings(page)).workspaceDensity).toBe('large');
+    // Saving after the conflict was shown is an explicit choice: it wins.
+    await saveAppearance(page, 'Saved in this browser.');
+    expect((await storedSettings(page)).workspaceDensity).toBe('compact');
+    await expect(page.locator('#density')).not.toHaveAttribute('aria-invalid');
+});
+
 test('a browser that refuses storage is saved for this tab only, never "Saved"', async ({ page }) => {
     await openAppearance(page);
     await page.evaluate(() => {

tests/browser/workspace_system_fixture.py

@@ -31,12 +31,22 @@ MAX_UNTHUMBNAILED = 50
 
 
 def register_system_commands(app, routes, api, root: Path):
+    # System › Appearance saves the host fields to this server's config.json (S3);
+    # reset restores both. The file is this server's own, inside its temp root:
+    # reset rewrites or deletes it, so a server whose configuration points
+    # anywhere else (a real config.json) refuses to register these hooks.
+    config_path = root / "config.json"
+    configured_path = os.environ.get("PROMPT_MANAGER_CONFIG_PATH")
+    if (
+        configured_path is None
+        or Path(configured_path).resolve() != config_path.resolve()
+    ):
+        raise RuntimeError(
+            f"PROMPT_MANAGER_CONFIG_PATH must be {config_path}, not {configured_path!r}"
+        )
     from py.config import GalleryConfig, PromptManagerConfig
 
     configured = list(GalleryConfig.MONITORING_DIRECTORIES)
-    # System › Appearance saves the host fields to this server's config.json (S3);
-    # reset restores both.
-    config_path = Path(os.environ["PROMPT_MANAGER_CONFIG_PATH"])
     config_bytes = config_path.read_bytes() if config_path.exists() else None
     host_values = (
         PromptManagerConfig.RESULT_TIMEOUT,
@@ -66,7 +76,10 @@ def register_system_commands(app, routes, api, root: Path):
         if config_bytes is None:
             config_path.unlink(missing_ok=True)
         else:
-            config_path.write_bytes(config_bytes)
+            # Replace in one step, as the server writes it (Windows preflight).
+            staged = config_path.with_name(config_path.name + ".reset")
+            staged.write_bytes(config_bytes)
+            os.replace(staged, config_path)
 
     @routes.post("/__test__/system/reset")
     async def system_reset(request):

tests/js/workspace-inspection-state.test.js

@@ -878,3 +878,20 @@ test('cache off rereads the current selection; on reuses the one current record'
         controller.dispose();
     }
 });
+
+test('cache off never rereads a selection whose source changed, and refresh never loads a deferred one', async () => {
+    const { api, requests } = pendingApi();
+    const controller = contract('createInspectionController')({ api, extractFacts: extractGenerationFacts, cache: false });
+    const first = controller.select(image());
+    requests[0].reject(new ApiError(409, refreshMessage, 'source-changed')); await first;
+    assert.equal(controller.state.refreshRequired, true);
+    controller.select(image());
+    assert.equal(requests.length, 1);
+    controller.dispose();
+    const deferred = contract('createInspectionController')({ api, extractFacts: extractGenerationFacts, autoLoad: false });
+    deferred.select(image());
+    assert.equal(deferred.refresh(), undefined);
+    assert.equal(requests.length, 1);
+    assert.equal(deferred.state.deferred, true);
+    deferred.dispose();
+});

tests/js/workspace-preferences.test.js

@@ -177,3 +177,17 @@ test('legacy paging keys are never consumed by the workspace', () => {
     const defaults = module.preferenceDefaults();
     for (const key of module.WORKSPACE_PREFERENCE_KEYS) assert.equal(defaults[key], gallerySettings.DEFAULT_SETTINGS[key], key);
 });
+
+test('a later save flushes the changes kept for this tab only', () => {
+    module.resetTabPreferences();
+    const store = storage(JSON.stringify({ gridColumns: 4 }), { quota: true });
+    assert.equal(module.writePreferences({ workspaceLayout: 'list' }, () => store).persisted, false);
+    // Storage accepts writes again: saving another key stores the tab-only change with it, so "Saved" is true for both.
+    const working = storage(store.text);
+    assert.deepEqual(module.writePreferences({ videoLoop: false }, () => working), { persisted: true, conflicts: [] });
+    assert.deepEqual(JSON.parse(working.text), { gridColumns: 4, workspaceLayout: 'list', videoLoop: false });
+    // Nothing is overlaid any more: another page's later change reads through.
+    working.text = JSON.stringify({ ...JSON.parse(working.text), workspaceLayout: 'masonry' });
+    assert.equal(module.readPreferences(() => working).values.workspaceLayout, 'masonry');
+    module.resetTabPreferences();
+});

tests/js/workspace-system-appearance.test.js

@@ -35,6 +35,7 @@ function fakeDOM() {
                 this.children.push(child);
             }
         }
+        remove() { if (this.parentNode) this.parentNode.children = this.parentNode.children.filter(child => child !== this); this.parentNode = null; }
         replaceChildren(...nodes) { this.children = []; this._text = ''; this.append(...nodes); }
         setAttribute(name, value) { this.attributes[name] = String(value); }
         getAttribute(name) { return Object.hasOwn(this.attributes, name) ? this.attributes[name] : null; }
@@ -66,7 +67,7 @@ const BROWSER = { workspaceLayout: 'grid', workspaceDensity: 'comfortable', auto
     imageQuality: 'medium', videoAutoplay: false, videoMute: true, videoLoop: true, showFilePaths: true, showImageInfo: true,
     sidebarCollapsedByDefault: false };
 
-async function mounted({ host = { webui_display_mode: 'newtab', result_timeout: 5 }, browser = BROWSER } = {}) {
+async function mounted({ host = { webui_display_mode: 'newtab', result_timeout: 5 }, browser = BROWSER, hostFails = false } = {}) {
     const { mountAppearance } = await appearanceModule;
     const { createSystemDrafts } = await stateModule;
     const { createScope } = await coreModule;
@@ -74,14 +75,15 @@ async function mounted({ host = { webui_display_mode: 'newtab', result_timeout:
     let hostValues = null;
     const drafts = createSystemDrafts({
         readBrowser: () => ({ ...browser }), readRuntime: () => ({}),
-        readHost: async () => ({ values: hostValues, revision: 'sha256:one' }),
+        readHost: async () => { if (hostFails) throw new Error('offline'); return { values: hostValues, revision: 'sha256:one' }; },
         writeBrowser: async () => ({ persisted: true }), writeHost: async () => ({ success: true, persisted: true }), writeRuntime: async () => ({}),
     });
     await drafts.load('browser');
     const scope = createScope();
     const [surface] = mountAppearance(null, { scope, drafts });
     const byId = id => dom.find(surface, node => node.dataset?.setting === id);
-    return { dom, drafts, scope, surface, byId, loadHost: async () => { hostValues = host; await drafts.load('host'); } };
+    return { dom, drafts, scope, surface, byId, loadHost: async () => { hostValues = host; await drafts.load('host'); },
+        fixHost: () => { hostFails = false; } };
 }
 
 test('the surface is the concept section with the concept rows in order, then the D97 rows', async () => {
@@ -166,3 +168,21 @@ test('reset restores defaults without touching other keys', async () => {
         assert.deepEqual([...DISPLAY_KEYS].sort(), Object.keys(BROWSER).sort());
     } finally { scope.dispose(); dom.restore(); }
 });
+
+test('host rows that could not be read stay disabled and say so; a later read clears it', async () => {
+    const { HOST_UNAVAILABLE } = await appearanceModule;
+    const { dom, drafts, scope, byId, surface, fixHost } = await mounted({ hostFails: true });
+    const notes = () => surface.children.slice(2).flatMap(row => row.children[0].children.slice(2).map(note => [row.children[0].children[0].textContent, note.className, note.textContent]));
+    try {
+        assert.deepEqual(notes(), []);
+        assert.equal(await drafts.reloadHost(), false);
+        assert.deepEqual(notes(), [['Open from ComfyUI', 'small', HOST_UNAVAILABLE], ['Node search auto-hide (seconds)', 'small', HOST_UNAVAILABLE]]);
+        assert.equal(byId('timeout').disabled, true);
+        // Repaints keep one line per row.
+        drafts.set('browser', 'videoMute', false);
+        assert.equal(notes().length, 2);
+        fixHost();
+        assert.equal(await drafts.reloadHost(), true);
+        assert.deepEqual(notes(), []);
+    } finally { scope.dispose(); assert.equal(dom.listenerCount(), 0); dom.restore(); }
+});

tests/js/workspace-system-state.test.js

@@ -283,3 +283,36 @@ test('a rejected value drops its draft and is highlighted; a conflict elsewhere
     assert.equal(drafts.get('browser', 'videoMute'), false);
     assert.throws(() => drafts.reject('nowhere', 'x'), TypeError);
 });
+
+test('a rejected value says why in the status until the next edit', async () => {
+    const drafts = await loaded(adapters({ host: { result_timeout: 5 } }));
+    drafts.reject('host', 'result_timeout');
+    assert.equal(drafts.statusText(), 'Some changes were not saved. Review the highlighted settings.');
+    drafts.set('host', 'result_timeout', 9);
+    assert.equal(drafts.statusText(), 'Unsaved changes.');
+});
+
+test('a browser write refused as changed elsewhere keeps the draft and reads the browser values again', async () => {
+    const domains = adapters({ browser: { workspaceDensity: 'comfortable' } });
+    const drafts = await loaded(domains);
+    drafts.set('browser', 'workspaceDensity', 'compact');
+    domains.outcome.browser = () => { throw Object.assign(new Error('changed'), { code: 'settings-changed' }); };
+    assert.deepEqual(await drafts.save(), { browser: 'conflict', host: 'clean', runtime: 'clean' });
+    assert.deepEqual(domains.calls.map(([name]) => name), ['writeBrowser', 'readBrowser']);
+    assert.equal(drafts.get('browser', 'workspaceDensity'), 'compact');
+    assert.deepEqual([...drafts.failed('browser')], ['workspaceDensity']);
+    assert.equal(drafts.statusText(), 'Settings changed elsewhere. Reload this section to see them.');
+});
+
+test('host read state says whether the server settings could be read', async () => {
+    const { createSystemDrafts } = await source;
+    const domains = adapters({ host: { result_timeout: 5 } });
+    let fail = true;
+    const drafts = createSystemDrafts({ ...domains, readHost: async () => { if (fail) throw new Error('offline'); return { values: {}, revision: 'r' }; } });
+    assert.equal(drafts.hostState(), 'pending');
+    assert.equal(await drafts.reloadHost(), false);
+    assert.equal(drafts.hostState(), 'failed');
+    fail = false;
+    assert.equal(await drafts.reloadHost(), true);
+    assert.equal(drafts.hostState(), 'loaded');
+});

tests/test_workspace_system_fixture.py

@@ -0,0 +1,38 @@
+"""The System browser hooks only ever reset their own server's config.json (#249 S3)."""
+
+import os
+import tempfile
+import unittest
+from pathlib import Path
+from unittest import mock
+
+from tests.browser.workspace_system_fixture import register_system_commands
+
+
+class TestWorkspaceSystemFixture(unittest.TestCase):
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.tmp.cleanup)
+        self.root = Path(self.tmp.name) / "server"
+        self.root.mkdir()
+
+    def test_refuses_a_config_path_outside_its_root(self):
+        elsewhere = Path(self.tmp.name) / "config.json"
+        elsewhere.write_bytes(b'{"real": true}')
+        for value in (str(elsewhere), None):
+            env = {
+                k: v for k, v in os.environ.items() if k != "PROMPT_MANAGER_CONFIG_PATH"
+            }
+            if value is not None:
+                env["PROMPT_MANAGER_CONFIG_PATH"] = value
+            with (
+                self.subTest(value=value),
+                mock.patch.dict(os.environ, env, clear=True),
+            ):
+                with self.assertRaisesRegex(RuntimeError, "PROMPT_MANAGER_CONFIG_PATH"):
+                    register_system_commands(None, None, None, self.root)
+        self.assertEqual(elsewhere.read_bytes(), b'{"real": true}')
+
+
+if __name__ == "__main__":
+    unittest.main()