From 4d554fbf409fecd3dad91f1b8eeadd8410c7a915 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 23 Oct 2026 06:55:02 -0700 Subject: [PATCH] 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. --- TESTING.md | 42 ++++++++--- tests/browser/workspace-system.spec.mjs | 75 ++++++++++++++++++-- tests/browser/workspace_system_fixture.py | 21 ++++-- tests/js/workspace-inspection-state.test.js | 17 +++++ tests/js/workspace-preferences.test.js | 14 ++++ tests/js/workspace-system-appearance.test.js | 26 ++++++- tests/js/workspace-system-state.test.js | 33 +++++++++ tests/test_workspace_system_fixture.py | 38 ++++++++++ tools/vendor/frontend-coverage-baseline.json | 8 +-- web/js/workspace/file-inspector.mjs | 4 +- web/js/workspace/images.mjs | 4 +- web/js/workspace/inspection-state.mjs | 8 ++- web/js/workspace/preferences.mjs | 7 +- web/js/workspace/system-appearance.mjs | 19 ++++- web/js/workspace/system-state.mjs | 23 ++++-- web/js/workspace/system.mjs | 31 +++++--- 16 files changed, 319 insertions(+), 51 deletions(-) create mode 100644 tests/test_workspace_system_fixture.py diff --git a/TESTING.md b/TESTING.md index 2ba63fd5..16cf4cc5 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1892,7 +1892,7 @@ the connection's `total_changes` is 0 at close and the data reads back unchanged `-shm` bytes are not compared: every WAL reader rewrites its read marks there, so no reader can keep it byte-identical (`test_health_check_never_writes`). -Recorded deviations (plan D60–D65 renumbered D92–D97; D121, D122 and D123 assigned): +Recorded deviations (plan D60–D65 renumbered D92–D97; D121, D122, D123 and D124 assigned): - **D92** (shell): no "Sample configuration" chip. The Save status is truthful: "No unsaved changes.", "Unsaved changes.", "Saved in this browser." / "Saved to the server @@ -1959,9 +1959,27 @@ Recorded deviations (plan D60–D65 renumbered D92–D97; D121, D122 and D123 as defaults" ("Restore this browser’s display defaults, then save. Other settings are kept.", button "Reset"), which drafts the gallery's defaults for the browser rows only. The node search timeout shows the server's real value, not the sample 30. CSS under D97. -- **D-NEW-S3-1** (inspector): with "Load metadata on selection" off, the Images panel and +- **D124** (inspector): with "Load metadata on selection" off, the Images panel and the viewer's details show "Metadata loads when you ask for it." and a "Load metadata" - button instead of reading the file; the concept has no such state. + button instead of reading the file. The concept has no such state, so no concept + comparison renders it: `metadata off shows an explicit load and loads once on request` + (workspace-system.spec) pins it instead. + +Appearance scope and states. The Default layout and Thumbnail size defaults start the +Images page only (a new visit, not the current view or a saved view); the embedded +galleries (a prompt history's Shared gallery and its All images / Repeat run dialogs) +have no toolbar, so they keep Grid/Comfortable, while the metadata, cache and card +defaults apply there too. A host row whose `/settings` read failed stays disabled with +"Server settings could not be read."; a rejected timeout says "Some changes were not +saved. Review the highlighted settings." A display default another page saved under one +of this tab's drafts flags that draft with the conflict status, whether it arrived while +System was mounted (a `storage` event) or while it was not (checked again on mount, and +before a write); the draft is never written over the other page's value without that +status first. A change kept for this tab only is stored by the next write storage accepts. + +Open question (saved-views follow-up, M8): a saved view stores no layout or size and +always opens at Grid/Comfortable, even when the Default layout is Masonry. Should saved +views store a layout, or follow the default? `tests/browser/system-concept.spec.mjs` compares the shell and Overview with the concept (dark and light, 1600×1100 and 390×844), mapping only the deviations above; unmapped, @@ -1984,18 +2002,24 @@ are no PNG baselines. `tests/browser/workspace-system.spec.mjs` covers the real the health check and metric 4, a failed check, the thumbnail row against the status endpoint (painted after the rest when its read is held, unavailable when it fails), the review row (Q6), the section links, the Statistics dialog against the route, a load failure with Retry and late answers after leaving, and Appearance: new -visits versus the current view and a saved view, metadata off and on request, the cache, -hidden paths, the video defaults, the host fields with their revision and a stale one, -reload and a second tab, storage refused, the D97 rows and reset. Its hooks are +visits versus the current view and a saved view, the embedded gallery keeping Grid, +metadata off and on request, the cache (counted up to a request barrier), hidden paths, +the video defaults, the host fields with their revision, a stale one and a failed read, +reload and a second tab, a second tab's save while System was closed, storage refused, +the D97 rows and reset. Its hooks are `tests/browser/workspace_system_fixture.py` (`/__test__/system/*`: reset, injected failures, an absent source, a deleted linked file, unthumbnailed files) on the concept-shaped fixture, with delays through the shared request holds; reset also restores -the server's host settings and its `config.json`. +the server's host settings and its `config.json`, which must be the one inside the server's +temp root (`tests/test_workspace_system_fixture.py`). ```sh -python3 -m unittest tests.test_database_health tests.test_system_api tests.test_library -v +python3 -m unittest tests.test_database_health tests.test_system_api tests.test_library \ + tests.test_workspace_system_fixture -v node --test tests/js/workspace-system-state.test.js tests/js/workspace-system-controls.test.js \ - tests/js/workspace-system-overview.test.js tests/js/workspace-state.test.js + tests/js/workspace-system-overview.test.js tests/js/workspace-state.test.js \ + tests/js/workspace-system-appearance.test.js tests/js/workspace-preferences.test.js \ + tests/js/workspace-inspection-state.test.js tests/js/workspace-images-state.test.js npm run --prefix tools/vendor test:workflows -- system-concept.spec.mjs workspace-system.spec.mjs \ --project chromium --project touch --project firefox --project webkit ``` diff --git a/tests/browser/workspace-system.spec.mjs b/tests/browser/workspace-system.spec.mjs index aa1ae755..ca2da6e7 100644 --- a/tests/browser/workspace-system.spec.mjs +++ b/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(() => { diff --git a/tests/browser/workspace_system_fixture.py b/tests/browser/workspace_system_fixture.py index ea1ce4b9..2413c1f0 100644 --- a/tests/browser/workspace_system_fixture.py +++ b/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): diff --git a/tests/js/workspace-inspection-state.test.js b/tests/js/workspace-inspection-state.test.js index 13bd1f12..27d095e9 100644 --- a/tests/js/workspace-inspection-state.test.js +++ b/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(); +}); diff --git a/tests/js/workspace-preferences.test.js b/tests/js/workspace-preferences.test.js index bf7f4d94..07b18cad 100644 --- a/tests/js/workspace-preferences.test.js +++ b/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(); +}); diff --git a/tests/js/workspace-system-appearance.test.js b/tests/js/workspace-system-appearance.test.js index 70f4ee09..497df7f6 100644 --- a/tests/js/workspace-system-appearance.test.js +++ b/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(); } +}); diff --git a/tests/js/workspace-system-state.test.js b/tests/js/workspace-system-state.test.js index bbb1043b..af60efb0 100644 --- a/tests/js/workspace-system-state.test.js +++ b/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'); +}); diff --git a/tests/test_workspace_system_fixture.py b/tests/test_workspace_system_fixture.py new file mode 100644 index 00000000..a43ec854 --- /dev/null +++ b/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() diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 722483f3..177df6b1 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -469,15 +469,15 @@ }, "workspace/system-state.mjs": { "lines": 98, - "branches": 93, + "branches": 94, "functions": 98, "statements": 98 }, "workspace/system.mjs": { - "lines": 96, + "lines": 98, "branches": 92, - "functions": 84, - "statements": 96 + "functions": 85, + "statements": 98 }, "workspace/theme.mjs": { "lines": 98, diff --git a/web/js/workspace/file-inspector.mjs b/web/js/workspace/file-inspector.mjs index d55035b3..80465acd 100644 --- a/web/js/workspace/file-inspector.mjs +++ b/web/js/workspace/file-inspector.mjs @@ -197,7 +197,7 @@ function prefixedChip(prefix, text) { * `variant: 'full'` (the viewer's `#viewer-details`, imageDetails(i, true)) leaves out the cover and adds "File & * provenance" and "Workflow & raw metadata" with the downloads. `inline: true` means `panel`; neither means `full`. */ -/** D-NEW-S3-1: the inspector's line while "Load metadata on selection" is off. */ +/** D124: the inspector's line while "Load metadata on selection" is off. */ export const DEFERRED_TEXT = 'Metadata loads when you ask for it.'; export function mountFileInspector(container, {api, sender, parentScope, variant, inline = false, promptContext = '', onTagFilter, onRetry, onRefresh, onLoad, onInteraction = () => {}, copyFallback = openCopyFallback}) { @@ -239,7 +239,7 @@ export function mountFileInspector(container, {api, sender, parentScope, variant function render(state) { const inner = el('div', undefined, full ? undefined : 'detail-inner'); if (state.loading || state.error) {container.append(inner); statusView(state, inner); return;} - // D-NEW-S3-1: "Load metadata on selection" is off, so the file's details wait for an explicit request. + // D124: "Load metadata on selection" is off, so the file's details wait for an explicit request. if (state.deferred) { container.append(inner); const status = el('p', DEFERRED_TEXT, 'small'); status.setAttribute('role', 'status'); inner.append(status); diff --git a/web/js/workspace/images.mjs b/web/js/workspace/images.mjs index a3ebd061..b96dab12 100644 --- a/web/js/workspace/images.mjs +++ b/web/js/workspace/images.mjs @@ -122,7 +122,9 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage const prior = embedded || saved || favorites ? null : lastVisit; // A stale visit keeps its filters, layout and size, and lists again instead of restoring (M6). const visit = prior?.stale ? null : prior; - const start = initialImagesView({prior, saved, preferences}); + // The layout and size defaults are the Images page's: an embedded gallery (a prompt history's, its dialogs) has no + // toolbar to change them, so it keeps its fixed Grid/Comfortable. The metadata and card defaults apply everywhere. + const start = initialImagesView({prior, saved, preferences:embedded ? null : preferences}); let density = start.density; let metrics = {...INITIAL_METRICS}; let layout = start.layout, selection = visit?.selection || null, frame = 0, loadFrame = 0, restoreFrame = 0, observer; diff --git a/web/js/workspace/inspection-state.mjs b/web/js/workspace/inspection-state.mjs index d9a0ec5f..f9567ed1 100644 --- a/web/js/workspace/inspection-state.mjs +++ b/web/js/workspace/inspection-state.mjs @@ -350,7 +350,8 @@ const emptyState = () => ({ loading: false, error: false, deferred: false, recor * One selection's inspection. #249 System S3 (Appearance): `autoLoad: false` ("Load metadata on selection" off) makes a * selection publish `deferred` instead of reading, and `load()` reads it once on request; `cache: false` ("Cache * metadata in this browser" off) re-reads the selected file when it is selected again, where the default reuses the one - * current record. A read in flight is shared either way. + * current record. A read in flight is shared either way, and a file whose source changed is never re-read by selecting + * it again (as `retry()` and `refresh()` refuse). */ export function createInspectionController({ api, extractFacts, adapter = rootedFileAdapter, autoLoad = true, cache = true }) { let state = Object.freeze(emptyState()), current = null, pending = null, disposed = false; @@ -375,7 +376,7 @@ export function createInspectionController({ api, extractFacts, adapter = rooted try { adapter.validate(image); const scope = adapter.scope(image), key = adapter.key(image), path = adapter.inspectionPath(image); - if (current?.key === key && current.scope === scope && current.path === path && (cache || state.loading || state.deferred)) return pending; + if (current?.key === key && current.scope === scope && current.path === path && (cache || state.loading || state.deferred || state.refreshRequired)) return pending; selected = { key, scope, path }; } catch { // The loader publishes validation errors after invalidating the previous source. @@ -396,7 +397,8 @@ export function createInspectionController({ api, extractFacts, adapter = rooted }, /** Re-read the selected file keeping the shown record until the answer (a retry when none is shown). */ refresh() { - if (disposed || !hasAttempt || state.refreshRequired) return; + // A deferred selection loads only through `load()` (the user's "Load metadata"). + if (disposed || !hasAttempt || state.refreshRequired || state.deferred) return; return read(attempted, { quiet: Boolean(state.record) && !state.loading && !state.error }); }, clear() { diff --git a/web/js/workspace/preferences.mjs b/web/js/workspace/preferences.mjs index a21b0294..c964d846 100644 --- a/web/js/workspace/preferences.mjs +++ b/web/js/workspace/preferences.mjs @@ -11,7 +11,7 @@ * - `writePreferences(dirty, getStorage, {baseline})` → `{persisted, conflicts}`: re-reads the stored object and writes * back only the dirty keys, every other key byte-for-byte as stored. A write the browser refused (denied storage, a * full quota) leaves the stored bytes as they were and keeps the change for this tab only, so every reader in this - * tab still sees it. `conflicts` lists the dirty keys another page changed since `baseline` was read; the explicit + * tab still sees it; the next write storage accepts stores those changes too. `conflicts` lists the dirty keys another page changed since `baseline` was read; the explicit * save still wins. * - `subscribePreferences(win, onChange, getStorage)` → unsubscribe: `onChange(readPreferences())` after another page * changed the settings (a `storage` event). @@ -82,12 +82,13 @@ export function writePreferences(dirty, getStorage = defaultStorage, { baseline const current = pick(normalizer()(stored)); const conflicts = baseline ? Object.keys(dirty).filter(key => Object.hasOwn(baseline, key) && current[key] !== baseline[key]) : []; try { - storage.setItem(PREFERENCES_KEY, JSON.stringify({ ...stored, ...dirty })); + // The changes this tab could not store before are stored with this one, so "Saved" is true for all of them. + storage.setItem(PREFERENCES_KEY, JSON.stringify({ ...stored, ...Object.fromEntries(tabOnly), ...dirty })); } catch { keep(); return { persisted: false, conflicts }; } - for (const key of Object.keys(dirty)) tabOnly.delete(key); + tabOnly.clear(); return { persisted: true, conflicts }; } diff --git a/web/js/workspace/system-appearance.mjs b/web/js/workspace/system-appearance.mjs index 4969933b..290bf778 100644 --- a/web/js/workspace/system-appearance.mjs +++ b/web/js/workspace/system-appearance.mjs @@ -13,12 +13,15 @@ * (`showImageInfo`), "Collapse viewer details by default" (`sidebarCollapsedByDefault`) and the operation "Reset * display defaults", which drafts the gallery's defaults for these browser rows and touches no other key. * - * Controls follow the store: a value read later (the host settings, another tab's change) repaints them. + * Controls follow the store: a value read later (the host settings, another tab's change) repaints them. A host row + * whose value could not be read stays disabled and says so ("Server settings could not be read."). */ import { settingRow, operationRow } from './system-controls.mjs'; import { preferenceDefaults } from './preferences.mjs'; export const RESULT_TIMEOUT_MAX = 300; +/** A host row's line while the server settings could not be read (its control stays disabled). */ +export const HOST_UNAVAILABLE = 'Server settings could not be read.'; const BOOLEAN = Object.freeze({ type: 'toggle' }); /** @@ -115,7 +118,7 @@ export function mountAppearance(container, { scope, drafts }) { surface.append(node); const input = controlOf(node, id); input.dataset.key = key; - controls.push({ input, control, domain, key }); + controls.push({ input, control, domain, key, text: node.children[0], note: null }); } surface.append(operationRow({ title: 'Reset display defaults', description: 'Restore this browser’s display defaults, then save. Other settings are kept.', label: 'Reset', scope, @@ -123,7 +126,17 @@ export function mountAppearance(container, { scope, drafts }) { const defaults = preferenceDefaults(); for (const key of DISPLAY_KEYS) drafts.set('browser', key, defaults[key]); } })); - const repaint = () => { for (const { input, control, domain, key } of controls) paint(input, control, drafts.get(domain, key)); }; + const repaint = () => { + const unavailable = drafts.hostState?.() === 'failed'; + for (const row of controls) { + const value = drafts.get(row.domain, row.key); + paint(row.input, row.control, value); + if (row.domain !== 'host') continue; + if (unavailable && value === undefined) { + if (!row.note) { row.note = el('p', HOST_UNAVAILABLE, 'small'); row.text.append(row.note); } + } else if (row.note) { row.note.remove(); row.note = null; } + } + }; scope.cleanup(drafts.subscribe(repaint)); repaint(); return [surface]; diff --git a/web/js/workspace/system-state.mjs b/web/js/workspace/system-state.mjs index 49f3a5c5..b827dc3e 100644 --- a/web/js/workspace/system-state.mjs +++ b/web/js/workspace/system-state.mjs @@ -15,8 +15,9 @@ * the page session (Q10a): a section left and opened again shows them, and a reload of its values keeps them. * * Adapters: `readBrowser()` and `readRuntime()` return the values; `readHost()` returns `{values, revision}`. - * `writeBrowser(changes)` returns `{persisted}` (false: kept for this tab only), `writeHost(changes, - * {expected_revision})` the settings response (`{success, persisted, revision}`), `writeRuntime(changes)` anything; + * `writeBrowser(changes)` returns `{persisted}` (false: kept for this tab only) and throws an error with `code: + * '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}`), `writeRuntime(changes)` anything; * a write that throws failed. */ export { SYSTEM_SECTIONS, isSystemSection } from './state.mjs'; @@ -64,6 +65,7 @@ export function createSystemDrafts(adapters) { const failedKeys = { browser: new Set(), host: new Set(), runtime: new Set() }; const listeners = new Set(); let revision = null; + let hostRead = 'pending'; let outcome = null; let saving = null; @@ -78,6 +80,7 @@ export function createSystemDrafts(adapters) { const read = await adapters.readHost(); values = read?.values ?? {}; revision = read?.revision ?? null; + hostRead = 'loaded'; } else { values = await (domain === 'browser' ? adapters.readBrowser() : adapters.readRuntime()); } @@ -159,8 +162,11 @@ export function createSystemDrafts(adapters) { } } - /** Read the host values again; a failed read leaves host saves refused until one succeeds. */ - const reloadHost = () => load('host').then(() => true, () => false); + /** + * Read the host values again; a failed read leaves host saves refused until one succeeds, and `hostState()` says + * so (System S3: the host rows show that the server settings could not be read). + */ + const reloadHost = () => load('host').then(() => true, () => { hostRead = 'failed'; notify(); return false; }); /** Save the dirty domains in order; a Save pressed while one runs joins it. */ function save() { @@ -170,6 +176,7 @@ export function createSystemDrafts(adapters) { for (const domain of DRAFT_DOMAINS) result[domain] = await saveDomain(domain); // Settings changed elsewhere: read them (and their revision), keeping the drafts (Q10). if (result.host === 'conflict') await reloadHost(); + if (result.browser === 'conflict') await load('browser').catch(() => {}); return result; })().then(result => { saving = null; @@ -180,11 +187,15 @@ export function createSystemDrafts(adapters) { return saving; } - /** A value the control could not take (System S3: a timeout outside 0–300): no draft, and the control is highlighted. */ + /** + * A value the control could not take (System S3: a timeout outside 0–300): no draft, the control is highlighted and + * the status says a change was not saved. + */ function reject(domain, key) { requireDomain(domain); drafts[domain].delete(key); failedKeys[domain].add(key); + outcome = { [domain]: 'failed' }; notify(); } @@ -203,6 +214,8 @@ export function createSystemDrafts(adapters) { return { load, reloadHost, + /** Whether the host values were read: `pending`, `loaded`, or `failed` (the last read failed). */ + hostState: () => hostRead, reject, conflict, /** The saved values of a domain (its last read or save), not the drafts. */ diff --git a/web/js/workspace/system.mjs b/web/js/workspace/system.mjs index 1f6437ca..27e1a54a 100644 --- a/web/js/workspace/system.mjs +++ b/web/js/workspace/system.mjs @@ -16,7 +16,7 @@ import { createSystemDrafts, DRAFT_DOMAINS } from './system-state.mjs'; import { sectionNav } from './system-controls.mjs'; import { healthNotice, mountOverview } from './system-overview.mjs'; import { mountAppearance } from './system-appearance.mjs'; -import { readPreferences, reconcilePreferences, subscribePreferences, writePreferences } from './preferences.mjs'; +import { displayPreferences, readPreferences, reconcilePreferences, subscribePreferences, writePreferences } from './preferences.mjs'; const SETTINGS_URL = '/prompt_manager/settings'; const ADMIN = Object.freeze({ href: '/prompt_manager/admin', label: 'Open existing administration' }); @@ -57,7 +57,14 @@ export function systemDrafts(api) { drafts ??= createSystemDrafts({ readBrowser: () => readPreferences().values, readRuntime: () => ({}), - writeBrowser: changes => ({ persisted: writePreferences(changes).persisted }), + writeBrowser: changes => { + // Another page changed a drafted value since this tab read it, and no storage event told it yet: refuse, + // so the store keeps the draft, flags it and reads the values again; the next Save is an explicit choice. + const baseline = drafts.saved('browser'); + const { conflicts } = reconcilePreferences({ baseline, drafts: changes, next: readPreferences().values }); + if (conflicts.length) throw Object.assign(new Error('Settings changed elsewhere'), { code: 'settings-changed' }); + return { persisted: writePreferences(changes, undefined, { baseline }).persisted }; + }, writeRuntime: undelivered, async readHost() { const result = await api.get(SETTINGS_URL, { cache: 'no-store' }); @@ -110,18 +117,24 @@ export const DELIVERED_SECTIONS = Object.freeze({ * * The host values are read on mount (and by the store again after a conflict), so a save carries the current revision; * until a read returned one, a host save fails without a request (system-state.mjs). The browser values are read on - * mount and again whenever another page changes them (a `storage` event). + * mount and again whenever another page changes them (a `storage` event). Either way a value another page changed + * under one of this tab's drafts flags that draft (D92's conflict status), including a change made while System was + * not mounted (the drafts outlive it, Q10). */ export function mountSystem(main, api, scope, { section, activity, sections = DELIVERED_SECTIONS, navigate = navigateTo }) { const store = systemDrafts(api); store.reloadHost(); - store.load('browser'); // Another tab or the legacy gallery changed the display defaults: clean values follow it; a draft it overtook stays - // and is flagged (D92's conflict status). - scope.cleanup(subscribePreferences(window, next => { - const { conflicts } = reconcilePreferences({ baseline: store.saved('browser'), drafts: store.changes('browser'), next: next.values }); - store.load('browser').then(() => store.conflict('browser', conflicts)); - })); + // and is flagged (D92's conflict status). `next` is null when the values cannot be read (no gallery-settings.js). + const reconcile = next => { + const baseline = store.saved('browser'); + // The first mount has read nothing yet, so nothing can have been overtaken. + const conflicts = next && Object.keys(baseline).length + ? reconcilePreferences({ baseline, drafts: store.changes('browser'), next }).conflicts : []; + store.load('browser').then(() => store.conflict('browser', conflicts), () => {}); + }; + reconcile(displayPreferences()); + scope.cleanup(subscribePreferences(window, next => reconcile(next.values))); const nav = sectionNav({ label: 'System sections', sections: SYSTEM_SECTIONS, active: section, scope, onSelect: id => navigate(`#/system/${id}`) }); const layout = el('div', undefined, 'settings-layout');