From 91cdb23029526330ca12b01c7c6fb2897c23d1de Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Tue, 27 Oct 2026 07:37:48 -0700 Subject: [PATCH] fix(system): confirm Clear all logs on a fresh file list and record coverage Review fixes for the Diagnostics & logs section: - Clear all logs reads the log files again before it confirms, so the dialog names the copies it deletes now (a list it cannot read gives the general sentence, never an old count); Refresh reads the files too. - Runtime values that cannot be read stay disabled and say so. - Create test link holds its button until the write answered, so a second press cannot write a second record. - The Lines field names its range. - The browser server builds its logger in its own root, so no log file is ever opened in another folder; the fixture closes every directory scan and replaces a link instead of writing through it. - system.mjs drops the unused section fallback; frontend coverage floors recorded with coverage:update (system-diagnostics.mjs 97.5% lines). Part of #329 --- .../workspace-system-diagnostics.spec.mjs | 34 +++++++++++- .../browser/workspace_diagnostics_fixture.py | 27 ++++++++-- tools/vendor/frontend-coverage-baseline.json | 52 +++++++++++-------- web/js/workspace/system-diagnostics.mjs | 37 +++++++++---- web/js/workspace/system.mjs | 18 +++---- 5 files changed, 121 insertions(+), 47 deletions(-) diff --git a/tests/browser/workspace-system-diagnostics.spec.mjs b/tests/browser/workspace-system-diagnostics.spec.mjs index 8f7fc42d..9eb07de6 100644 --- a/tests/browser/workspace-system-diagnostics.spec.mjs +++ b/tests/browser/workspace-system-diagnostics.spec.mjs @@ -294,7 +294,8 @@ test('Lines accepts 1 to 500 and sends nothing when invalid', async ({ page, req const limit = page.locator('#log-limit'); await expect(page.locator('label[for="log-limit"]')).toHaveText('Lines'); await expect(limit).toHaveValue('100'); - expect(await limit.evaluate(node => [node.type, node.min, node.max])).toEqual(['number', '1', '500']); + expect(await limit.evaluate(node => [node.type, node.min, node.max, node.title])) + .toEqual(['number', '1', '500', 'Lines to show: a whole number from 1 to 500.']); await limit.fill('20'); await limit.dispatchEvent('change'); await expect.poll(() => urls.at(-1)).toBe('?limit=20'); @@ -424,6 +425,17 @@ test('a refused runtime value is marked invalid', async ({ page }) => { await expect(select).not.toHaveAttribute('aria-invalid'); }); +test('runtime values that cannot be read stay disabled and say so', async ({ page }) => { + await page.route(`**${CONFIG_URL}`, route => route.fulfill({ status: 500, contentType: 'application/json', + body: JSON.stringify({ success: false, error: 'boom' }) })); + await openDiagnostics(page); + for (const label of ['Runtime log level', 'Console logging']) { + const row = settingOf(page, label); + await expect(row.locator('p.small')).toHaveText('The current value could not be read.'); + await expect(row.locator('select, input[type="checkbox"]')).toBeDisabled(); + } +}); + // ── Task 6: log files, the reviewed cleanup and Clear all logs ───────────────────────────────────────────────────── const LOG_FOLDER = { rotated: [3000, 2048, 1024], other: true, link: true, active: 100 }; @@ -529,6 +541,26 @@ test('Clear all logs states both counts, confirms first and reads everything aga expect(await names(request)).toEqual(['prompt_manager.log', ...(LINKS ? ['prompt_manager.log.4'] : []), 'prompt_manager.log.bak']); }); +test('Clear all logs reads the files again before it states what it deletes', async ({ page, request }) => { + await seedLogs(request, { rotated: [1024], active: 100 }); + await openDiagnostics(page); + const details = await openFiles(page); + await expect(details.locator(':scope > p.small').first()).toHaveText(/rotated copies: 1$/); + // The log rolls over while the page stays open: Clear names what it deletes now, not the count shown. + await seedLogs(request, { records: false, rotated: [1024, 1024, 1024] }); + await operation(page, 'Clear all logs').getByRole('button', { name: 'Clear all logs…', exact: true }).click(); + await expect(dialog(page).locator('#modal-body > p')).toHaveText(/^Empties prompt_manager\.log, deletes 3 rotated copies \(3\.0 KB\) /); + await expect(details.locator(':scope > p.small').first()).toHaveText(/rotated copies: 3$/); + await dialog(page).getByRole('button', { name: 'Cancel', exact: true }).click(); + // A list that cannot be read gives the general sentence, never an old count. + await page.route('**/prompt_manager/logs/files', route => route.fulfill({ status: 500, contentType: 'application/json', + body: JSON.stringify({ success: false, error: 'boom', files: [] }) })); + await operation(page, 'Clear all logs').getByRole('button', { name: 'Clear all logs…', exact: true }).click(); + await expect(dialog(page).locator('#modal-body > p')).toHaveText(/^Empties prompt_manager\.log, deletes its rotated copies and clears/); + await dialog(page).getByRole('button', { name: 'Cancel', exact: true }).click(); + expect(await names(request)).toEqual(['prompt_manager.log', 'prompt_manager.log.1', 'prompt_manager.log.2', 'prompt_manager.log.3']); +}); + test('each listed file downloads by its returned name and a link has no Download', async ({ page, request }) => { await seedLogs(request, LOG_FOLDER); await openDiagnostics(page); diff --git a/tests/browser/workspace_diagnostics_fixture.py b/tests/browser/workspace_diagnostics_fixture.py index cefecba3..9a47009a 100644 --- a/tests/browser/workspace_diagnostics_fixture.py +++ b/tests/browser/workspace_diagnostics_fixture.py @@ -66,10 +66,22 @@ def _manager(): def repoint_logs(root: Path) -> Path: - """Point the logger at ``/logs`` before anything is logged.""" - manager = _manager() + """Point the logger at ``/logs`` before anything is logged. + + The logger is built while ``logs_dir`` answers that folder, so its first + handler never opens a log file anywhere else (a node folder with a legacy + database would otherwise name a real log folder). + """ + from utils import library_location + folder = Path(root) / "logs" folder.mkdir(exist_ok=True) + original = library_location.logs_dir + library_location.logs_dir = lambda **_kwargs: str(folder) + try: + manager = _manager() + finally: + library_location.logs_dir = original with manager._files_lock: manager.log_dir = folder manager._setup_loggers() @@ -94,6 +106,9 @@ def _record(time_text, level, message, index): def _touch(path: Path, size: int, moment: datetime.datetime) -> None: + # A link at this name is replaced, never written through. + if path.is_symlink(): + path.unlink() with open(path, "wb") as handle: handle.write(b"x" * size) stamp = moment.timestamp() @@ -122,7 +137,9 @@ def register_diagnostics_commands(app, routes, api, root: Path): with manager._files_lock: held = manager._acquire_file_handlers() try: - for entry in os.scandir(log_dir): + with os.scandir(log_dir) as entries: + found = list(entries) + for entry in found: path = Path(entry.path) if entry.name == ACTIVE and entry.is_file(follow_symlinks=False): manager._truncate_main_log(path) @@ -184,7 +201,9 @@ def register_diagnostics_commands(app, routes, api, root: Path): manager = _manager() log_dir = folder() entries = [] - for entry in sorted(os.scandir(log_dir), key=lambda item: item.name): + with os.scandir(log_dir) as listing: + found = sorted(listing, key=lambda item: item.name) + for entry in found: info = entry.stat(follow_symlinks=False) entries.append( {"name": entry.name, "size": info.st_size, "link": entry.is_symlink()} diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 3a914de7..140905b4 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -1,9 +1,9 @@ { "admin.js": { - "lines": 32, + "lines": 37, "branches": 61, - "functions": 27, - "statements": 32 + "functions": 32, + "statements": 37 }, "api-client.js": { "lines": 98, @@ -48,10 +48,10 @@ "statements": 98 }, "gallery.js": { - "lines": 45, - "branches": 66, - "functions": 48, - "statements": 45 + "lines": 49, + "branches": 68, + "functions": 50, + "statements": 49 }, "html-escape.js": { "lines": 98, @@ -133,7 +133,7 @@ }, "sse-stream.js": { "lines": 98, - "branches": 90, + "branches": 92, "functions": 98, "statements": 98 }, @@ -181,7 +181,7 @@ }, "workspace/activity-scan.mjs": { "lines": 96, - "branches": 87, + "branches": 90, "functions": 98, "statements": 96 }, @@ -420,10 +420,10 @@ "statements": 98 }, "workspace/maintenance-review.mjs": { - "lines": 94, - "branches": 86, - "functions": 90, - "statements": 94 + "lines": 95, + "branches": 87, + "functions": 91, + "statements": 95 }, "workspace/media-viewer.mjs": { "lines": 98, @@ -515,15 +515,21 @@ "functions": 89, "statements": 98 }, + "workspace/system-diagnostics.mjs": { + "lines": 95, + "branches": 86, + "functions": 94, + "statements": 95 + }, "workspace/system-integrations.mjs": { - "lines": 96, - "branches": 88, - "functions": 91, - "statements": 96 + "lines": 97, + "branches": 90, + "functions": 92, + "statements": 97 }, "workspace/system-maintenance.mjs": { "lines": 93, - "branches": 80, + "branches": 81, "functions": 92, "statements": 93 }, @@ -546,10 +552,10 @@ "statements": 98 }, "workspace/system-tagging.mjs": { - "lines": 80, - "branches": 70, - "functions": 75, - "statements": 80 + "lines": 98, + "branches": 94, + "functions": 95, + "statements": 98 }, "workspace/system-thumbnails.mjs": { "lines": 98, @@ -560,7 +566,7 @@ "workspace/system.mjs": { "lines": 98, "branches": 92, - "functions": 86, + "functions": 98, "statements": 98 }, "workspace/theme.mjs": { diff --git a/web/js/workspace/system-diagnostics.mjs b/web/js/workspace/system-diagnostics.mjs index 4e0c33d6..3541e9c0 100644 --- a/web/js/workspace/system-diagnostics.mjs +++ b/web/js/workspace/system-diagnostics.mjs @@ -66,6 +66,8 @@ export const COPY = Object.freeze({ statsUnread: 'The log statistics could not be read.', filesUnread: 'The log files could not be read.', configUnread: 'The retention settings could not be read.', + runtimeUnread: 'The current value could not be read.', + limitHint: 'Lines to show: a whole number from 1 to 500.', logDownloadFailed: 'The log could not be downloaded.', reportDone: 'Diagnostic report downloaded.', reportFailed: 'The diagnostic report could not be downloaded. Nothing was saved to this device.', @@ -297,15 +299,16 @@ function testLinkRow({ api, scope, notify }) { onClick: button => { if (button.getAttribute('aria-disabled') === 'true' || !prompt) return; const target = prompt; + // The button is held until the write answered, so a second press cannot write a second record. confirmDialog({ scope, trigger: button, title: 'Create test link', sentence: testLinkSentence(target), label: 'Create', - onConfirm: async () => { + onConfirm: () => pressing(button, async () => { try { const answer = await api.post(TEST_LINK_URL, { prompt_id: target.id }); if (!scope.signal.aborted) notify(`Test link created (image record ${answer?.result?.image_id}).`); } catch { if (!scope.signal.aborted) notify(COPY.testLinkFailed); } - } }); + }) }); } }); const button = row.querySelector('.row-actions > button'); const description = row.querySelector('p'); @@ -410,8 +413,16 @@ function runtimeRows({ scope, drafts }) { }; scope.cleanup(drafts.subscribe(repaint)); repaint(); - drafts.load('runtime').catch(() => {}); - return [level, consoleRow]; + // A failed read leaves both controls disabled; each says why (the value the server holds is not known). + const notes = [level, consoleRow].map(row => { + const note = el('p', COPY.runtimeUnread, 'small'); + return () => row.children[0].append(note); + }); + const reload = () => drafts.load('runtime').then(() => {}, () => { + if (!scope.signal.aborted) for (const show of notes) show(); + }); + reload(); + return { rows: [level, consoleRow], reload }; } /** The "Logs" surface. */ @@ -436,6 +447,7 @@ function logsSurface({ api, activity, scope, drafts, navigate, notify, latestOnl limit.max = String(MAX_LIMIT); limit.step = '1'; limit.value = String(DEFAULT_LIMIT); + limit.title = COPY.limitHint; const refresh = createButton('Refresh'); const downloadLog = createButton('Download log'); toolbar.append(levelLabel, level, limitLabel, limit, refresh, downloadLog); @@ -456,11 +468,18 @@ function logsSurface({ api, activity, scope, drafts, navigate, notify, latestOnl const retention = el('p', LOADING, 'small'); const list = el('div', undefined, 'operation-list'); let listed = null; + // The confirmation names what Clear deletes now: the files are read again before it opens (a read that fails + // gives the general sentence, never an old count). const clear = operationRow({ scope, title: 'Clear all logs', description: COPY.clearDescription, label: 'Clear all logs…', - danger: true, onClick: button => confirmDialog({ scope, trigger: button, title: 'Clear all logs', sentence: clearSentence(listed), - label: 'Clear', danger: true, onConfirm: clearLogs }) }); + danger: true, onClick: button => pressing(button, async () => { + await readFiles(); + if (scope.signal.aborted) return; + confirmDialog({ scope, trigger: button, title: 'Clear all logs', sentence: clearSentence(listed), label: 'Clear', + danger: true, onConfirm: clearLogs }); + }) }); files.append(el('summary', 'Log files & retention'), line, review, retention, list, clear); - surface.append(el('h2', 'Logs'), toolbar, lines, stats, ...runtimeRows({ scope, drafts }), files); + const runtime = runtimeRows({ scope, drafts }); + surface.append(el('h2', 'Logs'), toolbar, lines, stats, ...runtime.rows, files); const logsRead = latestOnly({ lifetime: scope.signal }); async function readLogs() { @@ -553,12 +572,12 @@ function logsSurface({ api, activity, scope, drafts, navigate, notify, latestOnl if (scope.signal.aborted) return; if (answer) notify(clearToast(answer.results)); readAll(); - drafts.load('runtime').catch(() => {}); + runtime.reload(); } scope.listen(level, 'change', readLogs); scope.listen(limit, 'change', readLogs); - scope.listen(refresh, 'click', () => { readLogs(); readStats(); }); + scope.listen(refresh, 'click', () => { readLogs(); readStats(); readFiles(); }); scope.listen(downloadLog, 'click', () => saveLog(downloadLog, ACTIVE_LOG)); scope.listen(review, 'click', () => reviewPlan({ api, store: activity, scope, operation: 'rotated_logs', trigger: review, notify, navigate })); diff --git a/web/js/workspace/system.mjs b/web/js/workspace/system.mjs index eb73c857..52f13b90 100644 --- a/web/js/workspace/system.mjs +++ b/web/js/workspace/system.mjs @@ -40,10 +40,9 @@ async function runningJob(error) { return jobs.running()[0]; } -function el(tag, text, className) { +function el(tag, className) { const node = document.createElement(tag); - if (text !== undefined) node.textContent = text; - if (className) node.className = className; + node.className = className; return node; } @@ -136,8 +135,7 @@ export const DELIVERED_SECTIONS = Object.freeze({ /** * Mount System under the heading `renderHeading` wrote in `main`. Returns `{show(section)}`, which shows another * section in place. `scope` is the page's view scope; `sections` maps a section id to a delivered section's mount - * (`(container, {api, activity, scope, drafts}) => nodes`); an id without a mount shows Overview's. `activity` is the - * page's Activity store, which follows the jobs a section starts. + * (`(container, {api, activity, scope, drafts}) => nodes`) for every section. `activity` is the page's Activity store, which follows the jobs a section starts. * * 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 @@ -162,9 +160,9 @@ export function mountSystem(main, api, scope, { section, activity, sections = DE 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'); - const saveRow = el('div', undefined, 'save-row'); - const status = el('span', undefined, 'small'); + const layout = el('div', 'settings-layout'); + const saveRow = el('div', 'save-row'); + const status = el('span', 'small'); status.id = 'settings-status'; const save = createButton('Save preferences', 'ca-primary'); saveRow.append(status, save); @@ -192,8 +190,8 @@ export function mountSystem(main, api, scope, { section, activity, sections = DE sectionScope = createScope(); for (const button of nav.children) button.setAttribute('aria-pressed', String(button.dataset.value === next)); while (layout.firstChild !== saveRow) layout.firstChild.remove(); - const mount = sections[next] ?? sections.overview; - const nodes = mount(layout, { api, activity, scope: sectionScope, drafts: store }); + // Every section is delivered (S12), and the router only names known sections (state.mjs). + const nodes = sections[next](layout, { api, activity, scope: sectionScope, drafts: store }); layout.prepend(...nodes); paintStatus(); }