diff --git a/TESTING.md b/TESTING.md index 0ea8452..12adaec 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1929,6 +1929,56 @@ cache off selecting the shown file again re-reads it (on, the one current record reused); paths off leaves out only the Path row of File & provenance; the viewer reads the video defaults. +Sources & scans (`system-sources.mjs`, S4) is the concept's "sources" body on the +server's sources. `GET /prompt_manager/settings` reports them (public paths only): +`sources.configured` (each `gallery_root_paths` entry with `valid`, `code` and the +server's `reason`), `sources.effective` (what is scanned and served, with its +`root_index` and `watch`: `watching` only when the image monitor's observer is alive, +scheduled that folder and monitoring is on, `stopping` when it still watches it but +monitoring was switched off, `manual` when monitoring is off or no monitor runs, else +`restart_pending`), `sources.auto_detected`, `monitor` and `settings.monitoring_enabled`. +`POST /prompt_manager/settings/validate-source {path}` checks one folder and writes +nothing. Every caller (validate-source, the settings save, config.json at load) refuses +a source through the same checks (`sources.source_problem`): its form first, then +containment decided as text (`normcase(normpath)` against the allowed folders' own +spellings), before anything resolves, stats or opens the path, so a UNC share, the +`\\?\` and `\\.\` namespaces, a device or any folder outside the allowed ones never +reaches the filesystem or the network (no SMB request, no existence or timing oracle). +Only then is the path resolved and contained again, which catches a symlink out. The +filesystem root and the home directory answer `outside-allowed` with the same reason as +any outside path. ComfyUI folders outside its base directory (`--output-directory`) +are path anchors too, so their sources keep a relative public path +(`renders/portraits`) that round-trips and never collides by folder name. A save whose +list holds a source the server cannot use is a 400 `invalid-source` naming its public +path (`source`, as text: a refused path is never resolved to name it), its `index` and +`reason`, and saves nothing. A save that changes the sources (compared as folders, not +spellings) holds the registry as `records` ("Source change"), so it is refused with +409 `busy` while a scan runs. `POST /prompt_manager/jobs/scan {scope, roots, paths}` +scans only the given effective `root_index`es (strict ints, at most 64), each with the +public path the page read for it; an index past the list or one that now names another +folder is 409 `sources-changed` and starts nothing (the page toasts the server's +reason and reads the sources again); the legacy `/scan` is unchanged. The section +lists one row per source (" · watching | watching until restart | manual scan | +restart pending" with "Scan changes" for a watched source, else "Scan folder"), which +opens the shared "Review metadata scan" for that one source ("1 output folder") and +starts the session job over it; the job opens in Activity, and the toast only says it +started. Enrich +existing metadata and the notice's Review scan review every source. The notice shows +until this browser dismissed the 3.2.0 update notice (`pm_update_notice_dismissed`, +the legacy admin page's key); Later dismisses it without scanning, and Review scan +dismisses it as the legacy Scan does. "+ Add source" asks validate-source, then adds +the folder to the host draft of `gallery_root_paths`; Remove takes one out; "Watch for +new outputs" (`monitoring_enabled`) and Worker threads (`worker_threads`) are host +drafts too, and Save writes them with the revision. A save the server refuses because a +scan runs toasts "Another job is running: ." with a link to that job (D57) and +keeps the drafts; a save refused for one source says the server's message as the status +and marks that row. After a save the source list is read again; a monitoring change +waits for a restart (the running observer keeps watching until then, and the status +says "Restart ComfyUI to apply: Watch for new outputs."). Thumbnail generation's +Review options opens the legacy gallery in a new tab (D94) until its review dialog (S5). +A list that cannot be read shows "Sources could not be loaded. Your saved data has not +changed." and Retry, never an empty list. + Overview (`system-overview.mjs`) reads `GET /prompt_manager/system/overview` once per visit through `latestOnly()` on the section scope; a late answer after the section left paints nothing. Until it answers, the section shows "Loading the overview…"; a failure @@ -1956,7 +2006,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, D123, D124, D125 and D126 assigned): +Recorded deviations (plan D60–D66 and D81 renumbered D92–D98 and D113; D121–D128 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 @@ -2023,6 +2073,47 @@ Recorded deviations (plan D60–D65 renumbered D92–D97; D121, D122, D123, D124 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. +- **D98** (Sources): each configured source's row has a plain Remove after its scan + button; nothing changes until Save. A source added this session reads "<path> · not + saved yet" with Remove only until Save; a configured source the server cannot use + reads "<path> · <server reason>" with Remove only. A line under the list says when the + list is not the whole story: "No source is configured. After saving, ComfyUI’s output + folder is used." (the last one removed), "No source is configured; ComfyUI’s output + folder is used." and "No configured source is available; ComfyUI’s output folder is + used." (auto-detected rows, listed without Remove). "restart pending" is the third + watch state. The operation "Refresh files" ("Read every source again to find files + added or removed outside ComfyUI.", button "Refresh") closes the surface: it calls the + legacy gallery's `/gallery/rescan` and toasts "Files refreshed. N files in M output + folders." or "Files could not be refreshed. Try again.". The concept's "sample path" + note is prototype copy and is dropped. +- **D113** (Sources): the Worker threads range ends at the server's `cpu_count` (the + concept's fixed `max=16`) and shows the server's real `worker_threads` (the concept's + sample 4). +- **D127** (Sources): the concept's `add-source` `simpleInput` with production + copy: the field is "Folder path" (the concept's "(sample only)" dropped), it starts + empty (not the demo "/images/new-collection"), and the action is "Add" (not "Apply in + prototype"); Enter in the field adds too. `add source dialog matches the concept` + (system-concept.spec) compares the open dialog with these three mapped, and `add + source comparison detects mutations` proves the mapping is the only difference. A + folder the server refuses keeps the dialog open with the field + `aria-invalid` and the server's reason in a `p.small[role=alert]` above the actions; + a folder the server names as one already listed says "This folder is already a + source.", and a check that cannot finish says "The folder could not be checked. Try + again."; neither adds anything. + The concept dialog does no check, so `invalid source shows the server reason` + (workspace-system-sources.spec) pins it. +- **D128** (Sources): states the concept's sample rows never show. A source the + running monitor still watches after "Watch for new outputs" was saved off reads + "<path> · watching until restart" (still "Scan changes"), and the save's status adds + "Restart ComfyUI to apply: Watch for new outputs." from `restart_required_fields`. A + configured source the effective list holds under another spelling reads "<path> · + same folder as another source" with only Remove. A save refused for one source + (`invalid-source`) marks that row (`aria-invalid`, outlined like a D92 setting) and + the status is the server's "<path> can't be used: <reason>. Remove it or reconnect it, + then save." ("Remove it, then save." when reconnecting cannot help). Pinned by + `Watch for new outputs saves monitoring_enabled; a restart applies it` and `a save + refused for one unusable source names it and marks its row` + (workspace-system-sources.spec) and the system-state and system-sources node tests. - **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, so no concept @@ -2074,9 +2165,16 @@ renames a metric label, swaps the `--line` token, recolours a chip, renames a ch removes the totals table and renames a caption. `appearance matches the concept` maps only D97 and the real timeout (unmapped, exactly those paths differ), and `appearance comparison detects mutations` removes a row, renames a label and an option and swaps -the `--line` token. Paired captures (`{concept,production}-system-{dark,light}-{1600,390}.png`, +the `--line` token. `sources matches the concept with|without the notice` (both +themes and sizes; the fixture's monitor watches the first source, as the concept's +first sample is watched, and the concept's Later removes its notice) maps the real +sources for the two samples, D98's Remove and Refresh files, and D113; unmapped, +exactly those paths differ. `sources comparison detects mutations` removes the notice +and a source row, renames a label and the Add source button and swaps the `--line` +token. Paired captures (`{concept,production}-system-{dark,light}-{1600,390}.png`, `{concept,production}-{statistics,health}-{dark,light}.png`, -`{concept,production}-appearance-{dark,light}-{1600,390}.png`) are test artifacts; there +`{concept,production}-appearance-{dark,light}-{1600,390}.png`, +`{concept,production}-sources-{with,without}-notice-{dark,light}-{1600,390}.png`) are test artifacts; there are no PNG baselines. `tests/browser/workspace-system.spec.mjs` covers the real counts, 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 @@ -2085,9 +2183,14 @@ visits versus the current view and a saved view, the embedded gallery keeping Gr 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 +the D97 rows and reset. `tests/browser/workspace-system-sources.spec.mjs` covers the +notice's Later and Review scan, a scoped scan from one source into Activity, Add source +with its server check and Save, a refused folder, Remove down to auto-detect, an absent +configured source, the three watch states, the monitoring and worker rows, Refresh +files, the thumbnail hand-off, a save refused by a running scan and a failed read. 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 +failures, an absent source, a deleted linked file, unthumbnailed files, a fake image +monitor) on the concept-shaped fixture, with delays through the shared request holds; reset also restores 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`). @@ -2098,8 +2201,11 @@ python3 -m unittest tests.test_database_health tests.test_system_api tests.test_ 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-appearance.test.js tests/js/workspace-preferences.test.js \ - tests/js/workspace-inspection-state.test.js tests/js/workspace-images-state.test.js + tests/js/workspace-inspection-state.test.js tests/js/workspace-images-state.test.js \ + tests/js/workspace-system-sources.test.js +python3 -m unittest tests.test_settings_sources tests.test_scan_scope -v npm run --prefix tools/vendor test:workflows -- system-concept.spec.mjs workspace-system.spec.mjs \ + workspace-system-sources.spec.mjs \ --project chromium --project touch --project firefox --project webkit ``` diff --git a/py/api/admin.py b/py/api/admin.py index dc4e9bc..5b530ef 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -4,6 +4,7 @@ import asyncio import contextlib from contextlib import closing import datetime +import functools import hashlib import json import math @@ -1344,28 +1345,34 @@ class AdminRoutesMixin: } ) - def _start_scan_job(self, files=DEFAULT_SCAN_SCOPE): + def _start_scan_job(self, files=DEFAULT_SCAN_SCOPE, roots=None, output_dirs=None): """Register the scan job synchronously; its runner starts on the loop. - ``files`` is the scope (``SCAN_SCOPES``). Raises JobBusy while another - record-changing job runs. + ``files`` is the scope (``SCAN_SCOPES``). ``roots`` selects sources by + their index in ``output_dirs`` (the effective sources the caller + resolved); None scans every source, resolved when the job starts. + Raises JobBusy while another record-changing job runs. """ if files not in SCAN_SCOPES: raise ValueError(f"Unknown scan scope: {files!r}") + if roots is not None and ( + output_dirs is None or not roots or max(roots) >= len(output_dirs) + ): + raise ValueError("A scoped scan needs the sources its roots index") job = self._jobs.start( SCAN_JOB_KIND, SCAN_JOB_TITLE, record_changing=True, - scope={"files": files}, - runner=self._run_scan_job, + scope={"files": files, "roots": None if roots is None else list(roots)}, + runner=functools.partial(self._run_scan_job, output_dirs=output_dirs), groups=SCAN_GROUP_NAMES, ) self._scan_job = job return job - async def _run_scan_job(self, job): + async def _run_scan_job(self, job, output_dirs=None): final = None - async for payload in self._scan_images_events(job): + async for payload in self._scan_images_events(job, output_dirs): job.publish(payload) final = payload.get("type") state = SCAN_FINAL_STATES.get(final, "finished") @@ -1400,16 +1407,25 @@ class AdminRoutesMixin: ) return response - async def _scan_images_events(self, job): - """Yield progress payloads while scanning every configured output directory. + async def _scan_images_events(self, job, output_dirs=None): + """Yield progress payloads while scanning the output directories. - The payloads are the legacy SSE events, unchanged; ``job`` also gets - its status line, totals and one outcome per processed file. + Every effective source is walked, or only those ``job.scope["roots"]`` + selects (indexes into ``output_dirs``). The payloads are the legacy SSE + events, unchanged; ``job`` also gets its status line, totals and one + outcome per processed file. Rows keep each source's listing index. """ try: self.logger.info("Starting image scan operation") - output_dirs = self._get_all_output_dirs() - if not output_dirs: + if output_dirs is None: + output_dirs = self._get_all_output_dirs() + selected = job.scope.get("roots") + walked = ( + output_dirs + if selected is None + else [output_dirs[index] for index in selected] + ) + if not walked: self.logger.error("No output directories found") message = ( "No output directories found. " @@ -1419,18 +1435,18 @@ class AdminRoutesMixin: yield {"type": "error", "message": message} return - dir_names = [self._public_path(d) for d in output_dirs] - folders = _plural(len(output_dirs), "output folder") - job.scope["output_folders"] = len(output_dirs) + dir_names = [self._public_path(d) for d in walked] + folders = _plural(len(walked), "output folder") + job.scope["output_folders"] = len(walked) scope = SCAN_SCOPE_LOG[job.scope.get("files", "all")] job.log_line(f"Scan started · {folders} · {scope}") job.update(status_line=f"Looking for media files in {folders}") yield self._scan_progress( - 0, f"Scanning {len(output_dirs)} directory(ies) for media files..." + 0, f"Scanning {len(walked)} directory(ies) for media files..." ) media_files = await self._run_in_executor( - self._collect_output_media_sync, output_dirs + self._collect_output_media_sync, walked ) counts = dict.fromkeys(SCAN_COUNT_KEYS, 0) job.update( diff --git a/py/api/job_routes.py b/py/api/job_routes.py index f855660..83caa85 100644 --- a/py/api/job_routes.py +++ b/py/api/job_routes.py @@ -18,29 +18,74 @@ def _not_found(): return web.json_response({"success": False, "error": "Job not found"}, status=404) -def _bad_request(message): - return web.json_response({"success": False, "error": message}, status=400) +MAX_SCAN_ROOTS = 64 +_SCOPE_ERROR = 'The scan scope must be "new" or "all"' +_ROOTS_ERROR = ( + f"roots must be a non-empty list of up to {MAX_SCAN_ROOTS} source indexes, " + "with paths naming the public path of each" +) +SOURCES_CHANGED_ERROR = "The sources changed since they were loaded. Reload them." -async def _scan_scope(request): - """The requested scan scope ("new" or "all"), or None when the body is invalid. +def _bad_request(message, code=None): + body = {"success": False, "error": message} + if code is not None: + body["code"] = code + return web.json_response(body, status=400) - An empty body (also a chunked one with no bytes), or one without - ``scope``, asks for the default. + +def _scan_roots(body): + """``(roots, error)``: ``{index: public path}`` of the selected sources, or None. + + Absent ``roots`` means every source. When present it is a non-empty list + of at most MAX_SCAN_ROOTS non-negative ints (never bools or floats), and + ``paths`` the same-length list of the public path the caller read for + each (GET /settings), so a stale index never scans another folder. """ + if "roots" not in body: + return None, None + raw = body["roots"] + paths = body.get("paths") + if not isinstance(raw, list) or not raw or len(raw) > MAX_SCAN_ROOTS: + return None, _ROOTS_ERROR + if not isinstance(paths, list) or len(paths) != len(raw): + return None, _ROOTS_ERROR + selected = {} + for value, path in zip(raw, paths): + if isinstance(value, bool) or not isinstance(value, int) or value < 0: + return None, _ROOTS_ERROR + if not isinstance(path, str) or selected.get(value, path) != path: + return None, _ROOTS_ERROR + selected[value] = path + return dict(sorted(selected.items())), None + + +async def _scan_request(request): + """``({files, roots}, None)`` for a scan start, or ``(None, message)``. + + ``files`` is the scope ("new" or "all"); ``roots`` the selected source + indexes or None for every source. An empty body (also a chunked one with + no bytes), or one without ``scope``, asks for the default scope. + """ + default = {"files": DEFAULT_SCAN_SCOPE, "roots": None} if not request.can_read_body: - return DEFAULT_SCAN_SCOPE + return default, None text = await request.text() if not text.strip(): # e.g. a chunked POST that sent no bytes - return DEFAULT_SCAN_SCOPE + return default, None try: body = json.loads(text) except ValueError: - return None + return None, _SCOPE_ERROR if not isinstance(body, dict): - return None - scope = body.get("scope", DEFAULT_SCAN_SCOPE) - return scope if scope in SCAN_SCOPES else None + return None, _SCOPE_ERROR + files = body.get("scope", DEFAULT_SCAN_SCOPE) + if files not in SCAN_SCOPES: + return None, _SCOPE_ERROR + roots, error = _scan_roots(body) + if error: + return None, error + return {"files": files, "roots": roots}, None def busy_response(busy: JobBusy): @@ -64,6 +109,13 @@ def _summary_event(job): class JobRoutesMixin: """Mixin providing the /prompt_manager/jobs endpoints.""" + def _roots_still_listed(self, selected, output_dirs): + """Whether each ``{index: public path}`` still names that source.""" + return all( + index < len(output_dirs) and self._public_path(output_dirs[index]) == path + for index, path in selected.items() + ) + def _register_job_routes(self, routes): @routes.get("/prompt_manager/jobs") async def list_jobs_route(request): @@ -132,13 +184,37 @@ class JobRoutesMixin: """Start the metadata scan as a job (202), or 409 naming the busy job. The body's ``scope`` is "new" (the default: new and unresolved files) - or "all"; anything else is a 400. + or "all". ``roots``, when given, selects sources by their index in + the effective sources (``_get_all_output_dirs``), and ``paths`` names + the public path read for each. An index past them, or one whose + source now has another public path (the list changed since it was + read), is a 409 ``sources-changed`` and starts nothing. The job walks + the sources resolved here, so a later source change cannot move it. """ - files = await _scan_scope(request) - if files is None: - return _bad_request('The scan scope must be "new" or "all"') + scan, error = await _scan_request(request) + if error: + return _bad_request(error) + output_dirs = None + roots = None + if scan["roots"] is not None: + output_dirs = await self._run_in_executor(self._get_all_output_dirs) + listed = await self._run_in_executor( + self._roots_still_listed, scan["roots"], output_dirs + ) + if not listed: + return web.json_response( + { + "success": False, + "code": "sources-changed", + "error": SOURCES_CHANGED_ERROR, + }, + status=409, + ) + roots = list(scan["roots"]) try: - job = self._start_scan_job(files) + job = self._start_scan_job( + scan["files"], roots=roots, output_dirs=output_dirs + ) except JobBusy as busy: return busy_response(busy) return web.json_response({"success": True, "job": job.summary()}, status=202) diff --git a/py/api/settings_routes.py b/py/api/settings_routes.py index 258ad2a..4760e08 100644 --- a/py/api/settings_routes.py +++ b/py/api/settings_routes.py @@ -4,8 +4,15 @@ A save validates the payload, commits it to config.json through the config store and publishes it at runtime inside that commit; a save that cannot be written changes nothing at runtime and says so. ``_config_commit_failure`` is shared with the LoRA integration's save. + +GET also reports the image sources and the monitor (``sources.py``). A save +that changes the sources runs as a ``source_change`` registry hold, so it is +refused while a record-changing job (a scan) runs. ``monitoring_enabled`` +takes effect at the next restart. ``POST /prompt_manager/settings/validate-source`` +checks one path and writes nothing. """ +import contextlib import os from aiohttp import web @@ -15,7 +22,10 @@ from ..config_store import ( RevisionConflict, get_config_store, ) +from . import sources from .admin import _current_image_monitor +from .job_routes import busy_response +from .jobs import JobBusy try: from ...utils.validators import validate_result_timeout, validate_worker_threads @@ -23,13 +33,75 @@ except ImportError: from utils.validators import validate_result_timeout, validate_worker_threads -# Settings owned by POST /settings: config attribute -> (section, key). +# Settings owned by POST /settings: config attribute -> its keys in config.json. _SETTINGS_FILE_KEYS = { "RESULT_TIMEOUT": ("web_ui", "result_timeout"), "WEBUI_DISPLAY_MODE": ("web_ui", "webui_display_mode"), "INFINITE_SCROLL": ("web_ui", "infinite_scroll"), "WORKER_THREADS": ("performance", "worker_threads"), + "MONITORING_ENABLED": ("gallery", "monitoring", "enabled"), } +# Attributes that live on GalleryConfig; the others on PromptManagerConfig. +_GALLERY_SETTINGS = frozenset({"MONITORING_ENABLED"}) +SOURCE_CHANGE_JOB_KIND = "source_change" +SOURCE_CHANGE_JOB_TITLE = "Source change" + + +def _source_identity(path): + """The case-folded spelling two roots are the same directory by (Windows).""" + return os.path.normcase(path) + + +def _stat_identity(path): + """``(st_dev, st_ino)`` of an existing directory, or None. + + Catches case variants on a case-insensitive filesystem whose spelling + normcase keeps (macOS). + """ + try: + stat = os.stat(path) + except (OSError, ValueError): + return None + return (stat.st_dev, stat.st_ino) if stat.st_ino else None + + +# Refusals that a reconnected drive or folder can lift. +_RECONNECTABLE = frozenset({"not-found", "not-directory"}) + + +def _root_key(root): + """The directory a configured root names (resolved), else its spelling.""" + from ..config import GalleryConfig + + try: + return GalleryConfig.resolve_gallery_root(root) + except (OSError, TypeError, ValueError, AttributeError): + return root + + +def _request_error(message): + return {"code": "invalid-request", "error": message} + + +def _invalid_source(index, entry, code, reason): + """The 400 body for the source at ``index`` that cannot be used. + + Names it by its public spelling (computed as text: a refused path is + never resolved), so the page can mark that row and say why. + """ + source = sources.public_spelling(entry) + advice = ( + "Remove it or reconnect it, then save." + if code in _RECONNECTABLE + else "Remove it, then save." + ) + return { + "code": "invalid-source", + "source": source, + "index": index, + "reason": reason, + "error": f"{source} can't be used: {reason}. {advice}", + } def _section(config, name): @@ -47,10 +119,8 @@ def _settings_mutation(updates, new_roots): def mutate(config): for name, value in updates.items(): - section, key = _SETTINGS_FILE_KEYS[name] - merged = _section(config, section) - merged[key] = value - config[section] = merged + *sections, key = _SETTINGS_FILE_KEYS[name] + _set_nested(config, sections, key, value) if new_roots is not None: gallery = _section(config, "gallery") monitoring = _section(gallery, "monitoring") @@ -62,6 +132,16 @@ def _settings_mutation(updates, new_roots): return mutate +def _set_nested(config, sections, key, value): + """``config[s1][s2]...[key] = value``, each section a fresh dict.""" + if not sections: + config[key] = value + return + merged = _section(config, sections[0]) + _set_nested(merged, sections[1:], key, value) + config[sections[0]] = merged + + async def _read_json(request): """Parse a JSON object body through the package's capped reader.""" from . import _read_json_body @@ -81,6 +161,10 @@ class SettingsRoutesMixin: async def save_settings_route(request): return await self.save_settings(request) + @routes.post("/prompt_manager/settings/validate-source") + async def validate_source_route(request): + return await self.validate_source(request) + async def get_settings(self, request): """Get current settings.""" try: @@ -90,11 +174,15 @@ class SettingsRoutesMixin: self._public_path(d) for d in GalleryConfig.MONITORING_DIRECTORIES ] revision = await self._run_in_executor(self._config_revision) + source_status, monitor = await self._run_in_executor(self._source_status) return web.json_response( { "success": True, **revision, + "sources": source_status, + "monitor": monitor, "settings": { + "monitoring_enabled": GalleryConfig.MONITORING_ENABLED, "result_timeout": PromptManagerConfig.RESULT_TIMEOUT, "webui_display_mode": PromptManagerConfig.WEBUI_DISPLAY_MODE, "infinite_scroll": PromptManagerConfig.INFINITE_SCROLL, @@ -115,6 +203,61 @@ class SettingsRoutesMixin: status=500, ) + def _source_status(self): + """``(sources, monitor)`` of GET /settings (blocking).""" + from ..config import GalleryConfig + + output_dirs, auto_detected = self._output_dirs_with_origin() + return sources.describe_sources( + configured=list(GalleryConfig.MONITORING_DIRECTORIES), + output_dirs=output_dirs, + auto_detected=auto_detected, + monitor=_current_image_monitor(), + enabled=GalleryConfig.MONITORING_ENABLED, + ) + + async def validate_source(self, request): + """Check one source path ``{path}``; writes nothing. + + 200 ``{success, valid, label, path, code, reason}``: a valid source + answers its label and public path, an invalid one its code and reason + only. A body without a string ``path`` is a 400 ``invalid-request``. + """ + data, error_response = await _read_json(request) + if error_response is not None: + too_large = error_response.status == 413 + return web.json_response( + { + "success": False, + "code": "too-large" if too_large else "invalid-request", + "error": ( + "Request body too large" + if too_large + else "Request body must be a JSON object" + ), + }, + status=error_response.status, + ) + path = data.get("path") + if not isinstance(path, str): + return web.json_response( + { + "success": False, + "code": "invalid-request", + "error": "path must be a string", + }, + status=400, + ) + try: + result = await self._run_in_executor(sources.validate_source, path) + except Exception: + self.logger.exception("Source validation failed") + return web.json_response( + {"success": False, "code": None, "error": "Failed to check the source"}, + status=500, + ) + return web.json_response({"success": True, **result}) + def _monitored_directories(self): """Directories the running image monitor watches, else the configured roots.""" from ..config import GalleryConfig @@ -151,9 +294,15 @@ class SettingsRoutesMixin: {"success": False, "error": str(ve)}, status=400 ) - new_roots, error = self._parse_gallery_roots(data) + # Off the loop: an accepted root is resolved (filesystem access). + new_roots, error = await self._run_in_executor( + self._parse_gallery_roots, data + ) if error: - return web.json_response({"success": False, "error": error}, status=400) + return web.json_response({"success": False, **error}, status=400) + roots_change = new_roots is not None and await self._run_in_executor( + self._roots_change, new_roots + ) published = {} @@ -162,15 +311,30 @@ class SettingsRoutesMixin: # they were written, so the runtime ends on the last one. published.update(self._publish_settings(updates, new_roots)) - try: - _saved, revision = await self._run_in_executor( - get_config_store().commit, - _settings_mutation(updates, new_roots), - expected_revision=expected_revision, - publish=publish, - ) - except (ConfigUnreadable, RevisionConflict, OSError) as exc: - return self._config_commit_failure(exc) + async with contextlib.AsyncExitStack() as stack: + hold = None + if roots_change: + try: + hold = await stack.enter_async_context( + self._jobs.hold( + SOURCE_CHANGE_JOB_KIND, + SOURCE_CHANGE_JOB_TITLE, + admission="records", + ) + ) + except JobBusy as busy: + return busy_response(busy) + try: + _saved, revision = await self._run_in_executor( + get_config_store().commit, + _settings_mutation(updates, new_roots), + expected_revision=expected_revision, + publish=publish, + ) + except (ConfigUnreadable, RevisionConflict, OSError) as exc: + if hold is not None: + hold.finish("failed") + return self._config_commit_failure(exc) return web.json_response( { @@ -196,10 +360,11 @@ class SettingsRoutesMixin: as ``runtime_applied: false`` with the fields that need a restart; nothing is retried or rolled back. Never raises. """ - from ..config import PromptManagerConfig + from ..config import GalleryConfig, PromptManagerConfig for name, value in updates.items(): - setattr(PromptManagerConfig, name, value) + owner = GalleryConfig if name in _GALLERY_SETTINGS else PromptManagerConfig + setattr(owner, name, value) restart_fields = [] runtime_applied = True if new_roots is not None: @@ -212,11 +377,40 @@ class SettingsRoutesMixin: ) runtime_applied = False restart_fields.append("gallery_root_paths") + if "MONITORING_ENABLED" in updates: + # The monitor starts with the server: a restart applies the setting + # unless the observer already matches it. + if updates["MONITORING_ENABLED"] != self._observer_alive(): + restart_fields.append("monitoring_enabled") return { "runtime_applied": runtime_applied, "restart_required_fields": restart_fields, } + @staticmethod + def _roots_change(new_roots): + """Whether a save changes the sources (blocking: it resolves them). + + Compared as directories, not spellings: the legacy page resends the + roots resolved, while config.json may hold them as hand-written + (relative, another case), and an unchanged resend is no change. + """ + from ..config import GalleryConfig + + if new_roots is None: + return False + return [_root_key(root) for root in new_roots] != [ + _root_key(root) for root in GalleryConfig.MONITORING_DIRECTORIES + ] + + @staticmethod + def _observer_alive(): + """Whether the image monitor's observer is running now. Never raises.""" + try: + return sources.monitor_state(_current_image_monitor())[1] + except Exception: + return False + def _config_commit_failure(self, exc): """The response for a config commit that wrote nothing.""" not_saved = {"success": False, "persisted": False, "runtime_applied": False} @@ -283,6 +477,10 @@ class SettingsRoutesMixin: if not isinstance(data["infinite_scroll"], bool): raise ValueError("infinite_scroll must be true or false") updates["INFINITE_SCROLL"] = data["infinite_scroll"] + if "monitoring_enabled" in data: + if not isinstance(data["monitoring_enabled"], bool): + raise ValueError("monitoring_enabled must be true or false") + updates["MONITORING_ENABLED"] = data["monitoring_enabled"] if "worker_threads" in data: updates["WORKER_THREADS"] = validate_worker_threads( data["worker_threads"], PromptManagerConfig.max_worker_threads() @@ -295,15 +493,21 @@ class SettingsRoutesMixin: Accepts ``gallery_root_paths`` (list, preferred) or the legacy single ``gallery_root_path`` string. Blank entries are dropped. + Each entry passes the same checks as validate-source + (``sources.source_problem``): the form, then containment as text, + before anything is resolved. Blocking (an accepted root is resolved). + Returns: (roots, error): ``roots`` is the validated list, or ``None`` when - the payload carries no gallery root key; ``error`` is a message - when validation failed (and ``roots`` is then ``None``). + the payload carries no gallery root key; ``error`` is the coded + body of the 400 when validation failed (``roots`` is then None). + An unusable source is ``invalid-source`` naming its public path + and its position in the list. """ if "gallery_root_paths" in data: raw = data["gallery_root_paths"] if not isinstance(raw, list): - return None, "gallery_root_paths must be a list" + return None, _request_error("gallery_root_paths must be a list") elif "gallery_root_path" in data: raw = [data["gallery_root_path"]] else: @@ -312,27 +516,38 @@ class SettingsRoutesMixin: from ..config import GalleryConfig roots = [] - for entry in raw: + seen = set() + for index, entry in enumerate(raw): if not isinstance(entry, str): - return None, "Gallery root paths must be strings" + return None, _request_error("Gallery root paths must be strings") entry = entry.strip() if not entry: continue - ok, reason = GalleryConfig.validate_gallery_root(entry) - if not ok: - return ( - None, - f"Invalid gallery root '{os.path.basename(entry)}': {reason}", - ) - roots.append(GalleryConfig.resolve_gallery_root(entry)) + code, reason = sources.source_problem(entry) + if code is not None: + return None, _invalid_source(index, entry, code, reason) + root = GalleryConfig.resolve_gallery_root(entry) + keys = {("path", _source_identity(root)), ("file", _stat_identity(root))} + keys.discard(("file", None)) + if keys & seen: + continue # another spelling of a root already listed + seen |= keys + roots.append(root) return roots, None def _apply_gallery_roots(self, roots): - """Install validated gallery roots; returns True when they changed.""" + """Install validated gallery roots; returns True when they changed. + + Another spelling of the same directories installs the resolved + spelling and is no change (nothing to restart for). + """ from ..config import GalleryConfig if roots == list(GalleryConfig.MONITORING_DIRECTORIES): return False + if not self._roots_change(roots): + GalleryConfig.MONITORING_DIRECTORIES = roots + return False GalleryConfig.MONITORING_DIRECTORIES = roots self._cached_output_dir = None self.reset_gallery_roots_cache() diff --git a/py/api/sources.py b/py/api/sources.py new file mode 100644 index 0000000..90fb18c --- /dev/null +++ b/py/api/sources.py @@ -0,0 +1,280 @@ +"""Image sources as System › Sources & scans shows them (#249 S4). + +- ``source_form_error`` is the lexical check a submitted source path passes + before any filesystem access: no control characters, no ``..`` part, and + only the native path forms (on Windows a drive or a UNC share, never the + ``\\\\?\\`` and ``\\\\.\\`` namespaces, a drive-relative path, an alternate + data stream, a device name or a part Windows would silently trim; on POSIX + no Windows drive or backslash form). +- ``source_problem`` is the one refusal every caller answers (validate-source + and the settings save): the form check, then + ``GalleryConfig.gallery_root_problem``, which decides containment as text + before it touches the filesystem. +- ``describe_sources`` (blocking: it resolves paths) reports the configured + sources with their validity, the effective sources with their listing + index and watch state, and the image monitor's state. Every path in it is + a public path (``_public_path``), never an absolute one. + +A source's watch state is ``watching`` when the monitor's observer is alive, +scheduled that directory and monitoring is on; ``stopping`` when it still +watches it but monitoring was switched off (it stops at the next restart); +``manual`` when monitoring is off or this process has no monitor; else +``restart_pending`` (a source added since the monitor started, monitoring +switched on, or an observer that stopped). +""" + +import logging +import ntpath +import os +import re + +logger = logging.getLogger("prompt_manager.api") + +MAX_SOURCE_PATH = 4096 # characters +FORM_REASONS = { + "empty": "Gallery root must be a non-empty path", + "invalid-form": "Gallery root is not a folder path this server can use", + "traversal": "Gallery root must not contain '..'", +} +_DRIVE = re.compile(r"[A-Za-z]:") +_POSIX_FOREIGN = re.compile(r"^[A-Za-z]:") +# Reserved device names: Windows opens the device for any folder of that name. +_DEVICE_NAME = re.compile(r"^(con|prn|aux|nul|com[0-9¹²³]|lpt[0-9¹²³])(\..*)?$", re.I) +# folder_paths getters and the label of the directory each one reports. +_COMFYUI_LABELS = ( + ("get_output_directory", "ComfyUI output"), + ("get_input_directory", "ComfyUI input"), + ("get_temp_directory", "ComfyUI temp"), + ("get_user_directory", "ComfyUI user"), +) +WATCH_STATES = ("watching", "stopping", "manual", "restart_pending") + + +def source_form_error(text, *, windows=None): + """The form code that refuses ``text`` as a source path, or None. + + ``windows`` selects the path rules (default: this OS). No filesystem + access happens here. + """ + if windows is None: + windows = os.name == "nt" + if not isinstance(text, str) or len(text) > MAX_SOURCE_PATH: + return "invalid-form" + if any(ord(char) < 32 or ord(char) == 127 for char in text): + return "invalid-form" + text = text.strip() + if not text: + return "empty" + return _windows_form_error(text) if windows else _posix_form_error(text) + + +def _posix_form_error(text): + if "\\" in text or _POSIX_FOREIGN.match(text): + return "invalid-form" # a Windows path names nothing on this server + return "traversal" if ".." in text.split("/") else None + + +def _windows_form_error(text): + portable = text.replace("/", "\\") + if portable.startswith(("\\\\?\\", "\\\\.\\")): + return "invalid-form" # verbatim and device namespaces + if portable.startswith("\\\\"): + drive, rest = ntpath.splitdrive(portable) + share = drive[2:].split("\\") + if len(share) != 2 or any(part in ("", ".", "..") for part in share): + return "invalid-form" + else: + drive, rest = ntpath.splitdrive(portable) + if drive and (not _DRIVE.fullmatch(drive) or not rest.startswith("\\")): + return "invalid-form" # "C:folder" is relative to that drive's cwd + if ":" in rest: + return "invalid-form" # an alternate data stream + parts = rest.split("\\") + if ".." in parts: + return "traversal" + for part in parts: + if part in ("", "."): + continue + if part[-1] in ". " or _DEVICE_NAME.match(part): + return "invalid-form" + return None + + +def source_problem(text): + """``(code, reason)`` that refuses ``text`` as a source, else ``(None, "")``. + + The form check runs first and touches nothing; then containment is + decided as text before any filesystem access (see + ``GalleryConfig.gallery_root_problem``). Every caller answers alike. + """ + from ..config import GalleryConfig + + code = source_form_error(text) + if code is not None: + return code, FORM_REASONS[code] + return GalleryConfig.gallery_root_problem(text) + + +def public_spelling(text): + """How a refused source is named in a response, computed as text only. + + Relative to the path anchor it lies under (its public path), else its + last part: the caller sent the path, so naming it reveals nothing, and a + refused path is never resolved to name it. + """ + from ..config import GalleryConfig, _lexical_path, _lexically_inside + + text = str(text).strip() + try: + lexical = os.path.normpath(text) + key = _lexical_path(text) # the same length as ``lexical`` + if not os.path.isabs(text): + return lexical.replace("\\", "/") + for anchor in GalleryConfig.path_anchors(): + if _lexically_inside(key, [anchor]): + rest = lexical[len(anchor.rstrip(os.sep)) :].lstrip(os.sep) + return rest.replace(os.sep, "/") or "." + return os.path.basename(lexical) or lexical + except (TypeError, ValueError): + return "" + + +def identity(path): + """The comparison key of a directory: resolved and case-folded (Windows).""" + return os.path.normcase(os.path.realpath(str(path))) + + +def _comfyui_labels(): + """``{identity: label}`` of the directories ComfyUI reports.""" + try: + import folder_paths + except ImportError: + return {} + labels = {} + for getter_name, label in _COMFYUI_LABELS: + getter = getattr(folder_paths, getter_name, None) + if getter is None: + continue + try: + directory = getter() + except Exception: + continue + if isinstance(directory, str) and directory: + labels.setdefault(identity(directory), label) + return labels + + +def source_label(path, labels=None): + """ "ComfyUI output" for ComfyUI's own directories, else the folder name.""" + if labels is None: + labels = _comfyui_labels() + label = labels.get(identity(path)) + if label: + return label + return os.path.basename(os.path.normpath(str(path))) or str(path) + + +def _configured(entry, labels, public_path): + from ..config import GalleryConfig + + code, reason = GalleryConfig.gallery_root_problem(entry) + try: + resolved = GalleryConfig.resolve_gallery_root(entry) + except (OSError, TypeError, ValueError): + resolved = str(entry) + return { + "label": source_label(resolved, labels), + "path": public_path(resolved), + "valid": code is None, + "code": code, + "reason": reason or None, + } + + +def monitor_state(monitor): + """``(present, observer_alive, scheduled directories)`` of the image monitor.""" + if monitor is None: + return False, False, [] + status = monitor.get_status() + scheduled = [str(d) for d in status.get("monitored_directories") or []] + return True, bool(status.get("observer_alive")), scheduled + + +def watch_state(path, *, present, alive, scheduled_keys, enabled): + """One of ``WATCH_STATES`` (see the module docstring).""" + if alive and identity(path) in scheduled_keys: + return "watching" if enabled else "stopping" + if not present or not enabled: + return "manual" + return "restart_pending" + + +def describe_sources(*, configured, output_dirs, auto_detected, monitor, enabled): + """``(sources, monitor)`` for GET /settings (blocking; public paths only). + + ``configured`` is the configured roots, ``output_dirs`` and + ``auto_detected`` what ``_output_dirs_with_origin`` returned, ``monitor`` + the running ImageMonitor or None, ``enabled`` the monitoring setting. + """ + from . import _public_path + + labels = _comfyui_labels() + present, alive, scheduled = monitor_state(monitor) + scheduled_keys = {identity(path) for path in scheduled} + effective = [ + { + "label": source_label(path, labels), + "path": _public_path(path), + "root_index": index, + "watch": watch_state( + path, + present=present, + alive=alive, + scheduled_keys=scheduled_keys, + enabled=enabled, + ), + } + for index, path in enumerate(output_dirs) + ] + return ( + { + "configured": [ + _configured(entry, labels, _public_path) for entry in configured + ], + "effective": effective, + "auto_detected": bool(auto_detected), + }, + { + "present": present, + "observer_alive": alive, + "scheduled": [_public_path(path) for path in scheduled], + }, + ) + + +def validate_source(text): + """``{valid, label, path, code, reason}`` for one submitted path (blocking). + + Writes nothing. An invalid path answers its code and reason with no + label or path, so a response never echoes what was typed. + """ + from ..config import GalleryConfig + from . import _public_path + + code, reason = source_problem(text) + if code is not None: + return { + "valid": False, + "label": None, + "path": None, + "code": code, + "reason": reason, + } + resolved = GalleryConfig.resolve_gallery_root(text) + return { + "valid": True, + "label": source_label(resolved), + "path": _public_path(resolved), + "code": None, + "reason": None, + } diff --git a/py/config.py b/py/config.py index a228524..c6fa719 100644 --- a/py/config.py +++ b/py/config.py @@ -31,7 +31,6 @@ routes = server_instance.routes extension_uri = None # Will be set in __init__.py import os # noqa: E402 -from pathlib import Path # noqa: E402 from typing import Any, Dict, List, Optional, Tuple # noqa: E402 # Import logging system @@ -51,6 +50,14 @@ config_logger = get_logger("prompt_manager.config") # may be used as gallery roots in addition to ComfyUI's own directories. EXTRA_GALLERY_ROOTS_ENV = "PROMPT_MANAGER_EXTRA_GALLERY_ROOTS" +# The one answer for every path outside the allowed folders. The filesystem +# root and the home directory answer it too, so no caller learns the server's +# layout (home path, user name) from a refusal (#249 S4). +OUTSIDE_ALLOWED_REASON = ( + "Gallery root must be inside a ComfyUI directory (output, input, temp, user) " + f"or one listed in {EXTRA_GALLERY_ROOTS_ENV}" +) + # Environment variable overriding where config.json is read from and saved to. CONFIG_PATH_ENV = "PROMPT_MANAGER_CONFIG_PATH" @@ -89,6 +96,29 @@ def write_private_json(path: str, data: Any) -> None: os.chmod(path, 0o600) +def _lexical_path(path: str) -> str: + """``path`` normalised for comparison without touching the filesystem. + + normpath collapses ``.`` and ``..`` as text and normcase folds case on + Windows; neither opens, stats nor resolves anything (no SMB request for a + UNC path). A symlink is only seen by :func:`_canonical_path` later. + """ + return os.path.normcase(os.path.normpath(path)) + + +def _lexically_inside(path: str, parents: List[str]) -> bool: + """Whether the normalised ``path`` is one of ``parents`` or below one. + + Pure text on normalised spellings (:func:`_lexical_path` or + :func:`_canonical_path`): no filesystem access. + """ + sep = os.path.sep + return any( + path == parent or path.startswith(parent.rstrip(sep) + sep) + for parent in parents + ) + + def _is_filesystem_root(path: str) -> bool: """True for '/' on POSIX and drive roots such as 'C:\\' on Windows.""" return os.path.dirname(path) == path @@ -268,7 +298,12 @@ class GalleryConfig: """ monitoring = new_config.get("monitoring", {}) if "enabled" in monitoring: - cls.MONITORING_ENABLED = monitoring["enabled"] + if isinstance(monitoring["enabled"], bool): + cls.MONITORING_ENABLED = monitoring["enabled"] + else: # a hand-edited "false" would otherwise switch it on + config_logger.warning( + "Ignoring gallery monitoring enabled: expected true or false" + ) if "directories" in monitoring: directories = cls._validated_directories(monitoring["directories"]) if directories is not None: @@ -337,31 +372,56 @@ class GalleryConfig: Filesystem roots are never allowed as parents, even when listed in the environment, because that would re-open the whole disk. """ - parents = [] + return [canonical for canonical, _spelling in cls._allowed_parent_pairs()] + + @classmethod + def _allowed_parent_pairs(cls) -> List[Tuple[str, str]]: + """``(canonical, lexical spelling)`` of each allowed parent. + + Only trusted directories (ComfyUI's own and the environment's) are + resolved here; the spelling is how ComfyUI or the environment wrote + the directory, so a submitted path can be compared without resolving it. + """ + pairs = [] + seen = set() for candidate in _comfyui_directories() + _extra_gallery_roots(): try: canonical = _canonical_path(candidate) + spelling = _lexical_path(os.path.abspath(candidate)) except (OSError, ValueError): continue if _is_filesystem_root(canonical) or not os.path.isdir(canonical): continue - if canonical not in parents: - parents.append(canonical) - return parents + if canonical not in seen: + seen.add(canonical) + pairs.append((canonical, spelling)) + return pairs @classmethod def path_anchors(cls) -> List[str]: """Canonical directories that relative gallery paths are resolved against. The ComfyUI base directory comes first, followed by the parent of each - directory listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS, so an extra - root is addressed by its own name (``gallery/sub``) rather than by an - absolute path. The same anchors drive the public (relative) form of - paths in API responses. + ComfyUI directory outside it (``--output-directory D:\\Renders``) and of + each directory listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS, so such a + directory is addressed by its own name (``Renders/sub``) rather than by + an absolute path, and two folders of one name stay apart. The same + anchors drive the public (relative) form of paths in API responses. """ anchors = [] base = _comfyui_base_directory() candidates = [base] if base else [] + try: + base_key = _canonical_path(base) if base else None + except (OSError, ValueError): + base_key = None + for directory in _comfyui_directories(): + try: + canonical = _canonical_path(directory) + except (OSError, ValueError): + continue + if base_key is None or not _lexically_inside(canonical, [base_key]): + candidates.append(os.path.dirname(canonical)) candidates.extend(os.path.dirname(root) for root in _extra_gallery_roots()) for candidate in candidates: try: @@ -399,33 +459,69 @@ class GalleryConfig: (True, "") when the directory lies inside one of :meth:`allowed_gallery_parents`; otherwise (False, reason). """ + code, reason = cls.gallery_root_problem(path) + return code is None, reason + + @classmethod + def gallery_root_problem(cls, path: Any) -> Tuple[Optional[str], str]: + """``(code, reason)`` why ``path`` may not be a gallery root, else (None, ""). + + Codes: ``empty``, ``unresolvable``, ``outside-allowed``, ``not-found`` + and ``not-directory``. No reason names the path. + + Containment is decided twice. First lexically, with no filesystem + access at all: a path (UNC, ``\\\\?\\``, a device, anything) whose + normalised spelling lies outside every allowed parent answers + ``outside-allowed`` before it is resolved, so the server never opens + or stats it (no SMB connection, no existence or timing oracle). Only + then is it resolved and checked again, which catches a symlink out. + The filesystem root and the home directory answer ``outside-allowed`` + too: one answer for every caller (validate-source, the settings save, + config.json), and the detail goes to the log only. + """ if not isinstance(path, str) or not path.strip(): - return False, "Gallery root must be a non-empty path" + return "empty", "Gallery root must be a non-empty path" + + pairs = cls._allowed_parent_pairs() + spellings = [spelling for pair in pairs for spelling in pair] + try: + lexical = cls._lexical_candidates(path) + except (TypeError, ValueError): + return "unresolvable", "Gallery root could not be resolved" + if not any(_lexically_inside(candidate, spellings) for candidate in lexical): + return "outside-allowed", OUTSIDE_ALLOWED_REASON try: canonical = cls.resolve_gallery_root(path) except (OSError, ValueError): - return False, "Gallery root could not be resolved" + return "unresolvable", "Gallery root could not be resolved" if _is_filesystem_root(canonical): - return False, "Gallery root cannot be a filesystem root" + config_logger.info("Gallery root refused: a filesystem root") + return "outside-allowed", OUTSIDE_ALLOWED_REASON if canonical == _canonical_path(os.path.expanduser("~")): - return False, "Gallery root cannot be the home directory" + config_logger.info("Gallery root refused: the home directory") + return "outside-allowed", OUTSIDE_ALLOWED_REASON + if not _lexically_inside(canonical, [canonical for canonical, _ in pairs]): + return "outside-allowed", OUTSIDE_ALLOWED_REASON if not os.path.exists(canonical): - return False, "Gallery root does not exist" + return "not-found", "Gallery root does not exist" if not os.path.isdir(canonical): - return False, "Gallery root is not a directory" + return "not-directory", "Gallery root is not a directory" + return None, "" - candidate = Path(canonical) - for parent in cls.allowed_gallery_parents(): - if candidate.is_relative_to(Path(parent)): - return True, "" + @classmethod + def _lexical_candidates(cls, path: str) -> List[str]: + """Every spelling ``path`` may resolve to, normalised as text (no IO). - return ( - False, - "Gallery root must be inside a ComfyUI directory " - f"(output, input, temp, user) or one listed in {EXTRA_GALLERY_ROOTS_ENV}", - ) + An absolute path is itself; a relative one is joined to each + :meth:`path_anchors` entry, as :meth:`resolve_gallery_root` tries them. + """ + path = path.strip() + if os.path.isabs(path): + return [_lexical_path(path)] + anchors = cls.path_anchors() + return [_lexical_path(os.path.join(anchor, path)) for anchor in anchors] class IntegrationConfig: diff --git a/tests/browser/system-concept.spec.mjs b/tests/browser/system-concept.spec.mjs index 761134b..9317a19 100644 --- a/tests/browser/system-concept.spec.mjs +++ b/tests/browser/system-concept.spec.mjs @@ -533,3 +533,246 @@ test('appearance comparison detects mutations', async ({ page, request }) => { await undoToken(); expect(diffInventory(expected, await measure())).toEqual([]); }); + +/** + * Runs in the page: Sources & scans (app.js 679–681): the optional notice, then the "Image sources" surface with its + * panel heading, the source list, the two setting rows and the operations, with the concept's geometry and tokens. + */ +function measureSources(mainSelector) { + const layout = document.querySelector(`${mainSelector} .settings-layout`); + const css = (node, ...keys) => node ? Object.fromEntries(keys.map(key => [key, getComputedStyle(node)[key]])) : null; + const name = node => [node.localName, ...[...node.classList].sort()].join('.'); + const text = node => node?.textContent.trim() ?? null; + const button = node => ({ text: text(node), className: node.className, + ...css(node, 'backgroundColor', 'color', 'fontSize', 'paddingTop', 'paddingLeft', 'borderTopColor') }); + const operation = row => ({ tree: [...row.children].map(name), title: text(row.querySelector('h3')), + description: text(row.querySelector('p')), actions: [...row.querySelectorAll('.row-actions > button')].map(button) }); + const notice = layout.querySelector(':scope > .notice'); + const surface = layout.querySelector(':scope > section.surface'); + const heading = surface.querySelector(':scope > .panel-heading'); + const list = surface.querySelector(':scope > #source-list'); + const control = row => { + const toggle = row.querySelector(':scope > * input[type="checkbox"]'); + if (toggle) return { kind: 'switch', id: toggle.id, wrapper: name(toggle.parentElement), checked: toggle.checked, + ...css(toggle.parentElement, 'width', 'height') }; + const input = row.querySelector(':scope > input'); + return { kind: `input.${input.type}`, id: input.id, min: input.min, max: input.max, value: input.value, + ...css(input, 'width', 'accentColor') }; + }; + const rows = [...surface.querySelectorAll(':scope > .setting-row')]; + return { + layout: [...layout.children].map(name), + notice: notice && { tree: [...notice.children].map(name), + text: [...notice.querySelectorAll(':scope > div > *')].map(node => [node.localName, text(node)]), + actions: [...notice.querySelectorAll(':scope > .row-actions > button')].map(button), + ...css(notice, 'display', 'alignItems', 'columnGap', 'borderTopColor', 'backgroundColor', 'paddingTop', 'marginBottom') }, + surface: { tree: [...surface.children].map(name), intro: text(surface.querySelector(':scope > p')), + ...css(surface, 'paddingTop', 'paddingLeft', 'backgroundColor', 'borderTopColor', 'borderTopLeftRadius') }, + heading: { tree: [...heading.children].map(name), title: text(heading.querySelector('h2')), + action: button(heading.querySelector('button')), ...css(heading, 'display', 'alignItems', 'justifyContent', 'marginBottom'), + h2: css(heading.querySelector('h2'), 'fontSize', 'fontWeight', 'color') }, + sources: [...list.children].map(operation), + rows: rows.map(row => ({ tree: [...row.children].map(name), text: [...row.firstElementChild.children].map(node => [node.localName, text(node), + node.localName === 'label' ? node.htmlFor : null]), control: control(row) })), + operations: [...surface.querySelectorAll(':scope > .operation')].map(operation), + operation: css(list.querySelector('.operation'), 'display', 'alignItems', 'justifyContent', 'columnGap', 'paddingTop', + 'paddingBottom', 'borderTopWidth', 'borderTopColor'), + h3: css(list.querySelector('.operation h3'), 'fontSize', 'fontWeight', 'color'), + p: css(list.querySelector('.operation p'), 'fontSize', 'color', 'marginTop'), + row: css(rows[0], 'display', 'alignItems', 'justifyContent', 'columnGap', 'paddingTop', 'paddingBottom', 'borderTopColor'), + pageScroll: document.documentElement.scrollWidth > document.documentElement.clientWidth, + }; +} + +// D128: "watching until restart" after monitoring was switched off while the observer still runs. +const WATCH_TEXT = { watching: 'watching', stopping: 'watching until restart', manual: 'manual scan', restart_pending: 'restart pending' }; +const WATCHED = new Set(['watching', 'stopping']); +// D98: the operation Sources adds after the concept's own, as production renders it. +const REFRESH_FILES = { title: 'Refresh files', description: 'Read every source again to find files added or removed outside ComfyUI.', + label: 'Refresh' }; + +/** The concept's Sources with the recorded deviations applied, given the server's real settings and sources. */ +function mapSourcesConcept(reference, { settings, sources }) { + const [sample] = reference.sources; + const scan = sample.actions[0]; + return { + ...reference, + // The real sources replace the two samples (without the "sample path" note). D98: each configured one has Remove, + // a plain button after its scan. + sources: sources.effective.map(source => ({ ...sample, title: source.label, + description: `${source.path} · ${WATCH_TEXT[source.watch]}`, + actions: [{ ...scan, text: WATCHED.has(source.watch) ? 'Scan changes' : 'Scan folder' }, { ...scan, text: 'Remove' }] })), + rows: reference.rows.map(row => { + if (row.control.id === 'monitoring') return { ...row, control: { ...row.control, checked: settings.monitoring_enabled } }; + // D113: the range ends at the server's core count (the concept's fixed 16) and shows the real worker count. + return { ...row, control: { ...row.control, max: String(settings.cpu_count), value: String(settings.worker_threads) } }; + }), + // D98: "Refresh files" closes the surface. + surface: { ...reference.surface, tree: [...reference.surface.tree, 'div.operation'] }, + operations: [...reference.operations, { ...reference.operations[0], title: REFRESH_FILES.title, + description: REFRESH_FILES.description, actions: [{ ...reference.operations[0].actions[0], text: REFRESH_FILES.label }] }], + }; +} + +async function conceptSources(page, theme, { notice }) { + await openConcept(page, 'system', theme); + await page.locator('#main .section-nav').getByRole('button', { name: 'Sources & scans', exact: true }).click(); + await expect(page.locator('#main .panel-heading > h2')).toHaveText('Image sources'); + if (!notice) await page.locator('#main .notice').getByRole('button', { name: 'Later', exact: true }).click(); + await expect(page.locator('#main .settings-layout > .notice')).toHaveCount(notice ? 1 : 0); + await page.mouse.move(0, 0); + return page.evaluate(measureSources, '#main'); +} + +async function productionSources(page, theme, { notice }) { + // about:blank has no storage; the workspace page reads the dismissal. + if (!notice) await page.addInitScript(() => { if (location.protocol.startsWith('http')) localStorage.setItem('pm_update_notice_dismissed', '3.2.0'); }); + await openProduction(page, 'system/sources', theme); + await expect(page.locator('#workspace-main #source-list .operation')).toHaveCount(2); + await expect(page.locator('#workspace-main #workers')).toBeEnabled(); + await expect(page.locator('#workspace-main .settings-layout > .notice')).toHaveCount(notice ? 1 : 0); + await page.mouse.move(0, 0); + return page.evaluate(measureSources, '#workspace-main'); +} + +/** The fixture's sources as the concept shows its samples: the first one watched, the second not. */ +async function watchedSources(request) { + await expect(await request.post('/__test__/system/monitor', { data: { watching: ['output'] } })).toBeOK(); + return serverJson(request, '/prompt_manager/settings'); +} + +for (const theme of THEMES) for (const [width, height] of SIZES) for (const notice of [true, false]) { + const variant = notice ? 'with' : 'without'; + test(`sources matches the concept ${variant} the notice ${theme} ${width}x${height}`, async ({ page, request }, testInfo) => { + await page.setViewportSize({ width, height }); + const server = await watchedSources(request); + const concept = await conceptSources(page, theme, { notice }); + await page.screenshot({ path: testInfo.outputPath(`concept-sources-${variant}-notice-${theme}-${width}.png`), fullPage: true }); + const production = await productionSources(page, theme, { notice }); + await page.screenshot({ path: testInfo.outputPath(`production-sources-${variant}-notice-${theme}-${width}.png`), fullPage: true }); + expect(diffInventory(mapSourcesConcept(concept, server), production)).toEqual([]); + expect(production.pageScroll).toBe(false); + }); +} + +test('sources comparison detects mutations', async ({ page, request }) => { + await page.setViewportSize({ width: 1600, height: 1100 }); + const server = await watchedSources(request); + const concept = await conceptSources(page, 'dark', { notice: true }); + const expected = mapSourcesConcept(concept, server); + const production = await productionSources(page, 'dark', { notice: true }); + expect(diffInventory(expected, production)).toEqual([]); + // Unmapped, the concept differs only in the real sources, the D98 operation and the D113 range. + expect(diffInventory(concept, production).map(item => item.path.split('.').slice(0, 2).join('.')) + .filter((path, index, all) => all.indexOf(path) === index).sort()) + .toEqual(['operations.2', 'operations.length', 'rows.1', 'sources.0', 'sources.1', 'surface.tree'].sort()); + const measure = () => page.evaluate(measureSources, '#workspace-main'); + const paths = async () => diffInventory(expected, await measure()).map(item => item.path); + // A removed notice and a removed source row. + const undoNotice = await mutate(page, { remove: ['#workspace-main .settings-layout > .notice'] }); + expect(await paths()).toEqual(expect.arrayContaining(['layout.length', 'notice.tree.length'])); + await undoNotice(); + const undoRow = await mutate(page, { remove: ['#workspace-main #source-list > .operation:nth-child(2)'] }); + expect(await paths()).toEqual(expect.arrayContaining(['sources.length'])); + await undoRow(); + // A changed label and a changed button. + await page.evaluate(() => { document.querySelector('#workspace-main label[for="monitoring"]').textContent = 'Watch for outputs'; }); + expect(await paths()).toEqual(['rows.0.text.0.1']); + await page.evaluate(() => { document.querySelector('#workspace-main label[for="monitoring"]').textContent = 'Watch for new outputs'; }); + await page.evaluate(() => { document.querySelector('#workspace-main .panel-heading button').textContent = 'Add source'; }); + expect(await paths()).toEqual(['heading.action.text']); + await page.evaluate(() => { document.querySelector('#workspace-main .panel-heading button').textContent = '+ Add source'; }); + // A swapped token. + const undoToken = await mutate(page, { css: '#workspace-main .settings-layout { --line: rgb(255, 0, 0); }' }); + expect(await paths()).toEqual(expect.arrayContaining(['operation.borderTopColor'])); + await undoToken(); + expect(diffInventory(expected, await measure())).toEqual([]); +}); + +/** + * Runs in the page: the open "Add image source" dialog (the concept's `simpleInput`, app.js 1138 and 1497): its title, + * the body's tree, the field's label and input, and the actions, with the concept's geometry and tokens. + */ +function measureAddSource() { + const dialog = document.querySelector('dialog#modal[open]'); + const css = (node, ...keys) => node ? Object.fromEntries(keys.map(key => [key, getComputedStyle(node)[key]])) : null; + const name = node => [node.localName, ...[...node.classList].sort()].join('.'); + const text = node => node?.textContent.trim() ?? null; + const body = dialog.querySelector('#modal-body'); + const field = body.querySelector(':scope > .field'); + const label = field?.querySelector(':scope > label'); + const input = field?.querySelector(':scope > input'); + const actions = body.querySelector(':scope > .dialog-actions'); + const rect = dialog.getBoundingClientRect(); + return { + title: text(dialog.querySelector('#modal-title')), + tree: [...body.children].map(name), + field: field && { tree: [...field.children].map(name), ...css(field, 'display', 'flexDirection', 'rowGap', 'marginBottom') }, + label: label && { text: text(label), for: label.htmlFor, ...css(label, 'fontSize', 'fontWeight', 'color') }, + input: input && { id: input.id, type: input.type, value: input.value, + ...css(input, 'height', 'paddingLeft', 'fontSize', 'color', 'backgroundColor', 'borderTopColor', 'borderTopLeftRadius') }, + actions: { tree: [...actions.children].map(name), ...css(actions, 'display', 'justifyContent', 'columnGap', 'marginTop'), + buttons: [...actions.children].map(button => ({ text: text(button), className: button.className, + ...css(button, 'backgroundColor', 'color', 'fontSize', 'paddingTop', 'paddingLeft', 'borderTopColor') })) }, + width: Math.round(rect.width), + }; +} + +/** D127: the field is "Folder path", starts empty (no sample folder) and its action is "Add". */ +function mapAddSourceConcept(reference) { + const [cancel, apply] = reference.actions.buttons; + return { ...reference, label: { ...reference.label, text: 'Folder path' }, input: { ...reference.input, value: '' }, + actions: { ...reference.actions, buttons: [cancel, { ...apply, text: 'Add' }] } }; +} + +async function conceptAddSource(page, theme) { + await conceptSources(page, theme, { notice: false }); + await page.locator('#main .panel-heading').getByRole('button', { name: '+ Add source', exact: true }).click(); + await expect(page.locator('dialog#modal[open] #modal-title')).toHaveText('Add image source'); + await page.mouse.move(0, 0); + return page.evaluate(measureAddSource); +} + +async function productionAddSource(page, theme) { + await productionSources(page, theme, { notice: false }); + await page.locator('#workspace-main .panel-heading').getByRole('button', { name: '+ Add source', exact: true }).click(); + await expect(page.locator('dialog#modal[open] #modal-title')).toHaveText('Add image source'); + await page.mouse.move(0, 0); + return page.evaluate(measureAddSource); +} + +for (const theme of THEMES) for (const [width, height] of SIZES) { + test(`add source dialog matches the concept ${theme} ${width}x${height}`, async ({ page, request }, testInfo) => { + await page.setViewportSize({ width, height }); + await watchedSources(request); + const concept = await conceptAddSource(page, theme); + await page.screenshot({ path: testInfo.outputPath(`concept-add-source-${theme}-${width}.png`) }); + const production = await productionAddSource(page, theme); + await page.screenshot({ path: testInfo.outputPath(`production-add-source-${theme}-${width}.png`) }); + expect(diffInventory(mapAddSourceConcept(concept), production)).toEqual([]); + }); +} + +test('add source comparison detects mutations', async ({ page, request }) => { + await page.setViewportSize({ width: 1600, height: 1100 }); + await watchedSources(request); + const concept = await conceptAddSource(page, 'dark'); + const expected = mapAddSourceConcept(concept); + const production = await productionAddSource(page, 'dark'); + expect(diffInventory(expected, production)).toEqual([]); + // Unmapped, the concept differs only where D127 is recorded. + expect(diffInventory(concept, production).map(item => item.path).sort()) + .toEqual(['actions.buttons.1.text', 'input.value', 'label.text']); + const paths = async () => diffInventory(expected, await page.evaluate(measureAddSource)).map(item => item.path); + // A changed label, a swapped action order and a changed style. + await page.evaluate(() => { document.querySelector('dialog#modal[open] label[for="action-input"]').textContent = 'Folder'; }); + expect(await paths()).toEqual(['label.text']); + await page.evaluate(() => { document.querySelector('dialog#modal[open] label[for="action-input"]').textContent = 'Folder path'; }); + await page.evaluate(() => { const actions = document.querySelector('dialog#modal[open] .dialog-actions'); actions.prepend(actions.lastElementChild); }); + expect(await paths()).toEqual(expect.arrayContaining(['actions.buttons.0.text', 'actions.buttons.1.text'])); + await page.evaluate(() => { const actions = document.querySelector('dialog#modal[open] .dialog-actions'); actions.prepend(actions.lastElementChild); }); + const undoStyle = await mutate(page, { css: 'dialog#modal #action-input { border-color: rgb(255, 0, 0); } dialog#modal .dialog-actions { gap: 31px; }' }); + expect(await paths()).toEqual(expect.arrayContaining(['input.borderTopColor', 'actions.columnGap'])); + await undoStyle(); + expect(diffInventory(expected, await page.evaluate(measureAddSource))).toEqual([]); +}); diff --git a/tests/browser/workspace-system-sources.spec.mjs b/tests/browser/workspace-system-sources.spec.mjs new file mode 100644 index 0000000..6450344 --- /dev/null +++ b/tests/browser/workspace-system-sources.spec.mjs @@ -0,0 +1,401 @@ +import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; +import { test, expect } from './fixtures.mjs'; +import { seedConcept } from './concept-inventory.mjs'; + +// System › Sources & scans (#249 System S4 Task 11) on the concept-shaped real fixture, whose two sources are this +// server's `output` and `input` folders: the source list, monitoring and worker rows read GET /settings (S4 Task 9), +// Add/Remove edit the host draft behind the one Save row, and every scan starts the session job through the shared +// "Review metadata scan" (D55), scoped to one source from its row (S4 Task 10). The hooks are workspace_system_fixture.py's. +const workspace = '/prompt_manager/workspace'; +const SETTINGS_URL = '/prompt_manager/settings'; +const NOTICE_KEY = 'pm_update_notice_dismissed'; +const OUTSIDE_REASON = 'Gallery root must be inside a ComfyUI directory (output, input, temp, user) or one listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS'; + +test.beforeEach(async ({ page, request, browserName }) => { + await startCoverage(page, browserName); + await seedConcept(request); + await expect(await request.post('/__test__/system/reset')).toBeOK(); + await expect(await request.post('/__test__/jobs/restart')).toBeOK(); + page.systemErrors = []; + page.on('pageerror', error => page.systemErrors.push(error.message)); +}); + +test.afterEach(async ({ page, request, browserName }) => { + await saveCoverage(page, browserName); + await request.post('/__test__/release'); + await request.post('/__test__/jobs/restart'); + await request.post('/__test__/system/reset'); + expect(page.systemErrors).toEqual([]); +}); + +const toast = page => page.locator('#workspace-toast'); +const layoutOf = page => page.locator('#workspace-main .settings-layout'); +const sourceList = page => layoutOf(page).locator('#source-list'); +const statusOf = page => layoutOf(page).locator('#settings-status'); +const saveButton = page => layoutOf(page).getByRole('button', { name: 'Save preferences', exact: true }); +const dialog = page => page.locator('dialog#modal[open]'); + +async function serverJson(request, url) { + const response = await request.get(url); + await expect(response).toBeOK(); + return response.json(); +} + +/** Open Sources with the update notice dismissed (or shown, `notice: true`) once the source list has loaded. */ +async function openSources(page, { notice = false } = {}) { + if (!notice) await page.addInitScript(key => { if (location.protocol.startsWith('http')) localStorage.setItem(key, '3.2.0'); }, NOTICE_KEY); + await page.goto(`${workspace}#/system/sources`); + await expect(sourceList(page).locator('.operation').first()).toBeVisible(); + await expect(layoutOf(page).locator('#workers')).toBeEnabled(); + return layoutOf(page); +} + +/** The source list's rows: `[h3, p, [buttons]]`, then its note line when there is one. */ +function rowsOf(page) { + return sourceList(page).evaluate(list => [...list.children].map(row => row.matches('.operation') + ? [row.querySelector('h3').textContent, row.querySelector('p').textContent, + [...row.querySelectorAll('.row-actions > button')].map(button => button.textContent)] + : [row.localName, row.className, row.textContent])); +} + +const sourceRow = (page, title) => sourceList(page).locator('.operation').filter({ has: page.getByRole('heading', { name: title, exact: true }) }); + +test('notice Later persists the 3.2.0 dismissal (pm_update_notice_dismissed) without scanning', async ({ page, request }) => { + const writes = []; + page.on('request', item => { if (!['GET', 'HEAD', 'OPTIONS'].includes(item.method())) writes.push(item.url()); }); + const layout = await openSources(page, { notice: true }); + const notice = layout.locator(':scope > .notice'); + await expect(notice.locator('h2')).toHaveText('Bring older images into your prompt history'); + await expect(notice.locator('p')).toHaveText('The schema upgrades automatically. An optional metadata scan links existing positives and negatives without regenerating all thumbnails.'); + await expect(notice.locator('.row-actions > button')).toHaveText(['Later', 'Review scan']); + await notice.getByRole('button', { name: 'Later', exact: true }).click(); + await expect(layout.locator(':scope > .notice')).toHaveCount(0); + expect(await page.evaluate(key => localStorage.getItem(key), NOTICE_KEY)).toBe('3.2.0'); + await expect(dialog(page)).toHaveCount(0); + // The dismissal is the legacy admin page's own, so neither page shows the notice again. + await page.reload(); + await expect(sourceList(page).locator('.operation').first()).toBeVisible(); + await expect(layout.locator(':scope > .notice')).toHaveCount(0); + expect(writes).toEqual([]); + expect((await serverJson(request, '/prompt_manager/jobs')).jobs).toEqual([]); +}); + +test('Review scan opens the shared Review metadata scan with all sources', async ({ page, request }) => { + const layout = await openSources(page, { notice: true }); + // The notice's Review scan dismisses it, as the legacy page's does, and reviews every source. + await layout.locator(':scope > .notice').getByRole('button', { name: 'Review scan', exact: true }).click(); + await expect(dialog(page).locator('#modal-title')).toHaveText('Review metadata scan'); + await expect(dialog(page).locator('dl.meta-grid > div > dd')).toHaveText(['2 output folders', 'New and unresolved files']); + expect(await page.evaluate(key => localStorage.getItem(key), NOTICE_KEY)).toBe('3.2.0'); + await dialog(page).getByRole('button', { name: 'Cancel', exact: true }).click(); + await expect(layout.locator(':scope > .notice')).toHaveCount(0); + // Enrich existing metadata is the same review. + const enrich = layout.locator('.operation').filter({ hasText: 'Enrich existing metadata' }); + await expect(enrich.locator('p')).toHaveText('Resolve both prompts, deduplicate recipes, preserve uncertain links.'); + await enrich.getByRole('button', { name: 'Review scan', exact: true }).click(); + await expect(dialog(page).locator('dl.meta-grid > div > dd')).toHaveText(['2 output folders', 'New and unresolved files']); + await dialog(page).getByRole('button', { name: 'Cancel', exact: true }).click(); + expect((await serverJson(request, '/prompt_manager/jobs')).jobs).toEqual([]); +}); + +test('Scan changes opens it with that one source ("1 output folder") and starts a scoped job that opens in Activity', async ({ page, request }) => { + await expect(await request.post('/__test__/system/monitor', { data: { watching: ['input'] } })).toBeOK(); + const bodies = []; + page.on('request', item => { if (item.method() === 'POST' && item.url().endsWith('/prompt_manager/jobs/scan')) bodies.push(item.postDataJSON()); }); + await openSources(page); + const input = sourceRow(page, 'ComfyUI input'); + await expect(input.locator('p')).toHaveText('input · watching'); + await input.getByRole('button', { name: 'Scan changes', exact: true }).click(); + await expect(dialog(page).locator('#modal-title')).toHaveText('Review metadata scan'); + await expect(dialog(page).locator('dl.meta-grid > div > dd')).toHaveText(['1 output folder', 'New and unresolved files']); + await dialog(page).getByRole('button', { name: 'Start scan', exact: true }).click(); + await expect(page).toHaveURL(/#\/activity\/[0-9a-f]{32}$/); + // The toast says it started (or, for a scan this quick, that it already finished); Activity shows its progress. + await expect(toast(page)).toHaveText(/^Scan (started\.|finished · )/); + expect(bodies).toEqual([{ scope: 'new', roots: [1], paths: ['input'] }]); + const id = page.url().split('/').pop(); + // The job walks only that source (the count is recorded once the walk has listed it). + await expect.poll(async () => (await serverJson(request, `/prompt_manager/jobs/${id}`)).job.scope) + .toEqual({ files: 'new', roots: [1], output_folders: 1 }); + await expect(page.locator('#workspace-main h1')).toHaveText('Scan for metadata'); +}); + +test('Add source validates on the server, then marks the host draft dirty; Save persists', async ({ page, request }) => { + // A folder inside ComfyUI's output folder that is not a source yet. + await expect(await request.post('/__test__/system/unthumbnailed', { data: { count: 1 } })).toBeOK(); + const layout = await openSources(page); + expect(await rowsOf(page)).toEqual([ + ['ComfyUI output', 'output · manual scan', ['Scan folder', 'Remove']], + ['ComfyUI input', 'input · manual scan', ['Scan folder', 'Remove']], + ]); + await expect(layout.locator('.panel-heading > h2')).toHaveText('Image sources'); + const add = layout.locator('.panel-heading').getByRole('button', { name: '+ Add source', exact: true }); + await add.click(); + await expect(dialog(page).locator('#modal-title')).toHaveText('Add image source'); + await expect(dialog(page).locator('label[for="action-input"]')).toHaveText('Folder path'); + await expect(dialog(page).locator('#action-input')).toHaveValue(''); + await expect(dialog(page).locator('.dialog-actions > button')).toHaveText(['Cancel', 'Add']); + await dialog(page).locator('#action-input').fill('output/unthumbnailed'); + const validated = page.waitForResponse(response => response.url().endsWith('/prompt_manager/settings/validate-source')); + await dialog(page).getByRole('button', { name: 'Add', exact: true }).click(); + expect(await (await validated).json()).toEqual({ success: true, valid: true, label: 'unthumbnailed', path: 'output/unthumbnailed', code: null, reason: null }); + await expect(dialog(page)).toHaveCount(0); + await expect(add).toBeFocused(); + // Validation wrote nothing: the draft is dirty and the server still has two sources. + expect(await rowsOf(page)).toEqual([ + ['ComfyUI output', 'output · manual scan', ['Scan folder', 'Remove']], + ['ComfyUI input', 'input · manual scan', ['Scan folder', 'Remove']], + ['unthumbnailed', 'output/unthumbnailed · not saved yet', ['Remove']], + ]); + await expect(statusOf(page)).toHaveText('Unsaved changes.'); + expect((await serverJson(request, SETTINGS_URL)).sources.effective).toHaveLength(2); + const saved = page.waitForRequest(item => item.method() === 'POST' && item.url().endsWith(SETTINGS_URL)); + await saveButton(page).click(); + const body = (await saved).postDataJSON(); + expect(body.gallery_root_paths).toEqual(['output', 'input', 'output/unthumbnailed']); + expect(typeof body.expected_revision).toBe('string'); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + // The list is the server's again: three effective sources, each scannable. + await expect(sourceRow(page, 'unthumbnailed').locator('p')).toHaveText('output/unthumbnailed · manual scan'); + await expect(sourceRow(page, 'unthumbnailed').getByRole('button', { name: 'Scan folder', exact: true })).toBeVisible(); + expect((await serverJson(request, SETTINGS_URL)).sources.effective.map(source => source.path)).toEqual(['output', 'input', 'output/unthumbnailed']); +}); + +test('invalid source shows the server reason', async ({ page, request }) => { + const layout = await openSources(page); + await layout.getByRole('button', { name: '+ Add source', exact: true }).click(); + const field = dialog(page).locator('#action-input'); + for (const [path, reason, submit] of [ + ['output/not-there', 'Gallery root does not exist', 'click'], + // Enter in the field adds too (M6). + ['/etc', OUTSIDE_REASON, 'Enter'], + ]) { + await field.fill(path); + if (submit === 'Enter') await field.press('Enter'); + else await dialog(page).getByRole('button', { name: 'Add', exact: true }).click(); + await expect(dialog(page).locator('p.small[role="alert"]')).toHaveText(reason); + await expect(field).toHaveAttribute('aria-invalid', 'true'); + await expect(field).toHaveValue(path); + } + // A folder already listed (the server's answer names it) is not added twice. + await field.fill('input'); + await dialog(page).getByRole('button', { name: 'Add', exact: true }).click(); + await expect(dialog(page).locator('p.small[role="alert"]')).toHaveText('This folder is already a source.'); + // A check that cannot finish says so and adds nothing. + await page.route('**/prompt_manager/settings/validate-source', route => route.fulfill({ status: 500, + json: { success: false, code: null, error: 'Failed to check the source' } })); + await field.fill('output/not-checked'); + await dialog(page).getByRole('button', { name: 'Add', exact: true }).click(); + await expect(dialog(page).locator('p.small[role="alert"]')).toHaveText('The folder could not be checked. Try again.'); + await page.unroute('**/prompt_manager/settings/validate-source'); + // Typing again clears the reason. + await field.fill('output/unthumbnailed'); + await expect(dialog(page).locator('p.small[role="alert"]')).toHaveCount(0); + await expect(field).not.toHaveAttribute('aria-invalid', 'true'); + await dialog(page).getByRole('button', { name: 'Cancel', exact: true }).click(); + await expect(statusOf(page)).toHaveText('No unsaved changes.'); + expect((await serverJson(request, SETTINGS_URL)).sources.configured).toHaveLength(2); +}); + +test('Remove marks the draft; removing the last entry states auto-detect', async ({ page, request }) => { + await openSources(page); + await sourceRow(page, 'ComfyUI input').getByRole('button', { name: 'Remove', exact: true }).click(); + expect(await rowsOf(page)).toEqual([['ComfyUI output', 'output · manual scan', ['Scan folder', 'Remove']]]); + await expect(statusOf(page)).toHaveText('Unsaved changes.'); + await sourceRow(page, 'ComfyUI output').getByRole('button', { name: 'Remove', exact: true }).click(); + expect(await rowsOf(page)).toEqual([['p', 'small', 'No source is configured. After saving, ComfyUI’s output folder is used.']]); + await saveButton(page).click(); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + // Saved: ComfyUI's output folder is detected and listed without Remove. + await expect.poll(rowsOf.bind(null, page)).toEqual([ + ['ComfyUI output', 'output · manual scan', ['Scan folder']], + ['p', 'small', 'No source is configured; ComfyUI’s output folder is used.'], + ]); + const { sources } = await serverJson(request, SETTINGS_URL); + expect([sources.configured, sources.auto_detected]).toEqual([[], true]); +}); + +test('a configured source that does not exist says why and can be removed', async ({ page, request }) => { + await expect(await request.post('/__test__/system/missing-source')).toBeOK(); + await openSources(page); + // The fixture's unmounted drive is not inside a ComfyUI folder, so the server says that (it never probes outside). + expect((await rowsOf(page)).at(-1)).toEqual(['unmounted-drive', `unmounted-drive · ${OUTSIDE_REASON}`, ['Remove']]); + await sourceRow(page, 'unmounted-drive').getByRole('button', { name: 'Remove', exact: true }).click(); + await expect(sourceList(page).locator('.operation')).toHaveCount(2); + await saveButton(page).click(); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + expect((await serverJson(request, SETTINGS_URL)).sources.configured.map(source => source.path)).toEqual(['output', 'input']); +}); + +test('watching only with a live observer; restart pending otherwise', async ({ page, request }) => { + // No image monitor in this process: every source is scanned by hand. + await openSources(page); + await expect(sourceList(page).locator('.operation p')).toHaveText(['output · manual scan', 'input · manual scan']); + // A live observer that scheduled output: output is watched; input was added since it started. + await expect(await request.post('/__test__/system/monitor', { data: { watching: ['output'] } })).toBeOK(); + await page.reload(); + await expect(sourceList(page).locator('.operation p')).toHaveText(['output · watching', 'input · restart pending']); + await expect(sourceList(page).locator('.operation .row-actions > button:first-child')).toHaveText(['Scan changes', 'Scan folder']); + // An observer that died watches nothing. + await expect(await request.post('/__test__/system/monitor', { data: { watching: ['output', 'input'], alive: false } })).toBeOK(); + await page.reload(); + await expect(sourceList(page).locator('.operation p')).toHaveText(['output · restart pending', 'input · restart pending']); +}); + +test('Watch for new outputs saves monitoring_enabled; a restart applies it', async ({ page, request }) => { + await expect(await request.post('/__test__/system/monitor', { data: { watching: ['output', 'input'] } })).toBeOK(); + const layout = await openSources(page); + const monitoring = layout.getByRole('switch', { name: 'Watch for new outputs', exact: true }); + await expect(monitoring).toBeChecked(); + await expect(layout.locator('.setting-row').filter({ has: page.locator('#monitoring') }).locator('p')) + .toHaveText('New files enter the same library. Existing scans do not count as new runs.'); + await monitoring.uncheck(); + await expect(statusOf(page)).toHaveText('Unsaved changes.'); + const saved = page.waitForResponse(response => response.request().method() === 'POST' && response.url().endsWith(SETTINGS_URL)); + await saveButton(page).click(); + const answer = await (await saved).json(); + expect(answer.restart_required_fields).toEqual(['monitoring_enabled']); + // D128: saved for the next start, and the status says a restart applies it. + await expect(statusOf(page)).toHaveText('Saved to the server configuration. Restart ComfyUI to apply: Watch for new outputs.'); + // The running observer keeps watching until ComfyUI restarts, and the list says so. + const { settings, monitor } = await serverJson(request, SETTINGS_URL); + expect([settings.monitoring_enabled, monitor.observer_alive]).toEqual([false, true]); + await expect(sourceList(page).locator('.operation p')).toHaveText(['output · watching until restart', 'input · watching until restart']); + await expect(sourceList(page).locator('.operation .row-actions > button:first-child')).toHaveText(['Scan changes', 'Scan changes']); + // Switched back on before the restart: the observer still runs, so nothing waits for one. + await monitoring.check(); + await saveButton(page).click(); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + await expect(sourceList(page).locator('.operation p')).toHaveText(['output · watching', 'input · watching']); +}); + +test('worker range max is cpu_count', async ({ page, request }) => { + const { settings } = await serverJson(request, SETTINGS_URL); + const layout = await openSources(page); + const workers = layout.locator('#workers'); + await expect(layout.locator('label[for="workers"]')).toHaveText('Worker threads'); + await expect(workers).toHaveAttribute('type', 'range'); + await expect(workers).toHaveAttribute('min', '1'); + await expect(workers).toHaveAttribute('max', String(settings.cpu_count)); + await expect(workers).toHaveValue(String(settings.worker_threads)); + const next = settings.worker_threads === 1 ? settings.cpu_count : 1; + await workers.fill(String(next)); + await expect(statusOf(page)).toHaveText(next === settings.worker_threads ? 'No unsaved changes.' : 'Unsaved changes.'); + test.skip(next === settings.worker_threads, 'One core: the range has a single value'); + const saved = page.waitForRequest(item => item.method() === 'POST' && item.url().endsWith(SETTINGS_URL)); + await saveButton(page).click(); + expect((await saved).postDataJSON().worker_threads).toBe(next); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + expect((await serverJson(request, SETTINGS_URL)).settings.worker_threads).toBe(next); +}); + +test('Refresh files calls /gallery/rescan and toasts its counts', async ({ page }) => { + const layout = await openSources(page); + const refresh = layout.locator('.operation').filter({ has: page.getByRole('heading', { name: 'Refresh files', exact: true }) }); + await expect(refresh.locator('p')).toHaveText('Read every source again to find files added or removed outside ComfyUI.'); + const answered = page.waitForResponse(response => response.url().endsWith('/prompt_manager/gallery/rescan')); + await refresh.getByRole('button', { name: 'Refresh', exact: true }).click(); + const { total, roots } = await (await answered).json(); + expect(roots).toBe(2); + await expect(toast(page)).toHaveText(`Files refreshed. ${total.toLocaleString('en-US')} files in 2 output folders.`); + // A failed refresh says so. + await page.route('**/prompt_manager/gallery/rescan', route => route.fulfill({ status: 500, json: { success: false, error: 'busy disk' } })); + await refresh.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(toast(page)).toHaveText('Files could not be refreshed. Try again.'); +}); + +test('Thumbnail generation hands off to the existing gallery until its review dialog', async ({ page }) => { + const layout = await openSources(page); + const thumbnails = layout.locator('.operation').filter({ has: page.getByRole('heading', { name: 'Thumbnail generation', exact: true }) }); + await expect(thumbnails.locator('p')).toHaveText('Generate missing previews; rebuilding all is a separate explicit option.'); + const popup = page.waitForEvent('popup'); + await thumbnails.getByRole('button', { name: 'Review options', exact: true }).click(); + const gallery = await popup; + expect(new URL(gallery.url()).pathname).toBe('/prompt_manager/gallery'); + await gallery.close(); + await expect(statusOf(page)).toHaveText('No unsaved changes.'); +}); + +test('a save refused by a running scan shows the busy link toast', async ({ page, request }) => { + const running = await (await request.post('/__test__/jobs/start', { data: { title: 'Scan for metadata' } })).json(); + await openSources(page); + await sourceRow(page, 'ComfyUI input').getByRole('button', { name: 'Remove', exact: true }).click(); + await saveButton(page).click(); + await expect(toast(page)).toHaveText('Another job is running: Scan for metadata.'); + await expect(toast(page).getByRole('link', { name: 'Scan for metadata', exact: true })).toHaveAttribute('href', `#/activity/${running.job.id}`); + await expect(statusOf(page)).toHaveText('Some changes were not saved. Review the highlighted settings.'); + // The draft is kept for the next Save, and the server still has both sources. + await expect(sourceList(page).locator('.operation')).toHaveCount(1); + expect((await serverJson(request, SETTINGS_URL)).sources.configured).toHaveLength(2); + // An answer that does not name its job (an older server): the toast names the running one from the job list. + await page.route(`**${SETTINGS_URL}`, route => (route.request().method() === 'POST' + ? route.fulfill({ status: 409, json: { success: false, code: 'busy', error: 'Scan for metadata already running' } }) + : route.continue())); + await page.locator('#workspace-toast').evaluate(node => { node.textContent = ''; }); + await saveButton(page).click(); + await expect(toast(page)).toHaveText('Another job is running: Scan for metadata.'); + await expect(toast(page).getByRole('link', { name: 'Scan for metadata', exact: true })).toHaveAttribute('href', `#/activity/${running.job.id}`); + await page.unroute(`**${SETTINGS_URL}`); + await request.post(`/__test__/jobs/${running.job.id}/finish`, { data: { state: 'finished' } }); +}); + +test('a source list that cannot be read says so and retries', async ({ page }) => { + await page.addInitScript(key => { if (location.protocol.startsWith('http')) localStorage.setItem(key, '3.2.0'); }, NOTICE_KEY); + let failing = true; + await page.route(`**${SETTINGS_URL}`, route => (failing && route.request().method() === 'GET' + ? route.fulfill({ status: 500, json: { success: false, error: 'Injected failure' } }) : route.continue())); + await page.goto(`${workspace}#/system/sources`); + await expect(sourceList(page).getByRole('alert')).toHaveText('Sources could not be loaded. Your saved data has not changed.'); + await expect(sourceList(page).locator('.operation')).toHaveCount(0); + // The server settings rows say so too, and stay disabled. + await expect(layoutOf(page).locator('#workers')).toBeDisabled(); + failing = false; + await sourceList(page).getByRole('button', { name: 'Retry', exact: true }).click(); + await expect(sourceList(page).locator('.operation')).toHaveCount(2); +}); + +test('a scan from a list that changed since it was read starts nothing, says why and reads the list again', async ({ page, request }) => { + await openSources(page); + // The legacy page (or another tab) removes ComfyUI output: the page's "input" is index 1, now past the end. + await expect(await request.post(SETTINGS_URL, { data: { gallery_root_paths: ['input'] } })).toBeOK(); + const refused = page.waitForResponse(response => response.url().endsWith('/prompt_manager/jobs/scan')); + await sourceRow(page, 'ComfyUI input').getByRole('button', { name: 'Scan folder', exact: true }).click(); + await dialog(page).getByRole('button', { name: 'Start scan', exact: true }).click(); + const answer = await refused; + expect([answer.status(), (await answer.json()).code]).toEqual([409, 'sources-changed']); + await expect(toast(page)).toHaveText('The sources changed since they were loaded. Reload them.'); + // No scan started (the other save ran as its own source-change hold). + expect((await serverJson(request, '/prompt_manager/jobs')).jobs.map(job => job.kind)).toEqual(['source_change']); + // Read again: the one source is index 0 now, and its scan starts. + await expect.poll(rowsOf.bind(null, page)).toEqual([['ComfyUI input', 'input · manual scan', ['Scan folder', 'Remove']]]); + await expect(statusOf(page)).toHaveText('No unsaved changes.'); + const started = page.waitForRequest(item => item.method() === 'POST' && item.url().endsWith('/prompt_manager/jobs/scan')); + await sourceRow(page, 'ComfyUI input').getByRole('button', { name: 'Scan folder', exact: true }).click(); + await dialog(page).getByRole('button', { name: 'Start scan', exact: true }).click(); + expect((await started).postDataJSON()).toEqual({ scope: 'new', roots: [0], paths: ['input'] }); + await expect(page).toHaveURL(/#\/activity\/[0-9a-f]{32}$/); +}); + +test('a save refused for one unusable source names it and marks its row', async ({ page, request }) => { + await expect(await request.post('/__test__/system/missing-source', { data: { inside: true } })).toBeOK(); + await openSources(page); + await expect(sourceRow(page, 'unplugged').locator('p')).toHaveText('output/unplugged · manual scan'); + // The drive is unplugged after the page read the list; removing another source resends the whole list. + await expect(await request.post('/__test__/system/unplug')).toBeOK(); + await sourceRow(page, 'ComfyUI input').getByRole('button', { name: 'Remove', exact: true }).click(); + const saved = page.waitForResponse(response => response.request().method() === 'POST' && response.url().endsWith(SETTINGS_URL)); + await saveButton(page).click(); + const answer = await saved; + expect(answer.status()).toBe(400); + expect(await answer.json()).toMatchObject({ code: 'invalid-source', source: 'output/unplugged', index: 1 }); + await expect(statusOf(page)).toHaveText("output/unplugged can't be used: Gallery root does not exist. Remove it or reconnect it, then save."); + await expect(sourceRow(page, 'unplugged')).toHaveAttribute('aria-invalid', 'true'); + await expect(sourceList(page).locator('.operation[aria-invalid="true"]')).toHaveCount(1); + expect((await serverJson(request, SETTINGS_URL)).sources.configured).toHaveLength(3); + // Removed, the list saves. + await sourceRow(page, 'unplugged').getByRole('button', { name: 'Remove', exact: true }).click(); + await expect(sourceList(page).locator('.operation[aria-invalid="true"]')).toHaveCount(0); + await saveButton(page).click(); + await expect(statusOf(page)).toHaveText('Saved to the server configuration.'); + expect((await serverJson(request, SETTINGS_URL)).sources.configured.map(source => source.path)).toEqual(['output']); +}); diff --git a/tests/browser/workspace-system.spec.mjs b/tests/browser/workspace-system.spec.mjs index ca2da6e..a11cfdf 100644 --- a/tests/browser/workspace-system.spec.mjs +++ b/tests/browser/workspace-system.spec.mjs @@ -237,10 +237,11 @@ test('out-of-order overview responses never repaint a newer section', async ({ p await nav.getByRole('button', { name: 'Appearance', exact: true }).click(); await nav.getByRole('button', { name: 'Overview', exact: true }).click(); await expect.poll(held).toBe(2); - await nav.getByRole('button', { name: 'Sources & scans', exact: true }).click(); + // Integrations still hands off (S10), so its body is one known panel. + await nav.getByRole('button', { name: 'Integrations', exact: true }).click(); await expect(await request.post('/__test__/release')).toBeOK(); await expect.poll(held).toBe(0); - // Both late answers reached a section that had left: Sources keeps its own body. + // Both late answers reached a section that had left: Integrations keeps its own body. await expect(layout.locator(':scope > section.handoff')).toBeVisible(); await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)))); await expect(layout.locator('.summary-metrics')).toHaveCount(0); diff --git a/tests/browser/workspace.spec.mjs b/tests/browser/workspace.spec.mjs index 8d847a3..79313a3 100644 --- a/tests/browser/workspace.spec.mjs +++ b/tests/browser/workspace.spec.mjs @@ -425,8 +425,7 @@ for (const width of [1600, 390]) test(`overview and handoff render with concept const SYSTEM_SECTION_LABELS = ['Overview', 'Appearance', 'Sources & scans', 'Integrations', 'Auto-tagging', 'Maintenance', 'Backup & export', 'Diagnostics & logs']; const SYSTEM_HASHES = ['overview', 'appearance', 'sources', 'integrations', 'tagging', 'maintenance', 'backups', 'diagnostics']; -const SYSTEM_HANDOFFS = { sources: ['/prompt_manager/admin', '/prompt_manager/gallery'], - maintenance: ['/prompt_manager/admin', '/prompt_manager/gallery'] }; +const SYSTEM_HANDOFFS = { maintenance: ['/prompt_manager/admin', '/prompt_manager/gallery'] }; test('System is native: the concept shell, section navigation, sections, hand-offs and the Save row', async ({ page }) => { const mutations = []; @@ -462,14 +461,17 @@ test('System is native: the concept shell, section navigation, sections, hand-of await expect(nav.locator('[aria-pressed="true"]')).toHaveCount(1); // The section changes in place: focus stays on the pressed section, not the page title. await expect(button).toBeFocused(); - // S2 Task 6: Overview is native (workspace-system.spec.mjs), and so is Appearance (S3 Task 8); Maintenance shows + // S2 Task 6: Overview is native (workspace-system.spec.mjs), and so are Appearance (S3 Task 8) and Sources & scans + // (S4 Task 11, workspace-system-sources.spec.mjs, its update notice shown in a fresh browser); Maintenance shows // its health-check notice above the hand-off (D123); every other section hands off. if (hash === 'overview') await expect(layout.locator(':scope > .summary-metrics')).toBeVisible(); + if (hash === 'sources') await expect(layout.locator('#source-list .operation')).toHaveCount(2); expect(await layout.evaluate(node => [...node.children].map(child => `${child.localName}.${child.className}`))) .toEqual({ overview: ['div.summary-metrics', 'div.two-col', 'div.save-row'], appearance: ['section.surface', 'div.save-row'], + sources: ['div.notice', 'section.surface', 'div.save-row'], maintenance: ['div.notice', 'section.surface handoff', 'div.save-row'] }[hash] ?? ['section.surface handoff', 'div.save-row']); - if (hash === 'overview' || hash === 'appearance') continue; + if (['overview', 'appearance', 'sources'].includes(hash)) continue; const handoff = layout.locator('section.handoff'); await expect(handoff.locator('h2')).toHaveText('Your current tools are ready'); expect(await handoff.locator('a.workspace-link').evaluateAll(links => links.map(link => link.getAttribute('href')))) diff --git a/tests/browser/workspace_system_fixture.py b/tests/browser/workspace_system_fixture.py index 2413c1f..95f3218 100644 --- a/tests/browser/workspace_system_fixture.py +++ b/tests/browser/workspace_system_fixture.py @@ -11,11 +11,18 @@ read. These per-server hooks only change what a System test needs on top: requests for ``path`` (a ``/prompt_manager/system/`` route) with the routes' 500 error envelope; - ``POST /__test__/system/missing-source`` configures one more source that - does not exist (an unmounted drive); + does not exist (an unmounted drive); with ``{"inside": true}`` it is an + existing folder inside the output folder (``output/unplugged``), which + ``POST /__test__/system/unplug`` then removes (a drive unplugged while the + page shows it); - ``POST /__test__/system/missing-file`` deletes the newest linked image's file from disk, leaving its database row (a missing-file link); - ``POST /__test__/system/unthumbnailed`` ``{count}`` writes ``count`` plain - PNGs with no thumbnail into the output folder. + PNGs with no thumbnail into the output folder; +- ``POST /__test__/system/monitor`` ``{watching, alive}`` stands in a fake + image monitor whose observer is ``alive`` (default true) and schedules the + named folders of this server's root (``["output"]``), so Sources reads + watching, manual scan and restart pending states (S4). Delays use the shared request holds (``request_hold.py``). """ @@ -45,13 +52,17 @@ def register_system_commands(app, routes, api, root: Path): f"PROMPT_MANAGER_CONFIG_PATH must be {config_path}, not {configured_path!r}" ) from py.config import GalleryConfig, PromptManagerConfig + from utils import image_monitor configured = list(GalleryConfig.MONITORING_DIRECTORIES) config_bytes = config_path.read_bytes() if config_path.exists() else None host_values = ( PromptManagerConfig.RESULT_TIMEOUT, PromptManagerConfig.WEBUI_DISPLAY_MODE, + PromptManagerConfig.WORKER_THREADS, + GalleryConfig.MONITORING_ENABLED, ) + monitor = getattr(image_monitor, "_monitor_instance", None) failures: dict[str, int] = {} @web.middleware @@ -66,13 +77,21 @@ def register_system_commands(app, routes, api, root: Path): app.middlewares.append(inject_failures) + unplugged = root / "output" / "unplugged" + def reset(): failures.clear() + if unplugged.is_dir(): + unplugged.rmdir() GalleryConfig.MONITORING_DIRECTORIES = list(configured) api._health_report = None - PromptManagerConfig.RESULT_TIMEOUT, PromptManagerConfig.WEBUI_DISPLAY_MODE = ( - host_values - ) + ( + PromptManagerConfig.RESULT_TIMEOUT, + PromptManagerConfig.WEBUI_DISPLAY_MODE, + PromptManagerConfig.WORKER_THREADS, + GalleryConfig.MONITORING_ENABLED, + ) = host_values + image_monitor._monitor_instance = monitor if config_bytes is None: config_path.unlink(missing_ok=True) else: @@ -103,14 +122,26 @@ def register_system_commands(app, routes, api, root: Path): @routes.post("/__test__/system/missing-source") async def missing_source(request): - absent = str(root / "unmounted-drive") - if os.path.exists(absent): - raise web.HTTPConflict() + data = await request.json() if request.can_read_body else {} + if data.get("inside") is True: + unplugged.mkdir(exist_ok=True) + absent = str(unplugged) + else: + absent = str(root / "unmounted-drive") + if os.path.exists(absent): + raise web.HTTPConflict() GalleryConfig.MONITORING_DIRECTORIES = [*configured, absent] return web.json_response( {"success": True, "configured": len(GalleryConfig.MONITORING_DIRECTORIES)} ) + @routes.post("/__test__/system/unplug") + async def unplug(request): + if not unplugged.is_dir(): + raise web.HTTPConflict() + unplugged.rmdir() + return web.json_response({"success": True}) + @routes.post("/__test__/system/missing-file") async def missing_file(request): with api.db.model.get_connection() as conn: @@ -141,4 +172,35 @@ def register_system_commands(app, routes, api, root: Path): api.invalidate_gallery_cache() return web.json_response({"success": True}) + @routes.post("/__test__/system/monitor") + async def fake_monitor(request): + data = await request.json() + watching, alive = data.get("watching", []), data.get("alive", True) + if ( + not isinstance(watching, list) + or not all(name in ("output", "input") for name in watching) + or not isinstance(alive, bool) + ): + raise web.HTTPBadRequest() + image_monitor._monitor_instance = _FakeMonitor( + [str(root / name) for name in watching], alive + ) + return web.json_response({"success": True}) + return reset + + +class _FakeMonitor: + """The image monitor's status surface (``get_status``) with no observer thread.""" + + def __init__(self, directories, alive): + self.monitored_directories = directories + self._alive = alive + + def get_status(self): + return { + "running": True, + "monitored_directories": list(self.monitored_directories), + "handler_active": True, + "observer_alive": self._alive, + } diff --git a/tests/js/api-client.test.js b/tests/js/api-client.test.js index df2fa21..e22878f 100644 --- a/tests/js/api-client.test.js +++ b/tests/js/api-client.test.js @@ -355,3 +355,20 @@ test("keepalive is forwarded only when asked, so a request can outlive its page" for (const call of fetch.calls) call.resolve(jsonResponse(200, { success: true })); await Promise.all(writes); }); + +test("ApiError keeps a validated running job on busy and a validated source on invalid-source", () => { + const body = { running: { id: "a".repeat(32), title: "Scan for metadata" }, source: "output/gone" }; + const busy = new ApiError(409, "busy", "busy", null, body); + assert.deepEqual([busy.running, busy.source], [{ id: "a".repeat(32), title: "Scan for metadata" }, null]); + assert.notEqual(busy.running, body.running, "server objects must be copied"); + const invalid = new ApiError(400, "refused", "invalid-source", null, body); + assert.deepEqual([invalid.running, invalid.source], [null, "output/gone"]); + for (const running of [null, "x", { id: 1, title: "t" }, { id: "a b", title: "t" }, { id: "a", title: "" }, + { id: "a", title: "x".repeat(201) }, { id: "a".repeat(65), title: "t" }]) { + assert.equal(new ApiError(409, "busy", "busy", null, { running }).running, null); + } + for (const source of [null, 7, "", "x".repeat(4097)]) { + assert.equal(new ApiError(400, "refused", "invalid-source", null, { source }).source, null); + } + assert.deepEqual([new ApiError(409, "old caller").running, new ApiError(409, "old caller").source], [null, null]); +}); diff --git a/tests/js/workspace-system-sources.test.js b/tests/js/workspace-system-sources.test.js new file mode 100644 index 0000000..53e4663 --- /dev/null +++ b/tests/js/workspace-system-sources.test.js @@ -0,0 +1,145 @@ +// System › Sources & scans (#249 System S4 Task 11): the concept's "Image sources" list (app.js 679–681) built from the +// server's configured, effective and watched sources (GET /settings), the host draft of `gallery_root_paths`, the +// 3.2.0 update notice's dismissal and the Refresh files toast (D98). +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const sourcesModule = import('../../web/js/workspace/system-sources.mjs'); + +const STATUS = Object.freeze({ + configured: [ + { label: 'ComfyUI output', path: 'output', valid: true, code: null, reason: null }, + { label: 'ComfyUI input', path: 'input', valid: true, code: null, reason: null }, + ], + effective: [ + { label: 'ComfyUI output', path: 'output', root_index: 0, watch: 'watching' }, + { label: 'ComfyUI input', path: 'input', root_index: 1, watch: 'manual' }, + ], + auto_detected: false, +}); + +test('each effective source is the concept row: label, "<path> · watching|manual scan", Scan changes or Scan folder', async () => { + const { sourceRows } = await sourcesModule; + const saved = ['output', 'input']; + const { rows, note } = sourceRows({ sources: STATUS, saved, current: saved }); + assert.equal(note, null); + assert.deepEqual(rows, [ + { key: 'output', title: 'ComfyUI output', description: 'output · watching', scan: { label: 'Scan changes', root: 0, path: 'output' }, + remove: 'output' }, + { key: 'input', title: 'ComfyUI input', description: 'input · manual scan', scan: { label: 'Scan folder', root: 1, path: 'input' }, + remove: 'input' }, + ]); +}); + +test('watching only with a live observer; restart pending otherwise (D98)', async () => { + const { sourceRows, WATCH_COPY } = await sourcesModule; + assert.deepEqual(WATCH_COPY, { watching: 'watching', stopping: 'watching until restart', manual: 'manual scan', + restart_pending: 'restart pending' }); + const sources = { ...STATUS, effective: STATUS.effective.map(source => ({ ...source, watch: 'restart_pending' })) }; + const { rows } = sourceRows({ sources, saved: ['output', 'input'], current: ['output', 'input'] }); + assert.deepEqual(rows.map(row => [row.description, row.scan.label]), + [['output · restart pending', 'Scan folder'], ['input · restart pending', 'Scan folder']]); +}); + +test('a configured source that cannot be used shows the server reason and only Remove', async () => { + const { sourceRows } = await sourcesModule; + const sources = { ...STATUS, configured: [...STATUS.configured, + { label: 'unmounted-drive', path: 'unmounted-drive', valid: false, code: 'not-found', reason: 'Gallery root does not exist' }] }; + const saved = ['output', 'input', 'unmounted-drive']; + const { rows } = sourceRows({ sources, saved, current: saved }); + assert.deepEqual(rows.at(-1), { key: 'unmounted-drive', title: 'unmounted-drive', + description: 'unmounted-drive · Gallery root does not exist', scan: null, remove: 'unmounted-drive' }); +}); + +test('an added source is listed as not saved yet with its validated label; a removed one leaves the list', async () => { + const { sourceRows } = await sourcesModule; + const labels = new Map([['output/archive', 'archive']]); + const { rows, note } = sourceRows({ sources: STATUS, saved: ['output', 'input'], current: ['input', 'output/archive'], labels }); + assert.equal(note, null); + assert.deepEqual(rows.map(row => [row.title, row.description, row.scan?.label ?? null, row.remove]), [ + ['ComfyUI input', 'input · manual scan', 'Scan folder', 'input'], + ['archive', 'output/archive · not saved yet', null, 'output/archive'], + ]); + // Without a recorded label (a reload of System), the folder name stands in. + assert.equal(sourceRows({ sources: STATUS, saved: [], current: ['output/new-collection'] }).rows[0].title, 'new-collection'); +}); + +test('removing the last entry states auto-detect (D98)', async () => { + const { sourceRows, NOTES } = await sourcesModule; + const { rows, note } = sourceRows({ sources: STATUS, saved: ['output', 'input'], current: [] }); + assert.deepEqual(rows, []); + assert.equal(note, NOTES.removedAll); + assert.equal(NOTES.removedAll, 'No source is configured. After saving, ComfyUI’s output folder is used.'); +}); + +test('auto-detected sources are listed without Remove, with the line that says how they were chosen', async () => { + const { sourceRows, NOTES } = await sourcesModule; + const auto = { configured: [], effective: [{ label: 'ComfyUI output', path: 'output', root_index: 0, watch: 'manual' }], auto_detected: true }; + assert.deepEqual(sourceRows({ sources: auto, saved: [], current: [] }), { rows: [ + { key: 'auto:0', title: 'ComfyUI output', description: 'output · manual scan', scan: { label: 'Scan folder', root: 0, path: 'output' }, + remove: null }, + ], note: NOTES.autoDetected }); + // Every configured source unusable: ComfyUI's output folder is used instead. + const fallback = { ...auto, configured: [{ label: 'gone', path: 'gone', valid: false, code: 'not-found', reason: 'Gallery root does not exist' }] }; + const result = sourceRows({ sources: fallback, saved: ['gone'], current: ['gone'] }); + assert.deepEqual(result.rows.map(row => [row.title, row.remove]), [['gone', 'gone'], ['ComfyUI output', null]]); + assert.equal(result.note, NOTES.fallback); + assert.equal(sourceRows({ sources: { configured: [], effective: [], auto_detected: false }, saved: [], current: [] }).note, NOTES.none); +}); + +test('a path listed twice in the draft is one row', async () => { + const { sourceRows } = await sourcesModule; + assert.equal(sourceRows({ sources: STATUS, saved: ['output', 'input'], current: ['output', 'output'] }).rows.length, 1); +}); + +test('roots equal to the saved list are the saved list itself, so the draft is clean again', async () => { + const { draftRoots } = await sourcesModule; + const saved = ['output', 'input']; + assert.equal(draftRoots(['output', 'input'], saved), saved); + assert.deepEqual(draftRoots(['input'], saved), ['input']); + assert.notEqual(draftRoots(['input', 'output'], saved), saved); + assert.deepEqual(draftRoots([], undefined), []); +}); + +test('notice Later persists the 3.2.0 dismissal (pm_update_notice_dismissed); storage that throws keeps it for this view', async () => { + const { noticeDismissed, dismissNotice, NOTICE_KEY, NOTICE_VERSION } = await sourcesModule; + assert.deepEqual([NOTICE_KEY, NOTICE_VERSION], ['pm_update_notice_dismissed', '3.2.0']); + const values = new Map(); + const storage = { getItem: key => values.get(key) ?? null, setItem: (key, value) => values.set(key, String(value)) }; + assert.equal(noticeDismissed(() => storage), false); + values.set(NOTICE_KEY, '3.1.0'); + assert.equal(noticeDismissed(() => storage), false); + assert.equal(dismissNotice(() => storage), true); + assert.equal(values.get(NOTICE_KEY), '3.2.0'); + assert.equal(noticeDismissed(() => storage), true); + const denied = () => { throw new Error('SecurityError'); }; + assert.equal(noticeDismissed(denied), false); + assert.equal(dismissNotice(denied), false); + assert.equal(dismissNotice(() => ({ setItem() { throw new Error('QuotaExceededError'); } })), false); +}); + +test('Refresh files toasts the rescan counts, or says it finished when they are missing', async () => { + const { refreshedText } = await sourcesModule; + assert.equal(refreshedText({ total: 1234, roots: 2 }), 'Files refreshed. 1,234 files in 2 output folders.'); + assert.equal(refreshedText({ total: 1, roots: 1 }), 'Files refreshed. 1 file in 1 output folder.'); + assert.equal(refreshedText({ total: -1, roots: 1 }), 'Files refreshed.'); + assert.equal(refreshedText(null), 'Files refreshed.'); +}); + +test('monitoring switched off: a source still watched says so until the restart (D128)', async () => { + const { sourceRows } = await sourcesModule; + const sources = { ...STATUS, effective: [{ ...STATUS.effective[0], watch: 'stopping' }, STATUS.effective[1]] }; + const { rows } = sourceRows({ sources, saved: ['output', 'input'], current: ['output', 'input'] }); + assert.deepEqual(rows.map(row => [row.description, row.scan.label]), + [['output · watching until restart', 'Scan changes'], ['input · manual scan', 'Scan folder']]); +}); + +test('a valid configured source missing from the effective list is another spelling of a listed folder (D128)', async () => { + const { sourceRows, DUPLICATE_SOURCE } = await sourcesModule; + assert.equal(DUPLICATE_SOURCE, 'same folder as another source'); + const sources = { ...STATUS, configured: [...STATUS.configured, + { label: 'output', path: 'output/.', valid: true, code: null, reason: null }] }; + const saved = ['output', 'input', 'output/.']; + assert.deepEqual(sourceRows({ sources, saved, current: saved }).rows.at(-1), { key: 'output/.', title: 'output', + description: 'output/. · same folder as another source', scan: null, remove: 'output/.' }); +}); diff --git a/tests/js/workspace-system-state.test.js b/tests/js/workspace-system-state.test.js index af60efb..f7e611e 100644 --- a/tests/js/workspace-system-state.test.js +++ b/tests/js/workspace-system-state.test.js @@ -192,6 +192,7 @@ test('status text per outcome', async () => { partial: 'Some changes were not saved. Review the highlighted settings.', 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:', }); const clean = { browser: 'clean', host: 'clean', runtime: 'clean' }; assert.equal(statusText(clean), 'No unsaved changes.'); @@ -316,3 +317,50 @@ test('host read state says whether the server settings could be read', async () assert.equal(await drafts.reloadHost(), true); assert.equal(drafts.hostState(), 'loaded'); }); + +test('a host save names the settings that apply after a restart (D128)', async () => { + const { statusText, RESTART_LABELS } = await source; + assert.deepEqual(RESTART_LABELS, { monitoring_enabled: 'Watch for new outputs' }); + const saved = { browser: 'clean', host: 'saved', runtime: 'clean' }; + assert.equal(statusText({ ...saved, restart: ['monitoring_enabled'] }), + 'Saved to the server configuration. Restart ComfyUI to apply: Watch for new outputs.'); + // A field without a row label (the sources say "restart pending" per row) adds nothing. + assert.equal(statusText({ ...saved, restart: ['gallery_root_paths'] }), 'Saved to the server configuration.'); + assert.equal(statusText({ ...saved, restart: [] }), 'Saved to the server configuration.'); + + const domains = adapters({ host: { monitoring_enabled: true } }); + const drafts = await loaded(domains); + domains.outcome.host = () => ({ success: true, persisted: true, revision: 'sha256:two', + restart_required_fields: ['monitoring_enabled', 7] }); + drafts.set('host', 'monitoring_enabled', false); + assert.deepEqual(await drafts.save(), { browser: 'clean', host: 'saved', runtime: 'clean', restart: ['monitoring_enabled'] }); + assert.equal(drafts.statusText(), 'Saved to the server configuration. Restart ComfyUI to apply: Watch for new outputs.'); + // The next edit is unsaved again. + drafts.set('host', 'monitoring_enabled', true); + assert.equal(drafts.statusText(), 'Unsaved changes.'); +}); + +test('a host save refused for one source says the server reason and names it until the next edit', async () => { + const domains = adapters({ host: { gallery_root_paths: ['output', 'output/gone'] } }); + const drafts = await loaded(domains); + const message = "output/gone can't be used: Gallery root does not exist. Remove it or reconnect it, then save."; + domains.outcome.host = () => { throw Object.assign(new Error(message), { code: 'invalid-source', source: 'output/gone' }); }; + drafts.set('host', 'gallery_root_paths', ['output/gone']); + const outcome = await drafts.save(); + assert.deepEqual(outcome, { browser: 'clean', host: 'failed', runtime: 'clean', + invalid: { source: 'output/gone', message } }); + assert.equal(drafts.statusText(), message); + assert.equal(drafts.invalidSource(), 'output/gone'); + assert.deepEqual([...drafts.failed('host')], ['gallery_root_paths']); + // Any other failure keeps the generic copy and names no source. + domains.outcome.host = () => { throw Object.assign(new Error('busy'), { code: 'busy' }); }; + await drafts.save(); + assert.equal(drafts.statusText(), 'Some changes were not saved. Review the highlighted settings.'); + assert.equal(drafts.invalidSource(), null); + domains.outcome.host = () => { throw Object.assign(new Error(message), { code: 'invalid-source' }); }; + await drafts.save(); + assert.equal(drafts.invalidSource(), null); + drafts.set('host', 'gallery_root_paths', ['output']); + assert.equal(drafts.invalidSource(), null); + assert.equal(drafts.statusText(), 'Unsaved changes.'); +}); diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index b34c0df..8718199 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -52,6 +52,7 @@ class AdminAPITestCase(AioHTTPTestCase): self.outside_dir = Path(self.tmpdir) / "outside" self.outside_dir.mkdir() + self.addAsyncCleanup(self._drain_and_remove) self.config_path = os.path.join(self.tmpdir, "config.json") self._orig_env = { key: os.environ.pop(key, None) for key in (CONFIG_PATH_ENV, EXTRA_ROOTS_ENV) @@ -89,6 +90,23 @@ class AdminAPITestCase(AioHTTPTestCase): sys.modules.pop("folder_paths", None) else: sys.modules["folder_paths"] = self._orig_folder_paths + + async def _drain_and_remove(self): + """The last cleanup: no job outlives the temporary database. + + Registered first, so it runs after every cleanup a test added (which + may release its own gated jobs). A job still running then, such as a + scan a failed assertion never awaited, is cancelled and awaited + before the database and the tree are removed. + """ + api = getattr(self, "api", None) + jobs = api._jobs.list() if api is not None else [] + tasks = [ + job.task for job in jobs if job.task is not None and not job.task.done() + ] + for task in tasks: + task.cancel() + await asyncio.gather(*tasks, return_exceptions=True) PromptModel.close_all_instances() # restore may have swapped api.db assert_closed(self, self.tmpdir) shutil.rmtree(self.tmpdir, ignore_errors=True) diff --git a/tests/test_gallery_roots.py b/tests/test_gallery_roots.py index 5ae871b..b16ffed 100644 --- a/tests/test_gallery_roots.py +++ b/tests/test_gallery_roots.py @@ -23,7 +23,11 @@ sys.modules.setdefault("server", _mock_server) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -from py.config import GalleryConfig, PromptManagerConfig # noqa: E402 +from py.config import ( # noqa: E402 + OUTSIDE_ALLOWED_REASON, + GalleryConfig, + PromptManagerConfig, +) EXTRA_ROOTS_ENV = "PROMPT_MANAGER_EXTRA_GALLERY_ROOTS" @@ -158,9 +162,10 @@ class GalleryRootTestCase(unittest.TestCase): class TestValidateGalleryRootRejects(GalleryRootTestCase): def test_filesystem_root_rejected(self): + """Refused as outside the allowed folders: no reason names the layout.""" ok, reason = GalleryConfig.validate_gallery_root(os.path.abspath(os.sep)) self.assertFalse(ok) - self.assertIn("root", reason.lower()) + self.assertEqual(reason, OUTSIDE_ALLOWED_REASON) @unittest.skipUnless(os.name == "nt", "drive roots only exist on Windows") def test_windows_drive_root_rejected(self): @@ -170,7 +175,17 @@ class TestValidateGalleryRootRejects(GalleryRootTestCase): def test_home_directory_rejected(self): ok, reason = GalleryConfig.validate_gallery_root(os.path.expanduser("~")) self.assertFalse(ok) - self.assertIn("home", reason.lower()) + self.assertEqual(reason, OUTSIDE_ALLOWED_REASON) + + def test_home_directory_inside_an_allowed_parent_is_still_refused(self): + """Even when the environment opens the home's parent, home answers + like any outside path.""" + home = os.path.expanduser("~") + os.environ[EXTRA_ROOTS_ENV] = os.path.dirname(home) + self.assertEqual( + GalleryConfig.gallery_root_problem(home), + ("outside-allowed", OUTSIDE_ALLOWED_REASON), + ) def test_directory_outside_comfyui_rejected(self): outside = Path(self.tmpdir) / "elsewhere" diff --git a/tests/test_scan_scope.py b/tests/test_scan_scope.py index 6044714..f5f085f 100644 --- a/tests/test_scan_scope.py +++ b/tests/test_scan_scope.py @@ -260,7 +260,7 @@ class TestScanScope(ScanScopeTestCase): async def test_empty_read_body_asks_for_the_default_scope(self): """A chunked POST with no bytes can read a body: still the default.""" - from py.api.job_routes import _scan_scope + from py.api.job_routes import _scan_request class Request: can_read_body = True @@ -271,10 +271,21 @@ class TestScanScope(ScanScopeTestCase): async def text(self): return self._text - self.assertEqual(await _scan_scope(Request("")), "new") - self.assertEqual(await _scan_scope(Request(" \n")), "new") - self.assertEqual(await _scan_scope(Request('{"scope": "all"}')), "all") - self.assertIsNone(await _scan_scope(Request("{"))) + default = ({"files": "new", "roots": None}, None) + self.assertEqual(await _scan_request(Request("")), default) + self.assertEqual(await _scan_request(Request(" \n")), default) + self.assertEqual( + await _scan_request( + Request( + '{"scope": "all", "roots": [2, 0, 2],' + ' "paths": ["output/c", "output/a", "output/c"]}' + ) + ), + ({"files": "all", "roots": {0: "output/a", 2: "output/c"}}, None), + ) + request, error = await _scan_request(Request("{")) + self.assertIsNone(request) + self.assertIsInstance(error, str) async def test_invalid_scope_is_refused(self): for body in ({"scope": "everything"}, {"scope": 1}, ["new"]): @@ -292,6 +303,220 @@ class TestScanScope(ScanScopeTestCase): self.assertIsNone(self.api._scan_job) +class TestPerSourceScope(ScanScopeTestCase): + """``roots``: scan only the selected effective sources (#249 S4 Task 10).""" + + async def asyncSetUp(self): + await super().asyncSetUp() + self.a = self.output_dir / "a" + self.b = self.output_dir / "b" + self.a.mkdir() + self.b.mkdir() + _write_png(self.a / "in_a.png", prompt_text="a prompt in a") + _write_png(self.b / "in_b.png", prompt_text="a prompt in b") + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a), self._root(self.b)] + + async def _refused(self, body): + resp = await self.client.request("POST", "/prompt_manager/jobs/scan", json=body) + data = await resp.json() + self.assertEqual(resp.status, 400, data) + self.assertFalse(data["success"]) + self.assertNotIn(self.tmpdir, json.dumps(data)) + self.assertIsNone(self.api._scan_job) + self.assertEqual(self.api._jobs.list(), []) + return data + + async def test_scoped_scan_only_walks_selected_root(self): + walked = [] + original = self.api._collect_output_media_sync + + def spy(output_dirs): + walked.append([os.path.basename(str(d)) for d in output_dirs]) + return original(output_dirs) + + self.api._collect_output_media_sync = spy + reads = self._spy_reads() + + job = await self._scan({"scope": "all", "roots": [1], "paths": ["output/b"]}) + + self.assertEqual(walked, [["b"]]) + self.assertEqual(reads, ["in_b.png"]) + self.assertEqual( + job["scope"], {"files": "all", "roots": [1], "output_folders": 1} + ) + self.assertEqual( + job["outcomes"]["added"], + [ + { + "path": "in_b.png", + "root": "output/b", + "root_index": 1, + "detail": job["outcomes"]["added"][0]["detail"], + } + ], + ) + self.assertEqual(job["counters"]["added"], 1) + self.assertEqual(self.api._scan_job.last_event["directories"], ["output/b"]) + + async def test_duplicate_roots_are_one_selection(self): + job = await self._scan( + {"roots": [1, 0, 1], "paths": ["output/b", "output/a", "output/b"]} + ) + self.assertEqual(job["scope"]["roots"], [0, 1]) + self.assertEqual(job["scope"]["output_folders"], 2) + self.assertEqual(job["counters"]["added"], 2) + + async def test_scope_line_counts_selected_folders(self): + job = await self._scan({"roots": [0], "paths": ["output/a"]}) + self.assertTrue( + job["log"][0].endswith( + "Scan started · 1 output folder · new and unresolved files" + ), + job["log"][0], + ) + self.assertEqual(job["scope"]["output_folders"], 1) + + unscoped = await self._scan({"scope": "all"}) + self.assertEqual( + unscoped["scope"], {"files": "all", "roots": None, "output_folders": 2} + ) + self.assertIn("Scan started · 2 output folders · all files", unscoped["log"][0]) + + async def _stale(self, body): + resp = await self.client.request("POST", "/prompt_manager/jobs/scan", json=body) + data = await resp.json() + self.assertEqual(resp.status, 409, data) + self.assertEqual(data["code"], "sources-changed") + self.assertFalse(data["success"]) + self.assertNotIn(self.tmpdir, json.dumps(data)) + self.assertIsNone(self.api._scan_job) + self.assertEqual(self.api._jobs.list(), []) + return data + + async def test_index_past_the_sources_is_409_without_job(self): + await self._stale({"roots": [2], "paths": ["output/c"]}) + await self._stale({"roots": [0, 64], "paths": ["output/a", "output/x"]}) + + async def test_stale_index_never_scans_another_source(self): + """The list changed after the page read it: source 0 was removed, so + old index 1 (b) is past the end and old index 0 now names b.""" + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.b)] + await self._stale({"roots": [1], "paths": ["output/b"]}) + await self._stale({"roots": [0], "paths": ["output/a"]}) + + job = await self._scan({"roots": [0], "paths": ["output/b"]}) + self.assertEqual(job["outcomes"]["added"][0]["root"], "output/b") + + async def test_roots_need_their_paths(self): + for body in ( + {"roots": [0]}, + {"roots": [0], "paths": None}, + {"roots": [0], "paths": "output/a"}, + {"roots": [0], "paths": []}, + {"roots": [0, 1], "paths": ["output/a"]}, + {"roots": [0], "paths": [0]}, + {"roots": [0, 0], "paths": ["output/a", "output/b"]}, + ): + with self.subTest(body=body): + await self._refused(body) + + async def test_bool_and_float_roots_rejected(self): + for roots in ( + [True], + [False], + [0.0], + [1.5], + ["0"], + [None], + [-1], + [0, None], + [], + None, + 0, + "0", + {"0": 0}, + list(range(65)), + ): + with self.subTest(roots=roots): + paths = ["output/a"] * len(roots) if isinstance(roots, list) else [] + await self._refused({"roots": roots, "paths": paths}) + + async def test_legacy_scan_unscoped_unchanged(self): + from py.api.admin import SCAN_COUNT_KEYS + + resp = await self.client.request("POST", "/prompt_manager/scan", data="{}") + payloads = _sse_payloads(await resp.text()) + + final = payloads[-1] + self.assertEqual(set(final), set(SCAN_COUNT_KEYS) | {"type", "directories"}) + self.assertEqual(final["directories"], ["output/a", "output/b"]) + self.assertEqual(final["added"], 2) + self.assertEqual( + self.api._scan_job.scope, + {"files": "all", "roots": None, "output_folders": 2}, + ) + + async def test_legacy_scan_attaches_to_a_scoped_job_as_is(self): + import threading + + from py.api.admin import SCAN_COUNT_KEYS + + gate = threading.Event() + original = self.api._collect_output_media_sync + + def gated(output_dirs): + gate.wait(5) + return original(output_dirs) + + self.api._collect_output_media_sync = gated + # The legacy route has decided to attach once it streams the job. + attached = asyncio.Event() + stream = self.api._stream_scan_job + + async def attaching(request, job): + attached.set() + return await stream(request, job) + + self.api._stream_scan_job = attaching + resp = await self.client.request( + "POST", + "/prompt_manager/jobs/scan", + json={"roots": [0], "paths": ["output/a"]}, + ) + self.assertEqual(resp.status, 202) + started = (await resp.json())["job"] + legacy = asyncio.ensure_future( + self.client.request("POST", "/prompt_manager/scan", data="{}") + ) + try: + await asyncio.wait_for(attached.wait(), 10) + finally: + gate.set() + payloads = _sse_payloads(await (await legacy).text()) + await asyncio.wait_for(self.api._scan_job.task, 10) + + self.assertEqual(self.api._scan_job.id, started["id"]) + self.assertEqual(len(self.api._jobs.list()), 1) + final = payloads[-1] + self.assertEqual(set(final), set(SCAN_COUNT_KEYS) | {"type", "directories"}) + self.assertEqual(final["directories"], ["output/a"]) + self.assertEqual(final["added"], 1) + + async def test_scoped_scan_uses_the_sources_it_was_started_with(self): + """Sources saved after the start never change what the job walks.""" + resolved = [] + original = self.api._get_all_output_dirs + + def counting(): + resolved.append(1) + return original() + + self.api._get_all_output_dirs = counting + job = await self._scan({"roots": [1], "paths": ["output/b"]}) + self.assertEqual(len(resolved), 1) # the request's own resolution only + self.assertEqual(job["outcomes"]["added"][0]["root_index"], 1) + + class TestKnownResolvedPaths(ScanScopeTestCase): """``PromptDatabase.get_known_resolved_paths``: one streamed query.""" diff --git a/tests/test_settings_sources.py b/tests/test_settings_sources.py new file mode 100644 index 0000000..e4958bd --- /dev/null +++ b/tests/test_settings_sources.py @@ -0,0 +1,959 @@ +"""System › Sources & scans backend (#249 S4 Task 9). + +``GET /prompt_manager/settings`` reports the configured, effective and +watched sources and the monitoring setting; ``POST`` saves +``monitoring_enabled`` (a restart applies it) and refuses a source change +while a record-changing job runs; ``POST /prompt_manager/settings/validate-source`` +checks one path and writes nothing. No response carries an absolute path. +""" + +import asyncio +import functools +import json +import ntpath +import os +import sys +import types +import unittest +from contextlib import contextmanager +from unittest.mock import patch + +try: + from tests.test_api_admin import AdminAPITestCase +except ImportError: # discovered with tests/ as the top-level directory + from test_api_admin import AdminAPITestCase + +from py import config as config_module # noqa: E402 +from py.api import settings_routes, sources # noqa: E402 +from py.config import GalleryConfig # noqa: E402 + +VALIDATE = "/prompt_manager/settings/validate-source" +SETTINGS = "/prompt_manager/settings" + + +@contextmanager +def no_filesystem_access_for(marker, test): + """Fail ``test`` if anything resolves, stats or opens a path holding ``marker``. + + Trusted directories (ComfyUI's, the anchors) may still be resolved. + """ + real = { + "realpath": os.path.realpath, + "stat": os.stat, + "lstat": os.lstat, + } + + def guard(name): + def call(path, *args, **kwargs): + if isinstance(path, (str, bytes)) and marker in os.fsdecode(path): + test.fail(f"{name}({path!r}) touched an outside path") + return real[name](path, *args, **kwargs) + + return call + + with ( + patch("os.path.realpath", guard("realpath")), + patch("os.stat", guard("stat")), + patch("os.lstat", guard("lstat")), + ): + yield + + +class FakeMonitor: + """The image monitor as get_settings sees it: status and scheduled roots.""" + + def __init__(self, scheduled, alive=True): + self.monitored_directories = [str(path) for path in scheduled] + self.alive = alive + + def get_status(self): + return { + "running": True, + "monitored_directories": self.monitored_directories, + "handler_active": True, + "observer_alive": self.alive, + } + + +class SourcesTestCase(AdminAPITestCase): + async def asyncSetUp(self): + await super().asyncSetUp() + self.a = self.output_dir / "a" + self.b = self.output_dir / "b" + self.a.mkdir() + self.b.mkdir() + enabled = GalleryConfig.MONITORING_ENABLED + self.addCleanup(setattr, GalleryConfig, "MONITORING_ENABLED", enabled) + GalleryConfig.MONITORING_ENABLED = True + self._install_monitor(None) + + def _install_monitor(self, monitor): + import utils.image_monitor as im_mod + + orig = im_mod._monitor_instance + im_mod._monitor_instance = monitor + self.addCleanup(setattr, im_mod, "_monitor_instance", orig) + + async def _get(self): + resp = await self.client.request("GET", "/prompt_manager/settings") + body = await resp.text() + self.assertEqual(resp.status, 200, body) + self.assertNotIn(self.tmpdir, body) + return json.loads(body) + + async def _post(self, url, payload): + resp = await self.client.request("POST", url, json=payload) + body = await resp.text() + self.assertNotIn(self.tmpdir, body) + return resp.status, json.loads(body) + + def _config_bytes(self): + if not os.path.exists(self.config_path): + return None + with open(self.config_path, "rb") as f: + return f.read() + + def _states(self, data): + return {row["path"]: row["watch"] for row in data["sources"]["effective"]} + + +class TestSourceStatus(SourcesTestCase): + async def test_sources_list_configured_effective_and_auto_detected(self): + missing = self.output_dir / "gone" + missing.mkdir() + GalleryConfig.MONITORING_DIRECTORIES = [ + self._root(self.a), + self._root(missing), + ] + missing.rmdir() # an unmounted drive: configured, no longer a directory + + data = await self._get() + + self.assertEqual( + data["sources"]["configured"], + [ + { + "label": "a", + "path": "output/a", + "valid": True, + "code": None, + "reason": None, + }, + { + "label": "gone", + "path": "output/gone", + "valid": False, + "code": "not-found", + "reason": "Gallery root does not exist", + }, + ], + ) + self.assertEqual( + data["sources"]["effective"], + [{"label": "a", "path": "output/a", "root_index": 0, "watch": "manual"}], + ) + self.assertFalse(data["sources"]["auto_detected"]) + # The legacy keys are unchanged. + self.assertEqual( + data["settings"]["gallery_root_paths"], ["output/a", "output/gone"] + ) + + async def test_empty_list_means_auto_detect(self): + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a)] + self._write_config({"gallery": {"monitoring": {"directories": ["x"]}}}) + # Detection (the upward search for a ComfyUI tree) finds the output. + self.api._find_comfyui_output_dir = lambda: str(self.output_dir) + + status, saved = await self._post( + "/prompt_manager/settings", {"gallery_root_paths": []} + ) + + self.assertEqual(status, 200, saved) + self.assertEqual( + self._read_config()["gallery"]["monitoring"]["directories"], [] + ) + data = await self._get() + self.assertEqual(data["sources"]["configured"], []) + self.assertTrue(data["sources"]["auto_detected"]) + self.assertEqual( + data["sources"]["effective"], + [ + { + "label": "ComfyUI output", + "path": "output", + "root_index": 0, + "watch": "manual", + } + ], + ) + + async def test_watching_requires_live_observer_with_root(self): + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a), self._root(self.b)] + + # No monitor in this process: nothing is watched, a restart cannot help. + data = await self._get() + self.assertEqual( + data["monitor"], + {"present": False, "observer_alive": False, "scheduled": []}, + ) + self.assertEqual( + self._states(data), {"output/a": "manual", "output/b": "manual"} + ) + + # A dead observer watches nothing, even the roots it scheduled. + self._install_monitor(FakeMonitor([self.a], alive=False)) + data = await self._get() + self.assertEqual( + data["monitor"], + {"present": True, "observer_alive": False, "scheduled": ["output/a"]}, + ) + self.assertEqual( + self._states(data), + {"output/a": "restart_pending", "output/b": "restart_pending"}, + ) + + # A live observer watches its scheduled root; a root configured since + # it started waits for a restart. + self._install_monitor(FakeMonitor([self.a])) + data = await self._get() + self.assertTrue(data["monitor"]["observer_alive"]) + self.assertEqual( + self._states(data), {"output/a": "watching", "output/b": "restart_pending"} + ) + + # Monitoring switched off: the live observer still watches until the + # restart ("stopping"), and nothing else is pending. + GalleryConfig.MONITORING_ENABLED = False + data = await self._get() + self.assertFalse(data["settings"]["monitoring_enabled"]) + self.assertEqual( + self._states(data), {"output/a": "stopping", "output/b": "manual"} + ) + + async def test_scheduled_root_matches_through_its_spelling(self): + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a)] + spelled = os.path.join(str(self.output_dir), "b", "..", "a") + self._install_monitor(FakeMonitor([spelled])) + + data = await self._get() + + self.assertEqual(self._states(data), {"output/a": "watching"}) + self.assertEqual(data["monitor"]["scheduled"], ["output/a"]) + + async def test_status_failure_is_500_without_paths(self): + def boom(*args, **kwargs): + raise OSError(2, "No such file", f"{self.tmpdir}/secret") + + with patch.object(sources, "describe_sources", boom): + resp = await self.client.request("GET", "/prompt_manager/settings") + self.assertEqual(resp.status, 500) + self.assertNotIn(self.tmpdir, await resp.text()) + + +class TestMonitoringSetting(SourcesTestCase): + async def test_monitoring_enabled_strict_bool_and_restart_flag(self): + self._write_config({"custom": {"keep": True}}) + before = self._config_bytes() + for value in ("false", 0, 1, None, [], {"on": True}): + with self.subTest(value=value): + status, data = await self._post( + "/prompt_manager/settings", {"monitoring_enabled": value} + ) + self.assertEqual(status, 400, data) + self.assertFalse(data["success"]) + self.assertEqual(self._config_bytes(), before) + self.assertTrue(GalleryConfig.MONITORING_ENABLED) + + self._install_monitor(FakeMonitor([self.output_dir])) + status, data = await self._post( + "/prompt_manager/settings", {"monitoring_enabled": False} + ) + self.assertEqual(status, 200, data) + self.assertTrue(data["persisted"]) + self.assertTrue(data["restart_required"]) + self.assertEqual(data["restart_required_fields"], ["monitoring_enabled"]) + saved = self._read_config() + self.assertIs(saved["gallery"]["monitoring"]["enabled"], False) + self.assertEqual(saved["custom"], {"keep": True}) + self.assertFalse(GalleryConfig.MONITORING_ENABLED) + self.assertFalse((await self._get())["settings"]["monitoring_enabled"]) + + # Back on while the observer still runs: nothing waits for a restart. + status, data = await self._post( + "/prompt_manager/settings", {"monitoring_enabled": True} + ) + self.assertEqual(status, 200, data) + self.assertEqual(data["restart_required_fields"], []) + self.assertIs(self._read_config()["gallery"]["monitoring"]["enabled"], True) + + async def test_enabling_without_a_live_observer_needs_a_restart(self): + GalleryConfig.MONITORING_ENABLED = False + status, data = await self._post( + "/prompt_manager/settings", {"monitoring_enabled": True} + ) + self.assertEqual(status, 200, data) + self.assertEqual(data["restart_required_fields"], ["monitoring_enabled"]) + self.assertTrue(GalleryConfig.MONITORING_ENABLED) + + def test_config_file_enabled_must_be_a_bool(self): + GalleryConfig.MONITORING_ENABLED = True + with self.assertLogs("prompt_manager.config", level="WARNING"): + GalleryConfig.update_config({"monitoring": {"enabled": "false"}}) + self.assertIs(GalleryConfig.MONITORING_ENABLED, True) + GalleryConfig.update_config({"monitoring": {"enabled": False}}) + self.assertIs(GalleryConfig.MONITORING_ENABLED, False) + + +class TestSourceChangeHold(SourcesTestCase): + async def test_root_save_refused_while_scan_job_runs(self): + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a)] + self._write_config({"gallery": {"monitoring": {"directories": ["kept"]}}}) + before = self._config_bytes() + gate = asyncio.Event() + + async def runner(job): + await gate.wait() + + scan = self.api._jobs.start( + "scan", "Scan for metadata", record_changing=True, runner=runner + ) + try: + status, data = await self._post( + "/prompt_manager/settings", + {"gallery_root_paths": [str(self.b)]}, + ) + self.assertEqual(status, 409, data) + self.assertEqual(data["code"], "busy") + self.assertEqual( + data["running"], {"id": scan.id, "title": "Scan for metadata"} + ) + self.assertEqual(self._config_bytes(), before) + self.assertEqual(GalleryConfig.MONITORING_DIRECTORIES, [self._root(self.a)]) + + # A save without sources, or one resending the current sources + # (the legacy page always does), is not a source change. + status, data = await self._post( + "/prompt_manager/settings", {"result_timeout": 9} + ) + self.assertEqual(status, 200, data) + status, data = await self._post( + "/prompt_manager/settings", + {"result_timeout": 8, "gallery_root_paths": [str(self.a)]}, + ) + self.assertEqual(status, 200, data) + finally: + gate.set() + await asyncio.wait_for(scan.task, 5) + + async def test_root_save_runs_as_a_source_change_hold(self): + seen = [] + original = settings_routes.get_config_store + + def store(): + seen.extend( + (job.kind, job.title, job.admission, job.cancellable) + for job in self.api._jobs.list() + if job.running + ) + return original() + + with patch.object(settings_routes, "get_config_store", store): + status, data = await self._post( + "/prompt_manager/settings", {"gallery_root_paths": [str(self.b)]} + ) + self.assertEqual(status, 200, data) + self.assertIn(("source_change", "Source change", "records", False), seen) + self.assertFalse(any(job.running for job in self.api._jobs.list())) + + async def test_failed_root_save_ends_the_hold_failed(self): + os.makedirs(self.config_path) # config.json is a directory: unreadable + status, data = await self._post( + "/prompt_manager/settings", {"gallery_root_paths": [str(self.b)]} + ) + self.assertGreaterEqual(status, 400, data) + held = [j for j in self.api._jobs.list() if j.kind == "source_change"] + self.assertEqual([job.state for job in held], ["failed"]) + + async def test_mixed_case_windows_paths_dedupe(self): + """Two spellings of one Windows folder are saved once (normcase).""" + with ( + patch.object(sources, "source_problem", return_value=(None, "")), + patch.object( + GalleryConfig, "resolve_gallery_root", side_effect=lambda path: path + ), + patch.object(settings_routes, "_source_identity", ntpath.normcase), + patch.object(self.api, "_apply_gallery_roots", return_value=True), + ): + roots, error = self.api._parse_gallery_roots( + { + "gallery_root_paths": [ + "C:\\ComfyUI\\Output\\Renders", + "c:/comfyui/output/renders", + "C:\\ComfyUI\\output\\other", + ] + } + ) + self.assertIsNone(error) + self.assertEqual( + roots, ["C:\\ComfyUI\\Output\\Renders", "C:\\ComfyUI\\output\\other"] + ) + + async def test_case_variant_of_one_folder_is_saved_once_natively(self): + variant = str(self.a)[:-1] + "A" + if not os.path.isdir(variant): + self.skipTest("case-sensitive filesystem") + status, data = await self._post( + "/prompt_manager/settings", {"gallery_root_paths": [str(self.a), variant]} + ) + self.assertEqual(status, 200, data) + self.assertEqual(len(GalleryConfig.MONITORING_DIRECTORIES), 1) + + +class TestValidateSource(SourcesTestCase): + async def test_validate_source_writes_nothing(self): + self._write_config({"gallery": {"monitoring": {"directories": []}}}) + before = self._config_bytes() + roots = list(GalleryConfig.MONITORING_DIRECTORIES) + + status, data = await self._post(VALIDATE, {"path": str(self.a)}) + + self.assertEqual(status, 200, data) + self.assertEqual( + data, + { + "success": True, + "valid": True, + "label": "a", + "path": "output/a", + "code": None, + "reason": None, + }, + ) + self.assertEqual(self._config_bytes(), before) + self.assertEqual(GalleryConfig.MONITORING_DIRECTORIES, roots) + self.assertEqual(self.api._jobs.list(), []) + + async def test_relative_path_resolves_against_comfyui(self): + status, data = await self._post(VALIDATE, {"path": "output/b"}) + self.assertEqual((status, data["valid"], data["path"]), (200, True, "output/b")) + status, data = await self._post(VALIDATE, {"path": "output"}) + self.assertEqual(data["label"], "ComfyUI output") + + async def test_invalid_sources_answer_a_code(self): + a_file = self.output_dir / "file.png" + a_file.write_bytes(b"x") + cases = { + "": "empty", + " ": "empty", + str(self.output_dir / "missing"): "not-found", + str(a_file): "not-directory", + str(self.outside_dir): "outside-allowed", + os.path.abspath(os.sep): "outside-allowed", + os.path.expanduser("~"): "outside-allowed", + os.path.join(str(self.output_dir), "b", "..", "a"): "traversal", + "output/../output/a": "traversal", + str(self.a) + "\x00": "invalid-form", + "\\\\?\\C:\\ComfyUI\\output": "invalid-form", + "x" * 5000: "invalid-form", + } + if os.name != "nt": + cases["C:\\ComfyUI\\output"] = "invalid-form" + for path, code in cases.items(): + with self.subTest(code=code, path=path[:40]): + status, data = await self._post(VALIDATE, {"path": path}) + self.assertEqual(status, 200, data) + self.assertFalse(data["valid"]) + self.assertEqual(data["code"], code) + self.assertIsInstance(data["reason"], str) + self.assertIsNone(data["label"]) + self.assertIsNone(data["path"]) + self.assertNotIn(os.path.expanduser("~"), json.dumps(data)) + + async def test_outside_paths_answer_alike_whether_they_exist(self): + """No existence oracle: outside the allowed folders every path looks + the same, existing or not, file or folder.""" + a_file = self.outside_dir / "file.txt" + a_file.write_bytes(b"x") + answers = [] + for path in ( + self.outside_dir, + self.outside_dir / "missing", + a_file, + os.path.join(self.tmpdir, "no", "such", "tree"), + os.path.abspath(os.sep), + os.path.expanduser("~"), + ): + status, data = await self._post(VALIDATE, {"path": str(path)}) + answers.append((status, data)) + self.assertEqual(answers[0][1]["code"], "outside-allowed") + self.assertEqual([answer for answer in answers], [answers[0]] * len(answers)) + + async def test_settings_save_refuses_outside_paths_alike(self): + """The legacy save answers every outside path alike too: the + filesystem root, the home directory and a link to it included.""" + (self.outside_dir / "x").mkdir() + link = os.path.join(self.tmpdir, "link-home") + try: + os.symlink(os.path.expanduser("~"), link, target_is_directory=True) + except (OSError, NotImplementedError): + link = None + paths = [ + str(self.outside_dir / "x"), + self.tmpdir + "/nothing/x", + os.path.abspath(os.sep), + os.path.expanduser("~"), + ] + if link: + paths.append(link) + answers = [] + for path in paths: + resp = await self.client.request( + "POST", SETTINGS, json={"gallery_root_paths": [path]} + ) + body = await resp.text() + self.assertNotIn(os.path.expanduser("~"), body) + data = json.loads(body) + # Only the name the caller sent may differ. + answers.append( + ( + resp.status, + {k: v for k, v in data.items() if k not in ("source", "error")}, + ) + ) + self.assertEqual( + data["error"], + f"{data['source']} can't be used: " + f"{config_module.OUTSIDE_ALLOWED_REASON}. Remove it, then save.", + ) + self.assertEqual( + answers[0], + ( + 400, + { + "success": False, + "code": "invalid-source", + "index": 0, + "reason": config_module.OUTSIDE_ALLOWED_REASON, + }, + ), + ) + self.assertEqual(answers, [answers[0]] * len(answers)) + self.assertEqual(GalleryConfig.MONITORING_DIRECTORIES, []) + + def test_every_caller_folds_root_and_home_alike(self): + for path in (os.path.abspath(os.sep), os.path.expanduser("~")): + with self.subTest(path=path): + self.assertEqual( + GalleryConfig.gallery_root_problem(path), + ("outside-allowed", config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual( + GalleryConfig.validate_gallery_root(path), + (False, config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual( + sources.validate_source(path)["code"], "outside-allowed" + ) + + async def test_outside_path_is_never_resolved_or_stat(self): + """Containment is decided as text first: no realpath, stat or lstat of + an outside path, existing or not, by any caller (I2).""" + probe = self.outside_dir / "probe-x" + probe.mkdir() + (probe / "deep").mkdir() + cases = [str(probe), str(probe / "deep"), str(probe / "missing")] + with no_filesystem_access_for("probe-x", self): + for path in cases: + with self.subTest(path=path): + self.assertEqual( + GalleryConfig.gallery_root_problem(path)[0], "outside-allowed" + ) + self.assertEqual( + sources.validate_source(path)["code"], "outside-allowed" + ) + _roots, error = self.api._parse_gallery_roots( + {"gallery_root_paths": [path]} + ) + self.assertEqual(error["code"], "invalid-source") + status, data = await self._post(VALIDATE, {"path": path}) + self.assertEqual(data["code"], "outside-allowed") + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [path]} + ) + self.assertEqual((status, data["code"]), (400, "invalid-source")) + # A traversal that normalises outside is refused as text too. + sneaky = os.path.join( + str(self.output_dir), "..", "..", "outside", "probe-x" + ) + self.assertEqual( + GalleryConfig.gallery_root_problem(sneaky)[0], "outside-allowed" + ) + + async def test_settings_save_applies_the_form_checks(self): + """POST /settings refuses what validate-source refuses, by its form.""" + for path, code in { + str(self.a) + "\x00": "invalid-form", + os.path.join(str(self.output_dir), "b", "..", "a"): "traversal", + }.items(): + with self.subTest(code=code): + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [path]} + ) + self.assertEqual((status, data["code"]), (400, "invalid-source")) + self.assertEqual(data["reason"], sources.FORM_REASONS[code]) + windows = functools.partial(sources.source_form_error, windows=True) + with ( + patch.object(sources, "source_form_error", windows), + # The probes hold a token no trusted directory does (on Windows the + # temporary ComfyUI tree itself sits on C:). + no_filesystem_access_for("PMProbe", self), + ): + for path in ( + "\\\\?\\C:\\PMProbe\\output", + "\\\\.\\pipe\\PMProbe", + "C:\\PMProbe\\output\\CON", + "C:\\PMProbe\\output\\a:stream", + "C:PMProbe", + ): + with self.subTest(path=path): + _roots, error = self.api._parse_gallery_roots( + {"gallery_root_paths": ["output/a", path]} + ) + self.assertEqual(error["code"], "invalid-source") + self.assertEqual(error["index"], 1) + self.assertEqual( + error["reason"], sources.FORM_REASONS["invalid-form"] + ) + + async def test_inside_paths_say_why_they_are_not_usable(self): + a_file = self.output_dir / "file.png" + a_file.write_bytes(b"x") + status, missing = await self._post( + VALIDATE, {"path": str(self.output_dir / "missing")} + ) + status, not_dir = await self._post(VALIDATE, {"path": str(a_file)}) + self.assertEqual( + (missing["code"], not_dir["code"]), ("not-found", "not-directory") + ) + + async def test_malformed_request_is_400(self): + for body in ({}, {"path": 42}, {"path": None}, ["x"]): + with self.subTest(body=body): + status, data = await self._post(VALIDATE, body) + self.assertEqual(status, 400, data) + self.assertEqual(data["code"], "invalid-request") + self.assertFalse(data["success"]) + + +class TestSourceEdges(SourcesTestCase): + async def test_validation_failure_is_500_without_paths(self): + def boom(path): + raise OSError(13, "Permission denied", f"{self.tmpdir}/secret") + + with patch.object(sources, "validate_source", boom): + status, data = await self._post(VALIDATE, {"path": str(self.a)}) + self.assertEqual(status, 500) + self.assertEqual(data["error"], "Failed to check the source") + + async def test_oversized_body_is_413_too_large(self): + resp = await self.client.request( + "POST", VALIDATE, data=json.dumps({"path": "x" * (2 << 20)}) + ) + self.assertEqual(resp.status, 413) + self.assertEqual((await resp.json())["code"], "too-large") + + async def test_unreadable_monitor_counts_as_no_live_observer(self): + class Broken: + def get_status(self): + raise RuntimeError("observer gone") + + self._install_monitor(Broken()) + status, data = await self._post( + "/prompt_manager/settings", {"monitoring_enabled": True} + ) + self.assertEqual(status, 200, data) + self.assertEqual(data["restart_required_fields"], ["monitoring_enabled"]) + + def test_labels_skip_getters_that_fail_or_are_missing(self): + import sys + import types + + def fails(): + raise RuntimeError("no input dir") + + fake = types.SimpleNamespace( + get_output_directory=lambda: str(self.output_dir), + get_input_directory=fails, + get_temp_directory=lambda: None, + ) + with patch.dict(sys.modules, {"folder_paths": fake}): + self.assertEqual(sources.source_label(self.output_dir), "ComfyUI output") + self.assertEqual(sources.source_label(self.a), "a") + with patch.dict(sys.modules, {"folder_paths": None}): + self.assertEqual(sources.source_label(self.output_dir), "output") + + +class TestOutsideBaseDirectories(SourcesTestCase): + """ComfyUI folders outside the base directory (``--output-directory``): + their sources round-trip and two folders of one name stay apart (I3).""" + + async def asyncSetUp(self): + await super().asyncSetUp() + self.renders = self.outside_dir / "renders" + self.input_dir = self.comfy_dir / "input" + (self.renders / "portraits").mkdir(parents=True) + (self.input_dir / "portraits").mkdir(parents=True) + sys.modules["folder_paths"] = types.SimpleNamespace( + base_path=str(self.comfy_dir), + get_output_directory=lambda: str(self.renders), + get_input_directory=lambda: str(self.input_dir), + ) + + async def test_source_outside_base_round_trips(self): + status, rendered = await self._post( + VALIDATE, {"path": str(self.renders / "portraits")} + ) + self.assertEqual( + (rendered["valid"], rendered["path"]), (True, "renders/portraits") + ) + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [rendered["path"]]} + ) + self.assertEqual(status, 200, data) + self.assertEqual( + GalleryConfig.MONITORING_DIRECTORIES, + [self._root(self.renders / "portraits")], + ) + configured = (await self._get())["sources"]["configured"] + self.assertEqual( + [(row["path"], row["valid"]) for row in configured], + [("renders/portraits", True)], + ) + # The page resends what it read: still the same folder, no change. + status, data = await self._post( + SETTINGS, {"gallery_root_paths": ["renders/portraits"]} + ) + self.assertEqual((status, data["restart_required_fields"]), (200, [])) + + async def test_same_name_folders_stay_apart(self): + answers = [] + for path in (self.renders / "portraits", self.input_dir / "portraits"): + status, data = await self._post(VALIDATE, {"path": str(path)}) + answers.append(data["path"]) + self.assertEqual(answers, ["renders/portraits", "input/portraits"]) + status, data = await self._post(SETTINGS, {"gallery_root_paths": answers}) + self.assertEqual(status, 200, data) + sources_ = (await self._get())["sources"] + self.assertEqual([row["path"] for row in sources_["configured"]], answers) + self.assertEqual([row["path"] for row in sources_["effective"]], answers) + + +class TestUnusableSourceSave(SourcesTestCase): + async def test_unusable_source_is_named_and_saves_nothing(self): + """A configured source that went missing blocks the save and is named.""" + gone = self.output_dir / "gone" + gone.mkdir() + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a), self._root(gone)] + self._write_config({"custom": 1}) + before = self._config_bytes() + gone.rmdir() # an unplugged drive + + status, data = await self._post( + SETTINGS, + { + "gallery_root_paths": ["output/a", "output/gone", "output/b"], + "worker_threads": 2, + }, + ) + + self.assertEqual(status, 400, data) + self.assertEqual( + data, + { + "success": False, + "code": "invalid-source", + "source": "output/gone", + "index": 1, + "reason": "Gallery root does not exist", + "error": "output/gone can't be used: Gallery root does not exist. " + "Remove it or reconnect it, then save.", + }, + ) + self.assertEqual(self._config_bytes(), before) + + async def test_malformed_roots_are_coded_invalid_request(self): + for roots in ("output/a", [1]): + with self.subTest(roots=roots): + status, data = await self._post(SETTINGS, {"gallery_root_paths": roots}) + self.assertEqual((status, data["code"]), (400, "invalid-request")) + + +class TestUnchangedRootsTakeNoHold(SourcesTestCase): + async def test_resent_spelling_of_configured_roots_is_no_change(self): + """config.json holds a hand-written spelling; the legacy page resends + it resolved: no source change, so no hold and no 409 during a scan (M1).""" + GalleryConfig.MONITORING_DIRECTORIES = ["output/a"] + gate = asyncio.Event() + + async def runner(job): + await gate.wait() + job.finish("finished") + + self.api._jobs.start( + "scan", "Metadata scan", record_changing=True, runner=runner + ) + try: + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [str(self.a)], "result_timeout": 30} + ) + self.assertEqual(status, 200, data) + self.assertEqual(data["restart_required_fields"], []) + self.assertEqual([j.kind for j in self.api._jobs.list()], ["scan"]) + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [str(self.b)]} + ) + self.assertEqual((status, data["code"]), (409, "busy")) + finally: + gate.set() + + +class TestWindowsContainment(unittest.TestCase): + """Windows path forms, with ntpath fakes: anything outside the allowed + folders (a UNC share, the verbatim and device namespaces, another drive) + is refused as text, before any path is resolved or opened (I2).""" + + ALLOWED = "c:\\comfyui\\output" + NAS = "\\\\nas\\share\\renders" + + def setUp(self): + self.touched = [] + + def realpath(path, *args, **kwargs): + self.touched.append(path) + return ntpath.normcase(ntpath.normpath(path)) + + def probe(path): + self.touched.append(path) + return True + + names = ("normcase", "normpath", "isabs", "join", "dirname", "basename", "sep") + fake_path = types.SimpleNamespace( + **{name: getattr(ntpath, name) for name in names}, + realpath=realpath, + exists=probe, + isdir=probe, + expanduser=lambda path: "c:\\users\\someone", + ) + fake_os = types.SimpleNamespace( + path=fake_path, environ={}, pathsep=";", name="nt" + ) + for target, value in ( + (patch.object(config_module, "os", fake_os), None), + ( + patch.object( + GalleryConfig, + "_allowed_parent_pairs", + classmethod( + lambda cls: [(self.ALLOWED, self.ALLOWED), (self.NAS, self.NAS)] + ), + ), + None, + ), + ( + patch.object( + GalleryConfig, + "path_anchors", + classmethod(lambda cls: ["c:\\comfyui", "\\\\nas\\share"]), + ), + None, + ), + ): + target.start() + self.addCleanup(target.stop) + + def test_outside_forms_are_refused_untouched(self): + for path in ( + "\\\\attacker\\share\\x", + "//attacker/share/x", + "\\\\?\\C:\\ComfyUI\\output\\a", + "\\\\?\\UNC\\attacker\\share", + "\\\\.\\pipe\\x", + "\\\\.\\C:\\ComfyUI\\output", + "D:\\Renders", + "C:\\Windows", + "C:\\ComfyUI\\output\\..\\..\\Windows", + "C:\\ComfyUI\\output2", + "\\\\nas\\share\\other", + "..\\..\\Windows", + ): + with self.subTest(path=path): + self.touched.clear() + self.assertEqual( + GalleryConfig.gallery_root_problem(path), + ("outside-allowed", config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual(self.touched, []) + + def test_allowed_spellings_are_accepted(self): + for path in ( + "C:\\ComfyUI\\output\\a", + "c:/comfyui/OUTPUT/a", + "output\\a", + "\\\\NAS\\share\\renders\\a", # a UNC spelling of an allowed extra root + ): + with self.subTest(path=path): + self.assertEqual(GalleryConfig.gallery_root_problem(path), (None, "")) + + +class TestSourceFormError(unittest.TestCase): + """The lexical check that runs before any filesystem access.""" + + def test_windows_forms(self): + err = sources.source_form_error + for path in ( + "C:\\ComfyUI\\output", + "C:/ComfyUI/output", + "\\\\server\\share\\output", + "D:\\images", + ): + with self.subTest(path=path): + self.assertIsNone(err(path, windows=True)) + for path, code in { + "C:output": "invalid-form", # drive-relative + "C:\\out\\a:stream": "invalid-form", # alternate data stream + "C:\\out\\CON": "invalid-form", + "C:\\out\\nul.txt": "invalid-form", + "C:\\out\\folder.": "invalid-form", + "C:\\out\\folder \\sub": "invalid-form", + "\\\\?\\C:\\out": "invalid-form", + "//?/C:/out": "invalid-form", + "\\\\.\\pipe\\x": "invalid-form", + "\\\\server": "invalid-form", + "C:\\out\\..\\in": "traversal", + "output\\..\\input": "traversal", + }.items(): + with self.subTest(path=path): + self.assertEqual(err(path, windows=True), code) + + def test_posix_forms(self): + err = sources.source_form_error + for path in ("/srv/comfy/output", "output/renders", "out/CON", "a b/c."): + with self.subTest(path=path): + self.assertIsNone(err(path, windows=False)) + for path, code in { + "C:\\ComfyUI\\output": "invalid-form", + "C:/ComfyUI/output": "invalid-form", + "\\\\server\\share": "invalid-form", + "out\\renders": "invalid-form", + "/srv/../etc": "traversal", + "../output": "traversal", + "/srv/out\x00": "invalid-form", + "/srv/out\n": "invalid-form", + "": "empty", + }.items(): + with self.subTest(path=path): + self.assertEqual(err(path, windows=False), code) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 177df6b..75926eb 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -7,7 +7,7 @@ }, "api-client.js": { "lines": 98, - "branches": 95, + "branches": 96, "functions": 98, "statements": 98 }, @@ -163,7 +163,7 @@ }, "workspace/activity-scan.mjs": { "lines": 96, - "branches": 73, + "branches": 78, "functions": 98, "statements": 96 }, @@ -467,6 +467,12 @@ "functions": 98, "statements": 96 }, + "workspace/system-sources.mjs": { + "lines": 98, + "branches": 88, + "functions": 95, + "statements": 98 + }, "workspace/system-state.mjs": { "lines": 98, "branches": 94, @@ -476,7 +482,7 @@ "workspace/system.mjs": { "lines": 98, "branches": 92, - "functions": 85, + "functions": 86, "statements": 98 }, "workspace/theme.mjs": { diff --git a/web/js/api-client.js b/web/js/api-client.js index b334467..ee21d36 100644 --- a/web/js/api-client.js +++ b/web/js/api-client.js @@ -40,6 +40,21 @@ Number.isSafeInteger(current.tag_revision) && current.tag_revision >= 0) { this.current = { tags: current.tags.slice(), tag_revision: current.tag_revision }; } + // A busy answer names the running job that holds the slot; an + // invalid-source answer names the source it refused. Only + // well-formed values are kept. + this.running = null; + const running = current && current.running; + if (this.code === "busy" && running && typeof running.id === "string" && + /^[A-Za-z0-9_-]{1,64}$/.test(running.id) && typeof running.title === "string" && + running.title.length > 0 && running.title.length <= 200) { + this.running = { id: running.id, title: running.title }; + } + this.source = null; + if (this.code === "invalid-source" && current && typeof current.source === "string" && + current.source.length > 0 && current.source.length <= 4096) { + this.source = current.source; + } } } diff --git a/web/js/workspace/activity-scan.mjs b/web/js/workspace/activity-scan.mjs index 8ec2c87..f92af7b 100644 --- a/web/js/workspace/activity-scan.mjs +++ b/web/js/workspace/activity-scan.mjs @@ -17,6 +17,8 @@ export const REPORT_DONE = 'Job report downloaded.'; export const REPORT_FAILED = 'The report could not be downloaded. Try again.'; const JOBS_REPORT_BASE = '/prompt_manager/jobs'; export const START_FAILED = 'The scan could not start. Try again.'; +// A per-source scan whose sources changed since they were read (S4): the server's 409 says so. +const SOURCES_CHANGED_CODE = 'sources-changed'; const FALLBACK_NAME = 'prompts_backup.db'; const REPORT_FALLBACK_NAME = 'prompt-manager-report.json'; @@ -81,25 +83,37 @@ export function busyToast(running) { /** * Start the scan job over `files` ('new': new and unresolved files, or 'all') and open its view; a refused or failed - * start says why and stays on the page. + * start says why and stays on the page. `roots` (System › Sources, S4) limits it to those effective source indexes, and + * `paths` names the public path read for each: when the sources changed since (409 `sources-changed`) nothing starts, + * the toast says the server's reason and `onStale` (which every caller with `roots` passes) reads them again. */ -export async function startMetadataScan({ store, scope, files = 'new', notify = showWorkspaceToast, navigate = navigateTo }) { +export async function startMetadataScan({ store, scope, files = 'new', roots = null, paths = null, onStale, + notify = showWorkspaceToast, navigate = navigateTo }) { try { - const result = await store.start('scan', { scope: files }); + const result = await store.start('scan', roots ? { scope: files, roots, paths } : { scope: files }); if (scope.signal.aborted) return; if (result.busy !== undefined) { notify(busyToast(result.busy)); return; } navigate(`#/activity/${result.job.id}`); - } catch { - if (!scope.signal.aborted) notify(START_FAILED); + } catch (error) { + if (scope.signal.aborted) return; + if (error?.code === SOURCES_CHANGED_CODE) { + notify(error.message); + onStale(); + return; + } + notify(START_FAILED); } } -/** Open "Review metadata scan" for `sources` output folders. */ -export function openMetadataScan({ scope, store, sources, trigger = null }) { +/** + * Open "Review metadata scan" for `sources` output folders; with `roots` and `paths` (one source's index and public + * path, System › Sources) the scan reads only those, and `onStale` runs when they changed since they were read. + */ +export function openMetadataScan({ scope, store, sources, roots = null, paths = null, onStale, trigger = null }) { openScanReviewDialog({ scope, sources, trigger, onBackup: () => downloadDatabaseBackup(), - onStart: ({ scope: files }) => startMetadataScan({ store, scope, files }) }); + onStart: ({ scope: files }) => startMetadataScan({ store, scope, files, roots, paths, onStale }) }); } diff --git a/web/js/workspace/system-sources.mjs b/web/js/workspace/system-sources.mjs new file mode 100644 index 0000000..a204f07 --- /dev/null +++ b/web/js/workspace/system-sources.mjs @@ -0,0 +1,395 @@ +/** + * System › Sources & scans (#249 System S4 Task 11): the concept's `renderSystem` "sources" body (app.js 679–681). + * + * - The optional `.notice` "Bring older images into your prompt history" shows until this browser dismissed the 3.2.0 + * update notice (`pm_update_notice_dismissed`, the legacy admin page's own key). Later dismisses it without scanning; + * Review scan dismisses it, as the legacy page's Scan does, and reviews every source. + * - "Image sources" lists the server's sources (GET /settings, S4 Task 9) in `#source-list`, one `.operation` each: + * its label, "<path> · watching | watching until restart | manual scan | restart pending" and "Scan changes" + * (watched) or "Scan folder", which opens the shared "Review metadata scan" (D55) for that one source ("1 output + * folder") and starts the session job over it (`roots` with its public path, S4 Task 10). The job opens in Activity; + * the toast only says it started. When the sources changed since they were read, nothing starts: the toast says so + * and the list is read again. + * - "+ Add source" asks the server whether a folder can be a source (`validate-source`, which writes nothing), then adds + * it to the host draft of `gallery_root_paths`; Remove takes one out. Both wait for the one Save row. + * - "Watch for new outputs" (`monitoring_enabled`) and "Worker threads" (`worker_threads`, a range up to the server's + * `cpu_count`, D113) are host drafts too. "Enrich existing metadata" reviews a scan of every source. + * - "Thumbnail generation → Review options" opens the existing gallery's thumbnail tool until its review dialog (S5), + * the D94 hand-off for the one capability not native yet. + * + * D98: per-source Remove; an added source reads "not saved yet" until Save; the line under the list says when + * ComfyUI's output folder is used instead; "restart pending" for a source the running monitor does not watch yet; the + * "Refresh files" operation (`/gallery/rescan`) closes the surface. D127: the add dialog's field is "Folder path", + * starts empty and says the server's reason in place; its action is "Add", and Enter adds too. + * D128: "watching until restart" for a source the running monitor still watches after monitoring was switched off; + * "same folder as another source" for a configured source the effective list already holds under another spelling; a + * save refused for one source (`invalid-source`) marks that row and the status says the server's reason. + */ +import { createButton } from '../shared/toolkit/core.js'; +import { closeWorkspaceDialog, field, openContentDialog } from './workspace-dialogs.mjs'; +import { openMetadataScan } from './activity-scan.mjs'; +import { settingRow, operationRow } from './system-controls.mjs'; +import { HOST_UNAVAILABLE } from './system-appearance.mjs'; +import { showWorkspaceToast } from './toast.mjs'; +import { noun } from './plural.mjs'; + +const SETTINGS_URL = '/prompt_manager/settings'; +const VALIDATE_URL = '/prompt_manager/settings/validate-source'; +const RESCAN_URL = '/prompt_manager/gallery/rescan'; +const GALLERY_URL = '/prompt_manager/gallery'; +const ROOTS_KEY = 'gallery_root_paths'; + +export const NOTICE_KEY = 'pm_update_notice_dismissed'; +export const NOTICE_VERSION = '3.2.0'; +export const WATCH_COPY = Object.freeze({ watching: 'watching', stopping: 'watching until restart', manual: 'manual scan', + restart_pending: 'restart pending' }); +// The states whose observer runs now: their scan only has to catch up on changes. +const WATCHED = new Set(['watching', 'stopping']); +const NOT_SAVED = 'not saved yet'; +export const DUPLICATE_SOURCE = 'same folder as another source'; +/** D98: the line under the source list when it does not say everything by itself. */ +export const NOTES = Object.freeze({ + removedAll: 'No source is configured. After saving, ComfyUI’s output folder is used.', + autoDetected: 'No source is configured; ComfyUI’s output folder is used.', + fallback: 'No configured source is available; ComfyUI’s output folder is used.', + none: 'No source is available.', +}); +export const LOADING_TEXT = 'Loading sources…'; +export const LOAD_FAILED_TEXT = 'Sources could not be loaded. Your saved data has not changed.'; +export const DUPLICATE_TEXT = 'This folder is already a source.'; +export const VALIDATE_FAILED_TEXT = 'The folder could not be checked. Try again.'; +export const REFRESH_FAILED_TEXT = 'Files could not be refreshed. Try again.'; + +// Labels of the sources added in this page session (validate-source answers them), for the rows not saved yet. +const addedLabels = new Map(); + +const defaultStorage = () => window.localStorage; + +/** Whether this browser dismissed the 3.2.0 notice; storage that cannot be read shows it. */ +export function noticeDismissed(getStorage = defaultStorage) { + try { + return getStorage().getItem(NOTICE_KEY) === NOTICE_VERSION; + } catch { + return false; + } +} + +/** Dismiss the notice for this browser; false when storage refused (it stays dismissed for this view). */ +export function dismissNotice(getStorage = defaultStorage) { + try { + getStorage().setItem(NOTICE_KEY, NOTICE_VERSION); + return true; + } catch { + return false; + } +} + +const sameRoots = (a, b) => Array.isArray(a) && Array.isArray(b) && a.length === b.length && a.every((path, index) => path === b[index]); + +/** The roots to draft: the saved list itself when they are equal, so the draft is clean again. */ +export function draftRoots(next, saved) { + return sameRoots(next, saved) ? saved : [...next]; +} + +const folderName = path => path.split('/').filter(Boolean).at(-1) ?? path; + +function effectiveRow(source, remove) { + const watched = WATCHED.has(source.watch); + return { key: remove ?? `auto:${source.root_index}`, title: source.label, + description: `${source.path} · ${WATCH_COPY[source.watch] ?? WATCH_COPY.manual}`, + scan: { label: watched ? 'Scan changes' : 'Scan folder', root: source.root_index, path: source.path }, remove }; +} + +/** + * The source list for `sources` (GET /settings' `sources`), the saved `gallery_root_paths` and the drafted ones + * (`current`): `{rows: [{key, title, description, scan: {label, root, path} | null, remove: path | null}], note}`. + * A valid configured source missing from the effective list is another spelling of a folder already listed. + */ +export function sourceRows({ sources, saved, current, labels = addedLabels }) { + const configured = sources.configured ?? []; + const effective = sources.effective ?? []; + const savedRoots = saved ?? configured.map(source => source.path); + const roots = current ?? savedRoots; + const dirty = !sameRoots(roots, savedRoots); + const rows = []; + for (const path of new Set(roots)) { + const entry = configured.find(source => source.path === path); + const live = entry?.valid ? effective.find(source => source.path === path) : undefined; + if (live) rows.push(effectiveRow(live, path)); + else if (entry) { + const why = entry.valid ? DUPLICATE_SOURCE : entry.reason ?? WATCH_COPY.manual; + rows.push({ key: path, title: entry.label, description: `${path} · ${why}`, scan: null, remove: path }); + } else rows.push({ key: path, title: labels.get(path) ?? folderName(path), description: `${path} · ${NOT_SAVED}`, scan: null, remove: path }); + } + if (dirty) return { rows, note: roots.length ? null : NOTES.removedAll }; + if (sources.auto_detected) { + rows.push(...effective.map(source => effectiveRow(source, null))); + return { rows, note: configured.length ? NOTES.fallback : NOTES.autoDetected }; + } + return { rows, note: rows.length ? null : NOTES.none }; +} + +/** The Refresh files toast for the rescan's `{total, roots}`. */ +export function refreshedText(result) { + const { total, roots } = result ?? {}; + if (![total, roots].every(value => Number.isSafeInteger(value) && value >= 0)) return 'Files refreshed.'; + return `Files refreshed. ${total.toLocaleString('en-US')} ${noun(total, 'file')} in ${roots.toLocaleString('en-US')} output ${noun(roots, 'folder')}.`; +} + +function el(tag, text, className) { + const node = document.createElement(tag); + if (text !== undefined) node.textContent = text; + if (className) node.className = className; + return node; +} + +/** The concept's `.notice` with Later and Review scan. */ +function noticeNode({ scope, onLater, onReview }) { + const notice = el('div', undefined, 'notice'); + const text = el('div'); + text.append(el('h2', 'Bring older images into your prompt history'), + el('p', 'The schema upgrades automatically. An optional metadata scan links existing positives and negatives without regenerating all thumbnails.')); + const actions = el('div', undefined, 'row-actions'); + const later = createButton('Later'); + const review = createButton('Review scan', 'ca-primary'); + scope.listen(later, 'click', onLater); + scope.listen(review, 'click', onReview); + actions.append(later, review); + notice.append(text, actions); + return notice; +} + +/** + * "Add image source" (the concept's `simpleInput`, D127): the server checks the folder; a refused one keeps the + * dialog open with the server's reason under the field. Enter in the field adds, as the Add button does. + */ +function openAddSource({ api, scope, trigger, listed, onAdd }) { + const input = el('input'); + const reason = el('p', undefined, 'small'); + reason.setAttribute('role', 'alert'); + const cancel = createButton('Cancel'); + const add = createButton('Add', 'ca-primary'); + let open = true; + const refuse = text => { + reason.textContent = text; + if (!reason.isConnected) actions.before(reason); + input.setAttribute('aria-invalid', 'true'); + input.focus(); + }; + scope.listen(input, 'input', () => { + reason.remove(); + input.removeAttribute('aria-invalid'); + }); + scope.listen(input, 'keydown', event => { + if (event.key !== 'Enter' || event.isComposing) return; + event.preventDefault(); + submit(); + }); + scope.listen(cancel, 'click', () => closeWorkspaceDialog()); + scope.listen(add, 'click', () => submit()); + async function submit() { + const path = input.value.trim(); + if (!path) { input.focus(); return; } + if (add.getAttribute('aria-disabled') === 'true') return; + add.setAttribute('aria-disabled', 'true'); + try { + const result = await api.post(VALIDATE_URL, { path }, { signal: scope.signal }); + if (!open) return; + if (!result.valid) refuse(result.reason ?? VALIDATE_FAILED_TEXT); + else if (listed().includes(result.path)) refuse(DUPLICATE_TEXT); + else { + closeWorkspaceDialog(); + onAdd(result); + } + } catch { + if (open) refuse(VALIDATE_FAILED_TEXT); + } finally { + add.removeAttribute('aria-disabled'); + } + } + const actions = el('div', undefined, 'dialog-actions'); + actions.append(cancel, add); + openContentDialog({ scope, title: 'Add image source', trigger, nodes: [field('Folder path', input, 'action-input'), actions], + onClose: () => { open = false; } }); + input.focus(); +} + +/** The row's control (`data-setting`), wherever the Toolkit switch nests it. */ +const controlOf = (row, id) => row.querySelector(`[data-setting="${id}"]`); + +/** + * Mount Sources in `container` (System's `.settings-layout`); returns the section's nodes. `activity` is the page's + * Activity store (scans start through it), `drafts` the System drafts. + */ +export function mountSources(container, { api, activity, scope, drafts, notify = showWorkspaceToast, storage = defaultStorage, + openWindow = (url) => window.open(url, '_blank', 'noopener'), latestOnly = window.ApiClient.latestOnly, + STALE = window.ApiClient.STALE }) { + const statusRead = latestOnly({ lifetime: scope.signal }); + // undefined while read, null when the read failed, else GET /settings' `sources`. + let sources; + const nodes = []; + const scanAll = trigger => openMetadataScan({ scope, store: activity, trigger, sources: sources?.effective?.length ?? 0 }); + + if (!noticeDismissed(storage)) { + const notice = noticeNode({ scope, + onLater: () => { dismissNotice(storage); notice.remove(); }, + onReview: () => { + dismissNotice(storage); + // The pressed button leaves with the notice, so focus returns to the list's Add source. + notice.remove(); + scanAll(addButton); + } }); + nodes.push(notice); + } + + const surface = el('section', undefined, 'surface'); + const heading = el('div', undefined, 'panel-heading'); + const addButton = createButton('+ Add source'); + heading.append(el('h2', 'Image sources'), addButton); + const list = el('div'); + list.id = 'source-list'; + surface.append(heading, el('p', 'The same folders power browsing, monitoring and metadata enrichment.'), list); + + const roots = () => drafts.get('host', ROOTS_KEY) ?? sources?.configured?.map(source => source.path) ?? []; + const setRoots = next => drafts.set('host', ROOTS_KEY, draftRoots(next, drafts.saved('host')[ROOTS_KEY])); + scope.listen(addButton, 'click', () => openAddSource({ api, scope, trigger: addButton, listed: roots, + onAdd: ({ path, label }) => { + addedLabels.set(path, label); + setRoots([...roots(), path]); + } })); + + const monitoring = settingRow({ id: 'monitoring', label: 'Watch for new outputs', + description: 'New files enter the same library. Existing scans do not count as new runs.', control: { type: 'toggle' }, + scope, onChange: value => drafts.set('host', 'monitoring_enabled', value) }); + const workers = settingRow({ id: 'workers', label: 'Worker threads', + description: 'Balance background scans and thumbnail work against generation.', control: { type: 'range', value: 4 }, + scope, onChange: value => drafts.set('host', 'worker_threads', value) }); + const hostRows = [[monitoring, 'monitoring', 'monitoring_enabled'], [workers, 'workers', 'worker_threads']] + .map(([row, id, key]) => ({ input: controlOf(row, id), key, text: row.children[0], note: null })); + for (const row of hostRows) row.input.dataset.key = row.key; + surface.append(monitoring, workers, + operationRow({ scope, title: 'Enrich existing metadata', description: 'Resolve both prompts, deduplicate recipes, preserve uncertain links.', + label: 'Review scan', onClick: scanAll }), + // D94: the gallery keeps thumbnail generation until its review dialog (S5). + operationRow({ scope, title: 'Thumbnail generation', description: 'Generate missing previews; rebuilding all is a separate explicit option.', + label: 'Review options', onClick: () => openWindow(window.ApiClient.resolveUrl(GALLERY_URL)) }), + // D98: the legacy gallery's Refresh, which rereads every source's file list. + operationRow({ scope, title: 'Refresh files', description: 'Read every source again to find files added or removed outside ComfyUI.', + label: 'Refresh', onClick: refresh })); + nodes.push(surface); + + async function refresh(button) { + if (button.getAttribute('aria-disabled') === 'true') return; + button.setAttribute('aria-disabled', 'true'); + try { + const result = await api.post(RESCAN_URL, {}, { signal: scope.signal }); + if (!scope.signal.aborted) notify(refreshedText(result)); + } catch { + if (!scope.signal.aborted) notify(REFRESH_FAILED_TEXT); + } finally { + button.removeAttribute('aria-disabled'); + } + } + + function sourceNode(row) { + const actions = []; + if (row.scan) { + const scan = createButton(row.scan.label); + scope.listen(scan, 'click', () => openMetadataScan({ scope, store: activity, trigger: scan, sources: 1, + roots: [row.scan.root], paths: [row.scan.path], onStale: reread })); + actions.push(scan); + } + if (row.remove !== null) { + // D98: a plain button; nothing is removed until Save. + const remove = createButton('Remove'); + scope.listen(remove, 'click', () => setRoots(roots().filter(path => path !== row.remove))); + actions.push(remove); + } + const node = el('div', undefined, 'operation'); + // D128: the source the last save was refused for; the status says why. + if (row.remove !== null && row.remove === drafts.invalidSource?.()) node.setAttribute('aria-invalid', 'true'); + const text = el('div'); + text.append(el('h3', row.title), el('p', row.description)); + const group = el('div', undefined, 'row-actions'); + group.append(...actions); + node.append(text, group); + return node; + } + + function paintList() { + if (sources === undefined) { + const loading = el('p', LOADING_TEXT, 'small'); + loading.setAttribute('role', 'status'); + list.replaceChildren(loading); + return; + } + if (sources === null) { + const alert = el('p', LOAD_FAILED_TEXT); + alert.setAttribute('role', 'alert'); + const retry = createButton('Retry'); + scope.listen(retry, 'click', () => { + load(); + if (drafts.hostState?.() === 'failed') drafts.reloadHost(); + }); + list.replaceChildren(alert, retry); + return; + } + const { rows, note } = sourceRows({ sources, saved: drafts.saved('host')[ROOTS_KEY], current: drafts.get('host', ROOTS_KEY) }); + list.replaceChildren(...rows.map(sourceNode), ...(note ? [el('p', note, 'small')] : [])); + } + + function paintControls() { + const unavailable = drafts.hostState?.() === 'failed'; + for (const row of hostRows) { + const value = drafts.get('host', row.key); + row.input.disabled = value === undefined; + if (row.key === 'worker_threads') { + // D113: the range ends at the server's core count (the concept's fixed 16). + const cpus = drafts.get('host', 'cpu_count'); + if (Number.isSafeInteger(cpus) && cpus >= 1) row.input.max = String(cpus); + if (value !== undefined) row.input.value = String(value); + } else if (value !== undefined) row.input.checked = value === true; + 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; } + } + } + + // The sources changed since this page read them (a refused per-source scan): read them and the saved values again. + function reread() { + load(); + drafts.reloadHost?.(); + } + + async function load() { + sources = undefined; + paintList(); + let answer; + try { + answer = await statusRead(signal => api.get(SETTINGS_URL, { signal, cache: 'no-store' })); + } catch { + answer = null; + } + if (answer === STALE || scope.signal.aborted) return; + sources = answer?.sources ?? null; + paintList(); + } + + // A save (or a reread after a conflict) changed the saved sources or monitoring: read their state again. The first + // host read on mount is not a change. + const savedState = () => { + const saved = drafts.saved('host'); + return saved[ROOTS_KEY] === undefined ? undefined : JSON.stringify([saved[ROOTS_KEY], saved.monitoring_enabled]); + }; + let state = savedState(); + scope.cleanup(drafts.subscribe(() => { + const previous = state; + state = savedState(); + paintControls(); + if (previous !== undefined && state !== previous) load(); + else if (sources) paintList(); + })); + paintControls(); + load(); + return nodes; +} diff --git a/web/js/workspace/system-state.mjs b/web/js/workspace/system-state.mjs index b827dc3..a0d29ff 100644 --- a/web/js/workspace/system-state.mjs +++ b/web/js/workspace/system-state.mjs @@ -17,8 +17,10 @@ * Adapters: `readBrowser()` and `readRuntime()` return the values; `readHost()` returns `{values, revision}`. * `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. + * `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 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). */ export { SYSTEM_SECTIONS, isSystemSection } from './state.mjs'; @@ -32,21 +34,35 @@ export const SETTINGS_STATUS = Object.freeze({ partial: 'Some changes were not saved. Review the highlighted settings.', 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. + restart: 'Restart ComfyUI to apply:', }); +/** D128: the settings a host save can report in `restart_required_fields`, by their row label. */ +export const RESTART_LABELS = Object.freeze({ monitoring_enabled: 'Watch for new outputs' }); + export const DRAFT_DOMAINS = Object.freeze(['browser', 'host', 'runtime']); const CONFLICT_CODE = 'settings-changed'; +const INVALID_SOURCE_CODE = 'invalid-source'; const ADAPTERS = ['readBrowser', 'readHost', 'readRuntime', 'writeBrowser', 'writeHost', 'writeRuntime']; -/** The status copy of one `save()` outcome (`{browser, host, runtime}`, each clean/saved/tab-only/applied/failed/conflict). */ +/** + * The status copy of one `save()` outcome (`{browser, host, runtime}`, each clean/saved/tab-only/applied/failed/conflict). + * `restart` lists the host fields the server applies at its next start, `invalid` the source a refused host save named + * (`{source, message}`): its message is the status (D128). + */ 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 SETTINGS_STATUS.partial; + if (results.includes('failed')) return outcome?.invalid?.message || SETTINGS_STATUS.partial; const saved = []; if (outcome?.browser === 'saved') saved.push(SETTINGS_STATUS.browser); if (outcome?.browser === 'tab-only') saved.push(SETTINGS_STATUS.tabOnly); - if (outcome?.host === 'saved') saved.push(SETTINGS_STATUS.host); + if (outcome?.host === 'saved') { + saved.push(SETTINGS_STATUS.host); + const restart = (outcome.restart ?? []).map(field => RESTART_LABELS[field]).filter(Boolean); + if (restart.length) saved.push(`${SETTINGS_STATUS.restart} ${restart.join(', ')}.`); + } if (outcome?.runtime === 'applied') saved.push(SETTINGS_STATUS.runtime); return saved.join(' ') || SETTINGS_STATUS.clean; } @@ -68,6 +84,8 @@ export function createSystemDrafts(adapters) { let hostRead = 'pending'; let outcome = null; let saving = null; + // What the last host save said beyond its result: the restart fields, or the source it refused. + let hostDetail = {}; const notify = () => { for (const listener of [...listeners]) listener(); }; const changes = domain => Object.fromEntries(drafts[domain]); @@ -150,6 +168,8 @@ export function createSystemDrafts(adapters) { if (result?.code === CONFLICT_CODE) { fail(domain, written); return 'conflict'; } if (!result?.success || result.persisted !== true) { fail(domain, written); return 'failed'; } if (typeof result.revision === 'string') revision = result.revision; + const restart = result.restart_required_fields; + if (Array.isArray(restart)) hostDetail.restart = restart.filter(field => typeof field === 'string'); commit(domain, written); return 'saved'; } @@ -158,6 +178,9 @@ export function createSystemDrafts(adapters) { 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 }; + } return error?.code === CONFLICT_CODE ? 'conflict' : 'failed'; } } @@ -172,8 +195,10 @@ export function createSystemDrafts(adapters) { function save() { saving ??= (async () => { for (const domain of DRAFT_DOMAINS) failedKeys[domain].clear(); + hostDetail = {}; const result = {}; for (const domain of DRAFT_DOMAINS) result[domain] = await saveDomain(domain); + Object.assign(result, hostDetail); // 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(() => {}); @@ -228,6 +253,8 @@ export function createSystemDrafts(adapters) { changes, saving: () => saving !== null, failed: domain => { requireDomain(domain); return new Set(failedKeys[domain]); }, + /** The source the last save was refused for (its public path), until the next edit; else null. */ + invalidSource: () => outcome?.invalid?.source ?? null, /** The Save row's text: the last save's outcome until the next edit, else unsaved or clean. */ statusText: () => (outcome ? statusText(outcome) : dirty() ? SETTINGS_STATUS.dirty : SETTINGS_STATUS.clean), subscribe(listener) { diff --git a/web/js/workspace/system.mjs b/web/js/workspace/system.mjs index 27e1a54..1b4b870 100644 --- a/web/js/workspace/system.mjs +++ b/web/js/workspace/system.mjs @@ -4,8 +4,8 @@ * `aria-pressed`; a press navigates to its hash), then `.settings-layout` with the section's body and the Save row * (`span.small#settings-status` + "Save preferences"). D92: no "Sample configuration" chip, the status copy is * truthful and "Save preferences" is aria-disabled while nothing is unsaved. A section not delivered yet shows the - * per-section hand-off to its legacy tool (D94), never mock placeholders. Overview (system-overview.mjs) and - * Appearance (system-appearance.mjs) are native. + * per-section hand-off to its legacy tool (D94), never mock placeholders. Overview (system-overview.mjs), + * Appearance (system-appearance.mjs) and Sources & scans (system-sources.mjs) are native. * * Switching sections keeps the shell (and focus on the pressed button) and disposes the old section's scope, so its * listeners and timers never outlive it. The drafts are module-level (Q10): they live for the page session. @@ -16,6 +16,9 @@ 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 { mountSources } from './system-sources.mjs'; +import { busyToast } from './activity-scan.mjs'; +import { showWorkspaceToast } from './toast.mjs'; import { displayPreferences, readPreferences, reconcilePreferences, subscribePreferences, writePreferences } from './preferences.mjs'; const SETTINGS_URL = '/prompt_manager/settings'; @@ -27,7 +30,6 @@ const GALLERY = Object.freeze({ href: '/prompt_manager/gallery', label: 'Open ex * settings, thumbnails, duplicates and cache tools. */ export const SECTION_HANDOFFS = Object.freeze({ - sources: [ADMIN, GALLERY], integrations: [ADMIN], tagging: [ADMIN], maintenance: [ADMIN, GALLERY], @@ -37,6 +39,15 @@ export const SECTION_HANDOFFS = Object.freeze({ export const NOT_DELIVERED = 'This setting is not available here yet.'; let drafts = null; +// The Activity store of the System page shown last: a refused host save that does not name its job reads the list. +let jobs = null; + +/** The running job a 409 `busy` named, else the first running one from a fresh list read (the load never throws). */ +async function runningJob(error) { + if (error?.running) return error.running; + await jobs.load(); + return jobs.running()[0]; +} function el(tag, text, className) { const node = document.createElement(tag); @@ -71,7 +82,15 @@ export function systemDrafts(api) { // The store reads a missing value as none and a missing revision as unreadable (no host save). return { values: result.settings, revision: result.revision }; }, - writeHost: (changes, { expected_revision }) => api.post(SETTINGS_URL, { ...changes, expected_revision }), + async writeHost(changes, { expected_revision }) { + try { + return await api.post(SETTINGS_URL, { ...changes, expected_revision }); + } 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))); + throw error; + } + }, }); return drafts; } @@ -100,12 +119,13 @@ function markFailed(layout, store) { } /** - * The sections delivered so far: Overview (S2 Task 6), Appearance (S3 Task 8), and Maintenance's notice with "Run health checks" above the + * The sections delivered so far: Overview (S2 Task 6), Appearance (S3 Task 8), Sources & scans (S4 Task 11), and Maintenance's notice with "Run health checks" above the * hand-off to the maintenance tools that are not native yet (D123). */ export const DELIVERED_SECTIONS = Object.freeze({ overview: mountOverview, appearance: mountAppearance, + sources: mountSources, maintenance: (container, { api, activity, scope }) => [healthNotice({ api, activity, scope }), ...handoff('maintenance')], }); @@ -123,6 +143,7 @@ export const DELIVERED_SECTIONS = Object.freeze({ */ export function mountSystem(main, api, scope, { section, activity, sections = DELIVERED_SECTIONS, navigate = navigateTo }) { const store = systemDrafts(api); + jobs = activity; store.reloadHost(); // 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). `next` is null when the values cannot be read (no gallery-settings.js). diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index ace96b0..2de1f88 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -89,6 +89,8 @@ main:focus-visible { outline: 2px solid var(--ca-focus); outline-offset: 4px; } not save is highlighted, as its "Review the highlighted settings" status says. */ .save-row button[aria-disabled="true"] { opacity: .55; cursor: not-allowed; /* D92 */ } .setting-row [aria-invalid="true"] { outline: 2px solid var(--red); outline-offset: 2px; /* D92 */ } +/* D92: a source the last save was refused for (System › Sources) is highlighted the same way. */ +#source-list > .operation[aria-invalid="true"] { outline: 2px solid var(--red); outline-offset: 2px; /* D92 */ } /* D93: an Overview row whose action is not available yet (the review queue, Q6) keeps its button, disabled. */ .operation .row-actions button[aria-disabled="true"] { opacity: .55; cursor: not-allowed; /* D93 */ } /* D97: Appearance's legacy display rows. "Show image details on cards" off keeps a card's Info and Tag at the end of