From b4c7a15045096920d5f2355618e505db9050b632 Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 15:02:18 -0700 Subject: [PATCH] feat(review): Needs review page, live queue and concept comparison (#288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(review): add review queue schema with restore validation Add the Needs review schema group (media_review_metadata, media_review_items, media_review_decisions) beside the image annotation group it cites. review_schema_error validates the catalog, columns, foreign keys, row values and canonical 8 MiB evidence read-only; migrate_review_state installs it in a SAVEPOINT after the annotation group, and verify_database_file requires it under require_library_state. A review group without a valid annotation group is refused. Decisions null their pair and image navigation through indexes when those are deleted. Old backups and annotation-era backups restore and gain the group; browser fixtures clear the new tables on reset. * feat(review): persist ambiguous sampler evidence from scans and new outputs The output scan and the live monitor now queue every file the ingestion resolver finds ambiguous for Needs review. The file is captured, its metadata is read again from the capture and resolved with the default profile, and only an ambiguous result is stored, bound to the capture's annotation byte record and fenced by its storage epoch. Evidence holds one hashed projection per selectable candidate; none, more than 16 or over 8 MiB is a limited item. Recording is idempotent per bytes and evidence and never touches a decision. Scan statuses, counters, outcome rows and legacy SSE events are unchanged; the job scope gains review_items/review_errors and the log a Needs review line. The monitor records after its fallback link, best effort. * feat(review): explicit association decisions with compatible linking Needs review gains its queue reads and decisions: - GET /prompt_manager/review/items lists pending items oldest first with the exact pending total and an HMAC keyset cursor bound to the storage epoch and the review index revision. - GET /prompt_manager/review/items/{id} adds the candidates (role texts, label, selectability), the dependency fingerprint and the digest. - POST /prompt_manager/review/items/{id}/decision applies a candidate only for a capture of the item's bytes that the current file still hashes to and whose evidence is unchanged, through the ingester's compatible-only rules in the same transaction as the decision row; a conflict writes nothing. Leave unresolved records the decision and links nothing. ingest_resolved_pair keeps its contract; its body moves into _ingest_resolved_pair_in, which runs in the caller's transaction. Items keep the file's recorded spelling so targets keep their case on Windows, and a rescan skips the copy and hash of a file whose size and mtime match the bytes an item already cites. * fix(review): build the spelling validation test from native paths On Windows the locator is case-folded and uses backslashes, so the fixed POSIX pair /out/a.png no longer folded to itself and the new spelling check refused the good case. The test now spells a mixed-case native path and folds it with os.path.normcase, and also refuses a locator of another file. * fix(review): Apply sees the file under every spelling and rechecks only its choice Apply now finds the legacy rows of the same physical file however its root is reached. The roots are real paths while the monitor links under the directory it watches, so a symlinked or junctioned root, subst or a mapped drive used to hide the monitor's row and Apply added a second, differently linked row. Same-name rows are resolved to their physical identity (normcase(realpath), else samefile); the dependency fingerprint covers all of them, two rows refuse the choice, one row is enriched under the ingester's compatible-only rules, and a same-name row the request did not classify is a changed association. Apply re-verifies only what the decision depends on: the bytes are still ambiguous and still offer the chosen candidate. A resolver change that rewords the reason or reorders candidates leaves pending items applicable. The resolver version is one constant in prompt_pair_resolver, shared by ingestion provenance and Needs review. The transaction's byte check receives the re-hashed digest (or the body's for Leave unresolved), every Apply refusal branch is tested with zero writes, and the root listing and realpath calls run on a worker thread. * fix(review): validate stored review evidence once per startup Startup used to run the full review validation twice (the early guard and the install) and the annotation group's row checks inside each, so every evidence document was parsed and re-encoded twice. The early guard is now catalog-only (a review group without annotation tables), and the install validates the group once, trusting the annotation group it has just validated. The evidence predicate no longer pattern-scans every document in SQL, because the streamed canonical pass proves it, and the codec is built once. Measured on a database with 40,000 items of 4 KiB evidence: startup open went from 7.7 s to 2.5 s. A test pins one evidence pass per reopen. Restore and verify_database_file keep the full validation. * fix(review): keep one capture worker for interactive requests Needs review recording from the scan and the monitor shared the capture store's two workers with the Tag dialog and Apply, so a busy scan could answer a user's preview or decision with "Too many previews are open". Background captures now never take the last worker or capture slot, and wait up to two seconds for room before counting the file as not added. Interactive captures are admitted or refused at once, as before. * fix(review): refresh the recorded spelling when a file is seen under a new one A case-only rename on Windows or macOS keeps size and mtime_ns, so the rescan skipped the file and the item kept the old spelling. Its target was then formatted from a name the directory no longer lists, the capture refused it, and the item stayed pending with no way to apply it. The skip path and a duplicate insert now record the current spelling for every item at the locator and move the review index revision; item revisions do not change, so an open decision stays valid. Tested under case-folding semantics on every platform. * fix(review): final capture store close and recoverable stale cursors Closing the capture store at shutdown now runs under the store lock and is final: a monitor recording or scan batch still in flight gets a capture error instead of creating a new temp directory and restore subscription after the cleanup. A well-formed queue cursor this process cannot verify (opened before a ComfyUI restart, whose cursor secret is per process) now answers 409 review-conflict with the current total, like any other stale page, instead of an uncoded 400. Malformed cursors stay 400. * fix(review): make only the shutdown cleanup final The previous commit made every close of the capture store final, which also refused the explicit closes that swap in a fresh store. Only the host's on_cleanup hook is final now; an observe after it answers a coded capture error instead of an unhandled 500. * refactor(admin): move Needs review recording into its own mixin _record_scanned_review, _record_review_sync, record_review_file and _log_review_items move unchanged from py/api/admin.py to py/api/review_recording.py (ReviewRecordingMixin), keeping admin.py from growing further past the size ceiling. No behaviour change. * fix(review): record the review fix round and the skip's residual risks in TESTING.md The Needs review paragraphs now state the rescan skip's residual risks (an in-place rewrite that keeps size and mtime_ns is never re-queued; a deleted or moved file leaves its pending item counted), the physical identity rule for Apply, the candidate-only recheck and shared resolver version, the spelling refresh, background capture admission, the final shutdown close, the stale-cursor answer and the measured startup validation cost. * feat(review): bind queue cursors to the storage epoch only A decision or a new item (the monitor records one per generation) no longer makes an open cursor stale: the (created_at, id) keyset stays ordered when items leave or are appended. A restore still answers storage-changed and an unverifiable cursor review-conflict. * feat(review): Needs review page, SAVED VIEWS row and Activity hook The concept's Metadata review page at #/review: the notice with the exact pending count, the queue's media cards (shared with Images through image-card.mjs), and the Resolve the association panel with the sampler context, the chosen path's prompt blocks, Leave for later and Apply association. Apply takes a fresh capture; refusals keep the selection and explain inline; an unanswered decision is resent once. SAVED VIEWS lists Needs review after Favorites with its count (retires D50), and an ended scan that recorded review items offers Review metadata in Activity (D87). Queue items carry the card's media url. * feat(review): re-read an open Needs review page when its queue changes An open Needs review page now follows the queue without polling. A new server-sent event stream, GET /prompt_manager/review/events, sends "ready" when it opens and "change" after every committed queue change: an item the scan or the monitor recorded, a spelling refresh, a decision from any tab, and a restore. database/review_changes.py notifies after the commit from whichever thread wrote it. The page re-reads its first page on each event, keeps the open item, its choice and an unanswered decision, and re-reads the SAVED VIEWS count with it. When the open item left the queue, the next one opens with a note. A keepalive comment every 25 s lets the server drop a closed tab; shutdown ends every stream. Leaving the page mid-decision now releases that decision's capture. D-NEW-F1 is renumbered D119 in markers and TESTING.md. * test(review): Needs review page workflows The review fixture now seeds a real monitor-fallback conflict (the monitor pipeline links a new output to the most recent prompt, then queues it), a limited item and a file under no current root, and can add an output through the monitor or leave one for a metadata scan. New workflows: Apply links the chosen pair to that file only; a real association conflict leaves the links unchanged and explains; a limited item and an unavailable file can only be left unresolved; a tag save leaves the item pending; keyboard focus, labels and touch targets; an open page shows items the monitor and a scan add and a decision made in another tab; leaving the page releases captures, the change stream and requests. The faked 409 conflict test is replaced by the real one. * test(review): cover the card Tag button and raise the review floors The card's Tag on Needs review now has a workflow, and the page's queue change hook has no unused default. review.mjs and review-state.mjs floors rise to their measured coverage; no other floor changes. * feat(review): keep the loaded depth on live re-reads and read once per visit A queue change re-read only the first 40 items, so the pages Load more had shown vanished and the page jumped. Each re-read now reads page after page until it holds as many items as were shown (a restore met after the first page reads again from the start), so the cards and the scroll position stay. The stream's ready made every visit read the queue twice. List reads now answer the queue's version (library_epoch, review_revision) and ready names the version read after subscribing; the page re-reads on ready only when the queue it shows is older. The SAVED VIEWS count follows the page's total, so a change costs no count read. A stream counts as open only once ready is sent. * test(review): compare Needs review with the approved concept review-concept.spec.mjs measures the whole Metadata review page as a tree in the concept and in production, on the review fixture's new concept-shaped queue (the concept's first three image names and sizes and its first prompt). mapConceptReview applies only D29, D119, D3, production's hooks and accessible names and sample data, and the page then matches at 1600x1100 and 390x844 in both themes. The gallery layout follows the concept at every breakpoint, the SAVED VIEWS rows match with Needs review after Favorites, the queue's Tag image dialog matches the needs-review reference, and a dropped notice border or reordered actions fail. The comparison found one gap: an unavailable file's card lost its media area. Its empty media button now keeps the concept's 1.33 ratio (D119). TESTING.md records the comparison, the extended D119 and a summary of the image-tagging slice (routes, S1, capture limits, the record-changing job, the no-download rule, the restart note). A ready whose version cannot be read re-reads the queue (covered by a page test). * test(review): give the multi-route shell comparison a fitting time budget The dispatched CI run failed "shell geometry, tokens and controls match the concept 390x844 light" on WebKit with a 30 s test timeout (no assertion failed). Each of these tests makes twelve page loads with captures, hovers and focus checks: 15-19 s on a CI WebKit runner on main, 22-31 s on this run's loaded shard. With 2 CPUs and 3 workers it fails locally the same way (6 of 12 timed out); with a 90 s budget, as the other multi-state concept comparisons have, all 12 pass. * fix(review): coalesce queue change events and answer queue thumbnails - The change stream sends the first notice at once and collapses the notices of the next 0.4 s into one trailing change, so a busy scan costs an open page one re-read per 0.4 s at most (COALESCE_SECONDS). - Queue items answer thumbnail_url, the same URL the Images listing builds (one shared helper, media_thumbnail_url), or null without a thumbnail. - The "duplicate and refused decision send nothing" test spies on notify_review_change instead of reading the next event. * fix(review): keep the live queue right through applies, Load more and lost streams - A change heard while a decision is in flight is owed and read once the decision is refused or unanswered (I1). - Load more has its own read: during a re-read it waits and continues from the re-read queue; a re-read started during Load more takes the click over, and a failed re-read leaves Load more clickable (I2, M7). - A stream the browser gave up on (503, proxy error) reopens after 3 s, doubling to 30 s, with a quiet "Live updates are paused" line while it is down (I3, D-NEW-F2). - A hidden tab closes its stream and a visible one reopens it; the ready version keeps that to at most one read (I4). - Apply fences on the detail's epoch and a re-read under another epoch reads the open item again (M3); leaving during the observe releases its capture (M4); an unanswered decision that landed finishes as recorded (M5); cards follow a re-read's url, target and availability and show thumbnails (M6, M10). - The unreadable-ready test asserts exactly one re-read (M1); the panel's long-file-name wrap carries its D119 marker and TESTING.md entry (M8). * docs(review): number the live-updates-paused deviation D120 * fix(review): refuse monitor recording beside record-changing jobs System S1 (#289) gives every record change a registry admission class. The scan already records Needs review items under its own records job. The monitor's recording lasts one file and is not a job, so it now checks the records admission: while a records or guarded job or hold runs it records nothing, logs that the file waits for the next scan, and answers deferred. The fallback link stands and the next "new" scan reads the ambiguous file again, because a fallback link is not a resolved one. JobRegistry.blocker and active iterate a snapshot of the jobs, since the monitor's worker thread now asks them too. * test(backup): wait for the restore, not a 5 s timer, in the thread test test_database_is_usable_from_every_thread_after_restore let its worker wait 5 s for the restore. On a stalled Windows runner (py3.11, where the previous restore test also took 25 s instead of 0.3 s) the restore took longer, the wait timed out, and the worker read the old database before the restore ran. The worker now waits for the event under a 120 s bound that only a hung test reaches, reports a timeout as an error, and the main thread releases it even when the restore raises. With a restore delayed by 6 s the old test fails and the new one passes. * fix(activity): drop Cancel job from a job that had ended before Activity opened Since #289 the Activity list removed an item's actions only when they were isConnected. On the first render the item is not in the page yet, so a job that had already ended when Activity opened kept its "Cancel job" (and a running job's progress was moved after its actions, then back). The list now asks parentNode instead. The review spec "a scan job with review items offers Review metadata in Activity" failed on this before the change (Expected 0 buttons on a finished job, Received 1) and passes after it. * fix(review): store non-finite evidence as null and count pending items canonical_evidence now follows main's non-finite policy (#303): NaN and ±Infinity are written as null instead of raising, so a NaN-stamped graph never turns a review item limited. The System Overview reports the review queue's pending total in needs_review.pending, read in the same snapshot as the Library counts. * fix(review): show the pending count on the System Overview review row The Overview's review row now reads "N ambiguous image links" with the concept's line, from needs_review.pending. Its Review metadata action stays aria-disabled until the Needs review page ships. The system specs and their concept comparison follow the real count. * feat(review): open Needs review from the System Overview review row Review metadata on the Overview's review row now opens #/review, the concept's go-review, instead of staying aria-disabled. * fix(system): answer the Overview and Statistics only after every read ended asyncio.gather answered at the first failed read while its sibling executor reads were still inside SQLite. In the suite the test teardown then deleted the database under the running read, and py3.10 on Ubuntu segfaulted in get_statistics (test_statistics_failure_is_an_error_envelope). Both routes now wait for all their reads before answering. * fix(review): queue only ambiguous files that nothing links, and heal the queue The maintainer's live scan (19,908 files, scope "new") reported 4,677 files under Needs review, but the queue held 10,232 items: every file the strict resolver found ambiguous was queued, including the ones the legacy browser-parser fallback then linked (all sampled "added" rows) and the ones already linked to a known prompt (135 of 200 sampled "known" rows). A file is now queued only when its final outcome is ambiguous and unlinked: the resolver finds it ambiguous, the fallback links nothing and no saved row links it already. An ambiguous file that a saved row links is reported as unchanged ("Already linked to its prompt"), not as Needs review. The queue heals itself. After each batch, the scan removes the pending items of every listed file (read, or skipped as known) that a saved row now links. The monitor does the same for each ambiguous output it links, and never queues one. Only an undecided item is removed (pending, revision 0, no decision row), so a decision is never deleted or changed and the schema needs no new state. The scan job's scope.review_items now counts this scan's files that are in the pending queue after it, new or already there, so it agrees with the review group's ambiguous rows and with the queue total. review_added and review_settled are new, and the log gains "already in Needs review" and "left Needs review · now linked" lines. A duplicate whose item is decided is reported as "decided" and is not counted. * fix(review): announce items that leave the queue, and seed only queueable files #284 now queues a file only when its final outcome is ambiguous and unlinked, and it removes the pending items of files that are now linked. That removal is a queue change, so it calls notify_review_change after its commit, like recording and decisions: an open Needs review page re-reads its queue, and its count follows. The review browser fixture wrote graphs that the scan's fallback links, and it produced the conflict item through the monitor's fallback link. Neither is queued any more. Its files now carry ComfyUI's Infinity stamp, which the legacy parser cannot read, so they stay unlinked with the same two candidates. The conflict item is queued first and its file is linked to another prompt afterwards. "monitor" additions go through record_review_file from a worker thread. * fix(review): never queue an ambiguous file with nothing to choose The maintainer's live queue held 39 items whose candidates all read "unresolved" ("No positive / No negative"). An ambiguous result is now reviewable only when at least two candidates are resolved with non-blank positive text. With none the file could not be traced; with exactly one, the untraced branches may still have made the file, so that text is not certain either. Both are reported unresolved, never linked and never queued (also the A1111 two-delimiter case, which has no candidates). A rescan removes the pending items of such files whose own evidence offers no choice, with the same safe delete as the linked settle (pending, revision 0, no decision row). A limited item (over 16 candidates) that holds at least two candidates is kept. The job scope gains review_untraced and the log "= N images left Needs review · nothing to choose". * test(review): seed the limited item from a graph with more candidates than stored An A1111 block with two negative lines offers no candidate to choose, so Needs review no longer queues it (#284). The fixture now records a graph of 17 samplers, which the recorder stores limited. --- TESTING.md | 209 ++++- __init__.py | 3 + database/review.py | 24 +- database/review_changes.py | 64 ++ database/review_decisions.py | 6 +- py/api/__init__.py | 4 + py/api/images.py | 29 +- py/api/review.py | 45 +- py/api/review_events.py | 171 ++++ tests/browser/concept-inventory.mjs | 2 + tests/browser/review-concept.spec.mjs | 212 +++++ tests/browser/server.py | 4 + tests/browser/shell-concept.spec.mjs | 11 +- tests/browser/system-concept.spec.mjs | 8 +- .../browser/tagging-canonical-comparison.mjs | 149 ++++ tests/browser/workspace-review.spec.mjs | 556 ++++++++++++ tests/browser/workspace-saved-views.spec.mjs | 2 +- tests/browser/workspace-system.spec.mjs | 10 +- tests/browser/workspace.spec.mjs | 12 +- tests/browser/workspace_activity_fixture.py | 6 + tests/browser/workspace_review_fixture.py | 368 ++++++++ tests/js/workspace-review-state.test.js | 788 ++++++++++++++++++ tests/js/workspace-state.test.js | 43 +- tests/test_api_review.py | 83 +- tests/test_api_review_events.py | 364 ++++++++ tools/vendor/frontend-coverage-baseline.json | 20 +- web/js/workspace/activity-job-view.mjs | 8 +- web/js/workspace/activity.mjs | 18 + web/js/workspace/image-card.mjs | 81 ++ web/js/workspace/images.mjs | 62 +- web/js/workspace/index.mjs | 48 +- web/js/workspace/review-state.mjs | 449 ++++++++++ web/js/workspace/review.mjs | 347 ++++++++ web/js/workspace/state.mjs | 12 +- web/js/workspace/system-overview.mjs | 4 +- web/js/workspace/workspace.css | 11 + 36 files changed, 4083 insertions(+), 150 deletions(-) create mode 100644 database/review_changes.py create mode 100644 py/api/review_events.py create mode 100644 tests/browser/review-concept.spec.mjs create mode 100644 tests/browser/workspace-review.spec.mjs create mode 100644 tests/browser/workspace_review_fixture.py create mode 100644 tests/js/workspace-review-state.test.js create mode 100644 tests/test_api_review_events.py create mode 100644 web/js/workspace/image-card.mjs create mode 100644 web/js/workspace/review-state.mjs create mode 100644 web/js/workspace/review.mjs diff --git a/TESTING.md b/TESTING.md index 9117fe41..93b42d3c 100644 --- a/TESTING.md +++ b/TESTING.md @@ -627,8 +627,9 @@ Recorded shell deviations: `a saved view row matches the concept's` saves one view in both documents and compares the rows; `tests/browser/workspace-saved-views.spec.mjs` covers saving, reopening, the defaults that replace rejected values, and failed reads and saves. -- **D50**: no "Needs review" row in SAVED VIEWS until the tag-review slice (maintainer - ruling, 2026-10-21). The comparison maps only that row out. +- **D50** (retired by #249 PR F, ruling R5): SAVED VIEWS now lists the concept's + "Needs review" row after Favorites, with its count; the comparison maps nothing out + for it (only the concept's sample count "3" reads as the fixture's "0"). - **D51** (maintainer ruling, 2026-10-21, beyond the concept): SAVED VIEWS starts with the concept's Favorites row (star icon, name, count). Its count is the Favorites listing's own `limit=1` total (`/images/output?mode=workspace&favorites=1`), follows @@ -1880,6 +1881,187 @@ npm run --prefix tools/vendor test:workflows -- inspect-concept.spec.mjs workspa npm run --prefix tools/vendor test:comfyui -- inspect-sender.spec.mjs ``` +### Needs review page and recorded deviations (#249 PR F) + +`#/review` is the concept's Metadata review page (app.js `renderReview` a749–760) over +the review routes (`web/js/workspace/review.mjs`, `review-state.mjs`): breadcrumb and +document title "Review" (the concept's), heading "Metadata review" with its line and +**Open activity** (`#/activity`), the `.notice` "N images need a decision" with the +concept's sentence, then `.gallery-layout`: the queue's concept media cards +(`image-card.mjs`, shared with Images; the image opens the shared viewer, Info selects +the item, Tag opens the shared Tag image dialog with the review scope note) and +`section.surface` "Resolve the association" (filename, Tag image, Sampler context, +the chosen path's prompt blocks, Leave for later and primary Apply association). The +first item is selected on load, as the concept's first image is. Sampler context lists +"Sampler · / " per candidate (the server's label), disables +unusable ones, preselects the first workable one and ends with "Leave unresolved" +(rulings R2/R3); "Leave unresolved" shows the ambiguous status in both blocks. Apply +takes a fresh capture (`observe`) and posts the decision; Leave unresolved on a file +under no current root posts without a capture. Both actions return to Images (R7); a +recorded decision re-reads the row's count, as does every metadata scan that ends. A +refused decision keeps the selection and shows the server's sentence (the +association conflict's explanation included) as `p.warning-note` in the panel; a +restored storage or a changed queue reads the queue again; a request with no answer +keeps its capture and body, and the next Apply of the same choice resends it once. +The SAVED VIEWS row "Needs review" (review icon) follows Favorites with the exact +pending count (`GET /prompt_manager/review/items?limit=1`, its `total`). + +- **D86** (plan D63, ruling R6): an empty queue shows only the notice, "No images need a + decision" and "Ambiguous images are added when a metadata scan or a new generation + finds more than one sampler path." +- **D87** (plan D64): an ended metadata scan whose `scope.review_items` is above 0 + carries the concept's **Review metadata** (app.js a738 `go-review`): in its Activity + item after the item's line, and in its job view's heading after Download report. The + item's chip still names its state (D54), where the concept's says "Needs attention". +- **D119**: the queue cards have no selection checkbox and + no star (the page has no selection bar and the queue carries no favorite state); a + file under no current root has an empty, `aria-disabled` image button and Tag; more + than 40 items add the Images page's `.infinite-continuation` ("40 of 45 images", + Load more images) after the layout; loading and a failed read are the page's status + line (with Retry); a limited or unavailable file and the decision states are text in + the panel's `p.warning-note`; the toasts "Association accepted." and "Left + unresolved." replace the concept's prototype toast. The concept's "Sample review + queue" chip and "sample" wording are prototype copy (D29). When a re-read (below) + finds the open item gone, the next one opens with " was decided elsewhere, so + the next image is open."; a failed re-read keeps the queue and says "The review queue + could not be read again. Reload the page to see the latest images." A queue card's + title is its file name and its line the extension only (an undecided item names no + prompt, and D11 omits the size and seed a queue item does not carry); its image's alt + is the file name; the panel's Copy and Send buttons carry no prompt title in their + names; the Tag image dialog opened from the queue heads with the file name. An + unavailable file's empty media button keeps the concept's 1.33 media area, so its + card lines up with the others. A long file name in the panel's `p.small` wraps inside + the panel (`overflow-wrap: anywhere`), where the concept's short sample name never + overflows. A queue card shows the Images listing's thumbnail when the server knows one + (`thumbnail_url`, the same URL the Images page uses), else the original; a re-read + rebuilds a card whose media, target or availability changed (a root came or went), so + Open and Tag follow it. When an unanswered decision's item leaves the queue in a + re-read, the decision was recorded: the page finishes as a recorded decision does (its + toast, back to Images), not with "decided elsewhere". +- **D120** (the concept has no live-update state): while the + change stream is down, the heading carries the quiet status line "Live updates are + paused: changes made elsewhere appear once the connection returns." (`p.small + .review-live`, `role="status"`); the stream's next `ready` hides it. + +**Live queue (F2).** An open Needs review page holds one server-sent event stream, +`GET /prompt_manager/review/events` (`py/api/review_events.py`): `event: ready` when it +opens, then `event: change` after each committed queue change (an item the scan or the +monitor recorded, a spelling refresh, a decision from any tab; `database/review_changes.py` +notifies after the commit, from whichever thread wrote it) and after a restore. `change` +carries no data. `ready` carries the queue's version `{library_epoch, review_revision}`, +read after the stream subscribed, and every list read answers the same pair +(`review_revision` is the review index revision, which every queue change moves; a +restore moves the epoch). **One read per visit (F3):** the page re-reads on `ready` only +when the queue it shows is older than that version (an unknown version, `{}` when it +cannot be read, re-reads); a `ready` heard while the first read runs is compared once that +read answers. The test server counts a stream as open only once it has sent `ready`, so a +browser test that changes the queue after that count is a `change`, never part of the +version. The page re-reads on every `change`, keeping the open item, its choice and an +unanswered decision; re-reads asked for while one runs collapse into one more. A change +heard while a decision is in flight is owed: a refused or unanswered decision reads it +once the panel is idle again (a recorded one leaves the page). Load more asked for during +a re-read waits for it and continues after the queue it read; a re-read started during +Load more takes that click over and reads one page deeper, and a failed re-read leaves +Load more clickable. Apply fences on the epoch of the detail it decides: an observe under +another storage releases its capture and reads the queue and the item again, and any +re-read under another epoch reads the open item again. **Every re-read keeps the loaded depth (F3):** it +reads page after page (40 each, the route's limit) until it holds as many items as Load +more had shown, so the cards stay and the scroll position does not move; a restore met +after the first page reads once more from the first page. The row's count follows the +total the page shows (no count read per change). Nothing polls: a `: keepalive` comment +every 25 s only lets the server drop a closed tab's subscription. The server sends the +first change at once and collapses the notices of the next 0.4 s into one trailing +`change` (`COALESCE_SECONDS`), so a busy scan costs an open page one re-read per 0.4 s at +most. Shutdown (`on_shutdown`) ends every stream and answers a later one 503. A +reconnect the browser gives up on (a 503, or a proxy's error while ComfyUI restarts) is +opened again by the page after 3 s, doubling to 30 s, with D120's line shown while +the stream is down; its `ready` re-syncs the queue. The stream lives only while the tab is +visible: a hidden tab closes it and a visible one opens it again, and that `ready` reads +only when the queue changed meanwhile. A duplicate recording and a refused decision change +nothing and send nothing. Residual: each visible Needs review page holds one HTTP/1.1 +connection (browsers allow six per origin, shared with ComfyUI's editor), so several +side-by-side visible windows on the page, during a metadata scan's job stream, can still +queue the origin's other requests; a re-read of a deep queue costs one request per 40 +items shown. + +Pinned by `tests/test_api_review_events.py`, `tests/js/workspace-review-state.test.js` +("refresh …", "a refresh keeps the loaded depth …", "ready naming …") and +`tests/browser/workspace-review.spec.mjs` ("an open page shows …", "a visit reads its +queue once …", "a live re-read keeps the loaded depth and the scroll position", "a stream +the browser gives up on …", "a hidden tab closes its stream …", "a re-read refreshes a +card …"); the controller's owed, Load more and epoch cases are "I1: …", "I2: …", "M3: +…", "M5: …" and "M7: …" in `workspace-review-state.test.js`, and the coalescing is "a +burst of changes …" in `test_api_review_events.py`. +Leaving the page mid-decision releases that decision's capture (the server may never see +the request), and leaving during the observe releases the capture it then answers +("leaving during the observe …"). +**Concept comparison (F3).** `tests/browser/review-concept.spec.mjs` measures the whole +page (`reviewInventory` in `tests/browser/tagging-canonical-comparison.mjs`: every shown +box below `main` with its place, fonts, edges and layout, a leaf's text, a button's name, +a select's options) in the concept at `#review` and in production on the fixture's +concept-shaped queue (`POST /__test__/review/reset {items: 3, concept: true}`: the +concept's first three image names and sizes, and the concept's first prompt on the open +item), and `mapConceptReview` applies only: D29 (no "Sample review queue" chip, no +"sample" in the panel heading and Apply), D119 (above), D3 (the grid's +`position: relative`), production's hooks and accessible names (`review-queue`, +`review-panel`, the list roles, the region names) and sample data (the candidate options +except Leave unresolved; widths and heights that follow from the changed copy, moved by +exactly that much). It then matches at 1600×1100 and 390×844 in both themes on all four +projects; the gallery layout follows the concept at 1600, 1200, 900 and 390px (310px, +275px with a 15px gap, 280px, one column); the SAVED VIEWS rows match with Needs review +after Favorites; the queue's Tag image dialog matches the concept's needs-review dialog +(`needs-review-tagging.png`) under the C4 mapping; and the comparison is shown to notice +a dropped notice border and reordered actions. The paired captures (populated queue in +both themes and widths, conflict, limited item, unavailable file, empty queue) are test +artifacts; no screenshot baseline is added. The comparison found one gap, the collapsed +media area of an unavailable file's card, fixed above (`workspace-review.spec.mjs` "a +limited item and an unavailable file …" pins it). + +- **D3** and **D20** apply to the queue grid (its cards stay in the grid's flow; it is + not virtualized) and to the panel's prompt blocks (their mount point is transparent). + +Tests: `tests/browser/review-concept.spec.mjs` (concept comparison), +`tests/js/workspace-review-state.test.js` (controller), +`tests/js/workspace-state.test.js` (route and count), +`tests/browser/workspace-review.spec.mjs` (queue, count, apply, leave, Tag, empty +queue, Load more, Activity's Review metadata; items recorded by the real recorder in +`tests/browser/workspace_review_fixture.py`), and the SAVED VIEWS rows in +`shell-concept.spec.mjs` and `workspace-saved-views.spec.mjs`. + +### Native image tagging and Needs review (#249 PRs A–F) + +One place for the whole image-tagging slice; the details live in the sections named. + +- **Routes.** Tagging: `POST /prompt_manager/image-annotations/observe`, `GET`/`DELETE + /prompt_manager/image-annotations/capture/`, `POST + /prompt_manager/image-annotations/suggest`, `PUT /prompt_manager/image-annotations/tags` + ("Tag image dialog and recorded deviations"). Needs review: `GET + /prompt_manager/review/items?limit=1..40&cursor=` (answers `library_epoch`, + `review_revision`, `total`), `GET /prompt_manager/review/items/`, `POST + /prompt_manager/review/items//decision` and the change stream `GET + /prompt_manager/review/events` ("Needs review decisions and queue reads", "Needs review + page and recorded deviations"). Every legacy route keeps its payload; the only legacy + change is `409 model-busy` from the legacy Auto-tag routes while another model + operation holds the shared gate. +- **S1 (fingerprint-current reads, ruled 2026-10-22).** The Images Tags filter, search and + the inspector/viewer chips use a byte record only while the file's `(st_size, + st_mtime_ns)` equals its verified fingerprint; writes hash the bytes. Residual risk + under "Image annotations schema". +- **Capture limits.** 256 MiB per file, 8 completed captures, 512 MiB reserved, 2 + capture/verify workers, 900 s idle, 1800 s hard; the browser fixtures release every + capture on reset, and leaving the Needs review page mid-decision releases its capture. +- **The record-changing job.** Bulk Add tags and Auto-tag selected run as the + `image-tags` Activity job (PR D); like the metadata scan it is a record-changing job, so + a start while another runs is refused (`busy`). Needs review items come from that scan + and from the live monitor, and a decision changes links only when compatible + (`association-conflict` otherwise, with zero writes). +- **No downloads.** No automated test downloads or loads a model: the browser server + injects `TestModel` (its `download_model` raises), native routes and jobs never + download, and every tagging spec asserts no `/autotag/download/` request. +- **Restart.** PRs A, B, D and E and PR F (the review change stream, the queue version) + change Python: the maintainer restarts ComfyUI once after merging. HTML, CSS and JS + changes need only a browser refresh. + ### Native System (#249 System) `#/system/
` is a native workspace route rendered by `web/js/workspace/system.mjs` @@ -2020,8 +2202,8 @@ Recorded deviations (plan D60–D66 and D81 renumbered D92–D98 and D113; D121 integrity" is "Not checked" until a check ran this session, then "N issues" (checks that found issues, even beside a check that could not run), "Passed" or "Incomplete". The review row is "N ambiguous image links" (the Needs review queue's pending count, - `needs_review.pending`) with the concept's line; its Review metadata action is - `aria-disabled` (CSS under D93) until the Needs review page ships (#288). A count + `needs_review.pending`) with the concept's line; its Review metadata action opens + `#/review` (the concept's `go-review`). A count that is not an integer reads "Ambiguous image links" / "Not available yet." (Q6). "Backup status" reads "No retained backup record." until verified backups exist (S8). The thumbnail row is "Checking thumbnails…" until its own read answers, then "No @@ -2492,13 +2674,18 @@ Needs review decisions and queue reads (#249, `py/api/review.py`, `database/review.py`, `database/review_decisions.py`, `tests/test_api_review.py`): `GET /prompt_manager/review/items?limit=1..40&cursor=` answers `{success, library_epoch, total, items, next_cursor}` with pending items -oldest first; `total` is the exact pending count. The cursor is an HMAC keyset -on `(created_at, id)` bound to the storage epoch and `review_index_revision`: a -malformed one is 400, a well-formed one this process cannot verify (tampered, -or opened before a ComfyUI restart: the secret is per process) is 409 -`review-conflict` with the current `total`, one from before a restore is 409 -`storage-changed`, one from before any queue change (a decision, a new item, a -refreshed spelling) is 409 `review-conflict`. +oldest first (each item adds the card's `url`, the Images listing's +`/prompt_manager/images/serve/?root=&root_scope=` formatted without a +stat, null when the file is under no current root, and `is_video` from its extension); `total` is the exact pending count. The cursor is an HMAC keyset +on `(created_at, id)` bound to the storage epoch only (ruling M3, so "load more" +keeps working while the monitor adds an item per generation): a malformed one +is 400, a well-formed one this process cannot verify (tampered, or opened +before a ComfyUI restart: the secret is per process) is 409 `review-conflict` +with the current `total`, and one from before a restore is 409 +`storage-changed`. A decision or a new item leaves an open cursor valid: a +decided item drops out of later pages and new items (stamped `utc_now_iso()`, +microseconds) append after it. Residual risk: an item recorded after a wall +clock step backwards sorts before an open cursor and shows only on reload. Targets are formatted from `spelled_path` against the current roots (the roots' real paths are the only file access); a file under no current root is `unavailable` with a null target. Every read and decision lists the roots and diff --git a/__init__.py b/__init__.py index 6a67386e..6123a47d 100644 --- a/__init__.py +++ b/__init__.py @@ -91,6 +91,9 @@ try: if host_app is not None and hasattr(host_app, "on_cleanup"): host_app.on_cleanup.append(api.cleanup_history_reads) host_app.on_cleanup.append(api.cleanup_annotation_captures) + if host_app is not None and hasattr(host_app, "on_shutdown"): + # Open Needs review change streams end before the server waits on them. + host_app.on_shutdown.append(api.close_review_events) except Exception as e: # Log to internal logging system without console spam diff --git a/database/review.py b/database/review.py index 40780e83..51321d8f 100644 --- a/database/review.py +++ b/database/review.py @@ -31,6 +31,7 @@ from .annotation_schema import ( ) from .annotations import _new_id, _validate_locator, _validate_target from .models import normalize_image_path, utc_now_iso +from .review_changes import notify_review_change from .review_schema import ( DECISIONS_TABLE, ITEMS_TABLE, @@ -315,7 +316,9 @@ class ReviewOperations: refreshed = _refresh_spelling(conn, locator, spelled_path) if inserted or refreshed: conn.execute(_BUMP_REVIEW_INDEX) - return inserted + if inserted or refreshed: + notify_review_change(self.model.db_path) + return inserted def refresh_review_spelling(self, locator: str, spelled_path: str) -> bool: """Record ``spelled_path`` as the spelling of every item at ``locator``. @@ -332,7 +335,9 @@ class ReviewOperations: refreshed = _refresh_spelling(conn, locator, spelled_path) if refreshed: conn.execute(_BUMP_REVIEW_INDEX) - return refreshed + if refreshed: + notify_review_change(self.model.db_path) + return refreshed def review_index_revision(self) -> int: """The queue's revision (moved by every queue change).""" @@ -341,6 +346,19 @@ class ReviewOperations: raise sqlite3.DatabaseError("Missing review queue metadata") return row[0] + def review_queue_version(self) -> dict: + """``{library_epoch, review_revision}`` of the queue, from one snapshot. + + The pair a list read answers: equal pairs mean the same queue (a + restore moves the epoch, every queue change moves the revision). + """ + with self._annotation_read() as conn: + epoch = conn.execute(_EPOCH).fetchone() + revision = conn.execute(_REVIEW_INDEX).fetchone() + if epoch is None or revision is None: + raise sqlite3.DatabaseError("Missing review queue metadata") + return {"library_epoch": epoch[0], "review_revision": revision[0]} + def review_items_for(self, locator: str) -> list: """Every item of the file at ``locator``, oldest first, evidence parsed.""" _validate_locator(locator) @@ -423,6 +441,8 @@ class ReviewOperations: removed += conn.execute(_SETTLE_ITEM, (item_id,)).rowcount if removed: conn.execute(_BUMP_REVIEW_INDEX) + if removed: + notify_review_change(self.model.db_path) return removed def settle_untraced_review_items(self, locators) -> int: diff --git a/database/review_changes.py b/database/review_changes.py new file mode 100644 index 00000000..31c54b69 --- /dev/null +++ b/database/review_changes.py @@ -0,0 +1,64 @@ +"""Needs review queue change notices (#249 PR F2). + +Every committed change to the pending queue (an item recorded by the scan or +the monitor, a spelling refresh, a decision) calls ``notify_review_change`` +after its transaction, from whichever thread wrote it. The review event +stream (``py/api/review_events.py``) subscribes, so an open Needs review page +re-reads its queue when it changes instead of polling. Observers are keyed by +the database file, like ``subscribe_history_restore``; an observer that fails +is logged and never fails the write. +""" + +import os +import threading +from typing import Callable + +try: + from ..utils.logging_config import get_logger +except ImportError: + from utils.logging_config import get_logger + +_logger = get_logger("prompt_manager.database.review_changes") +_lock = threading.Lock() +_observers: dict[str, dict[object, Callable[[], None]]] = {} + + +def _identity(db_path: str) -> str: + return os.path.normcase(os.path.realpath(db_path)) + + +def subscribe_review_changes( + db_path: str, callback: Callable[[], None] +) -> Callable[[], None]: + """Call ``callback()`` after each committed queue change; return the unsubscribe.""" + identity = _identity(db_path) + token = object() + with _lock: + _observers.setdefault(identity, {})[token] = callback + + def unsubscribe() -> None: + with _lock: + observers = _observers.get(identity) + if observers is not None: + observers.pop(token, None) + if not observers: + _observers.pop(identity, None) + + return unsubscribe + + +def observer_count(db_path: str) -> int: + """How many observers watch ``db_path`` (tests and diagnostics).""" + with _lock: + return len(_observers.get(_identity(db_path), {})) + + +def notify_review_change(db_path: str) -> None: + """Tell every observer of ``db_path``; call it after the commit.""" + with _lock: + callbacks = tuple(_observers.get(_identity(db_path), {}).values()) + for callback in callbacks: + try: + callback() + except Exception: + _logger.warning("Needs review change observer failed", exc_info=True) diff --git a/database/review_decisions.py b/database/review_decisions.py index 3b5b1ffb..a4ad767c 100644 --- a/database/review_decisions.py +++ b/database/review_decisions.py @@ -29,6 +29,7 @@ from .review import ( dependent_images, same_name_rows, ) +from .review_changes import notify_review_change from .review_schema import DECISIONS_TABLE, ITEMS_TABLE, canonical_evidence DECISION_KIND = "user-selection" @@ -119,7 +120,7 @@ class ReviewDecisionOperations: raise ValueError("A selected decision needs a candidate and a file") try: with self._pair_transaction() as conn: - return self._decide_in( + decided = self._decide_in( conn, item_id=item_id, expected_epoch=expected_epoch, @@ -135,6 +136,9 @@ class ReviewDecisionOperations: ) except _Rollback as rollback: raise rollback.error from None + # After the commit: open Needs review pages re-read their queue. + notify_review_change(self.model.db_path) + return decided def _decide_in(self, conn: sqlite3.Connection, **command) -> dict: epoch = self._library_epoch(conn, command["expected_epoch"]) diff --git a/py/api/__init__.py b/py/api/__init__.py index a37208fd..8b12c439 100644 --- a/py/api/__init__.py +++ b/py/api/__init__.py @@ -10,6 +10,7 @@ Split into domain-specific mixins for maintainability: - ImageTagJobsMixin: bulk Add tags / Auto-tag as image-tags jobs - ReviewRoutesMixin: the Needs review queue, item detail and decisions - ReviewRecordingMixin: Needs review recording for the scan and the monitor +- ReviewEventsMixin: the Needs review change stream an open page re-reads on - SystemRoutesMixin: System health check, Overview and usage statistics """ @@ -34,6 +35,7 @@ from .vocabulary import VocabularyRoutesMixin from .annotations import AnnotationRoutesMixin from .review import ReviewRoutesMixin from .review_recording import ReviewRecordingMixin +from .review_events import ReviewEventsMixin from .saved_views import SavedViewRoutesMixin from .history import HistoryRoutesMixin from .images import ImageRoutesMixin @@ -270,6 +272,7 @@ class PromptManagerAPI( AnnotationRoutesMixin, ReviewRoutesMixin, ReviewRecordingMixin, + ReviewEventsMixin, SavedViewRoutesMixin, HistoryRoutesMixin, PromptRoutesMixin, @@ -493,6 +496,7 @@ class PromptManagerAPI( self._register_vocabulary_routes(routes) self._register_annotation_routes(routes) self._register_review_routes(routes) + self._register_review_event_routes(routes) self._register_saved_view_routes(routes) self._register_history_routes(routes) self._register_prompt_routes(routes) diff --git a/py/api/images.py b/py/api/images.py index 3bf0a341..c37a6bbd 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -671,6 +671,25 @@ def _iter_media_files(output_path, max_files): yield root_path / name +def media_thumbnail_url(output_path, rel_path, root_index, *, root_scope=None): + """The Images listing's thumbnail URL for *rel_path* under *output_path*, + or None when no thumbnail exists (one blocking stat). The Needs review + queue cards use the same URL (#249 F, M10).""" + rel_path = Path(rel_path) + extension = rel_path.suffix.lower() + thumb_rel = ( + f"thumbnails/{rel_path.with_suffix('').as_posix()}_thumb" + f"{'.jpg' if extension in VIDEO_EXTENSIONS else extension}" + ) + thumbnail_url = thumbnail_url_for(thumb_rel, Path(output_path) / thumb_rel) + if thumbnail_url is None: + return None + thumbnail_url += f"&root={root_index}" + if root_scope is not None: + thumbnail_url += f"&root_scope={root_scope}" + return thumbnail_url + + def _output_image_entry( media_path, output_path, root_index, public_root=None, *, root_scope=None ): @@ -686,19 +705,13 @@ def _output_image_entry( extension = media_path.suffix.lower() is_video = extension in VIDEO_EXTENSIONS - thumb_rel = ( - f"thumbnails/{rel_path.with_suffix('').as_posix()}_thumb" - f"{'.jpg' if is_video else extension}" + thumbnail_url = media_thumbnail_url( + output_path, rel_path, root_index, root_scope=root_scope ) - thumbnail_url = thumbnail_url_for(thumb_rel, output_path / thumb_rel) - if thumbnail_url is not None: - thumbnail_url += f"&root={root_index}" encoded_path = urllib.parse.quote(rel_path.as_posix(), safe="/") url = f"/prompt_manager/images/serve/{encoded_path}?root={root_index}" if root_scope is not None: url += f"&root_scope={root_scope}" - if thumbnail_url is not None: - thumbnail_url += f"&root_scope={root_scope}" entry = { "id": hashlib.sha1( diff --git a/py/api/review.py b/py/api/review.py index 2d3e425d..6fb5f4fb 100644 --- a/py/api/review.py +++ b/py/api/review.py @@ -1,8 +1,9 @@ """Needs review routes (#249 E3): the pending queue, item detail, decisions. ``GET /prompt_manager/review/items`` lists pending items oldest first behind -an HMAC keyset cursor bound to the storage epoch and the review index -revision (any decision, new item or restore makes an old cursor stale); +an HMAC keyset cursor bound to the storage epoch only (a restore makes an +old cursor stale; a decision or a new item does not, because the +``(created_at, id)`` keyset stays ordered when items leave or are appended); ``total`` is the exact pending count. ``GET .../items/{id}`` adds the candidates and the dependency fingerprint. ``POST .../items/{id}/decision`` records one explicit decision: Apply links the chosen sampler's pair only @@ -23,10 +24,13 @@ import hmac import json import os import re +import urllib.parse from aiohttp import web from .annotations import _HEX64, _TOKEN, _UUID, _capture_error, _error +from .images import media_thumbnail_url +from .inspection_service import VIDEO_EXTENSIONS from .prompts import bad_request try: @@ -153,9 +157,9 @@ class _StaleCursor(ValueError): """A well-formed cursor this process cannot verify (e.g. after a restart).""" -def _encode_cursor(epoch: str, revision: int, key: tuple) -> str: +def _encode_cursor(epoch: str, key: tuple) -> str: payload = json.dumps( - {"v": 1, "e": epoch, "r": revision, "k": list(key)}, + {"v": 2, "e": epoch, "k": list(key)}, separators=(",", ":"), ensure_ascii=True, ).encode() @@ -164,7 +168,7 @@ def _encode_cursor(epoch: str, revision: int, key: tuple) -> str: def _decode_cursor(token: str) -> tuple: - """``(epoch, revision, (created_at, id))`` of a cursor. + """``(epoch, (created_at, id))`` of a cursor. ``ValueError`` when malformed; :class:`_StaleCursor` when well-formed but signed by another secret (the per-process secret changes on every start, @@ -185,15 +189,14 @@ def _decode_cursor(token: str) -> tuple: data = json.loads(payload) key = data["k"] if ( - data["v"] != 1 + data["v"] != 2 or not isinstance(data["e"], str) - or type(data["r"]) is not int or not isinstance(key, list) or len(key) != 2 or not all(isinstance(part, str) for part in key) ): raise ValueError() - return data["e"], data["r"], tuple(key) + return data["e"], tuple(key) except _StaleCursor: raise except (ValueError, KeyError, TypeError, UnicodeError, binascii.Error): @@ -253,7 +256,7 @@ class ReviewRoutesMixin: def _review_view(self, row: dict, roots, real_roots) -> dict: """The public ReviewItem of one stored row (no path, no locator).""" found = format_review_target(row["spelled_path"], row["locator"], real_roots) - target = None + target = url = thumbnail_url = None if found is not None: index, relative = found target = { @@ -261,16 +264,30 @@ class ReviewRoutesMixin: "root_scope": self._gallery_root_scope(roots[index], index), "path": relative, } + # The Images listing's media URL for the same file (formatting only). + url = ( + "/prompt_manager/images/serve/" + f"{urllib.parse.quote(relative, safe='/')}?root={index}" + f"&root_scope={target['root_scope']}" + ) + # The Images listing's thumbnail, when one exists (one stat). + thumbnail_url = media_thumbnail_url( + roots[index], relative, index, root_scope=target["root_scope"] + ) + filename = os.path.basename(row["spelled_path"]) return { "id": row["id"], "state": row["state"], "revision": row["revision"], "created_at": row["created_at"], - "filename": os.path.basename(row["spelled_path"]), + "filename": filename, "target": target, "unavailable": target is None, "candidate_count": row["candidate_count"], "limited": row["limited"], + "url": url, + "thumbnail_url": thumbnail_url, + "is_video": os.path.splitext(filename)[1].lower() in VIDEO_EXTENSIONS, } @staticmethod @@ -299,26 +316,26 @@ class ReviewRoutesMixin: return bad_request(str(exc)) try: page, views = await self._run_in_executor( - self._review_page, limit, bound[2] if bound else None + self._review_page, limit, bound[1] if bound else None ) except Exception: self.logger.exception("Review queue read failed") return _failed() if bound is not None and bound[0] != page["epoch"]: return _coded("storage-changed", library_epoch=page["epoch"]) - if bound is not None and bound[1] != page["revision"]: - return _coded("review-conflict", total=page["total"]) items = page["items"] next_cursor = None if page["more"] and items: last = items[-1] next_cursor = _encode_cursor( - page["epoch"], page["revision"], (last["created_at"], last["id"]) + page["epoch"], (last["created_at"], last["id"]) ) return web.json_response( { "success": True, "library_epoch": page["epoch"], + # F3: the queue version the change stream's ``ready`` names. + "review_revision": page["revision"], "total": page["total"], "items": views, "next_cursor": next_cursor, diff --git a/py/api/review_events.py b/py/api/review_events.py new file mode 100644 index 00000000..b59b6112 --- /dev/null +++ b/py/api/review_events.py @@ -0,0 +1,171 @@ +"""Needs review change stream (#249 PR F2). + +``GET /prompt_manager/review/events`` is a server-sent event stream for an +open Needs review page: ``event: ready`` when it opens, then +``event: change`` after each committed queue change (an item the scan or the +monitor recorded, a decision from any tab, a spelling refresh) and after a +restore. The first notice is sent at once; notices in the next +``COALESCE_SECONDS`` collapse into one trailing ``change`` (M9: a busy scan +records file after file, and each event costs every open page a re-read). +The page re-reads its queue on each ``change``; nothing polls. ``change`` +carries no data (``{}``). ``ready`` carries the queue's version +``{library_epoch, review_revision}``, read after the stream subscribed, the +same pair a list read answers: a page whose first read already answered that +version skips the re-read (F3, one read per visit), and any later change is a +``change`` event. Nothing else about the queue is pushed. + +A ``: keepalive`` comment every ``HEARTBEAT_SECONDS`` lets the server notice +a closed tab (its write fails) and drop the subscription; clients ignore +comments. Shutdown (``close_review_events``, an ``on_shutdown`` hook) ends +every stream, and a stream asked for afterwards is a 503. +""" + +import asyncio +import json + +from aiohttp import web + +try: + from ...database.execution_history import subscribe_history_restore + from ...database.review_changes import subscribe_review_changes +except ImportError: + from database.execution_history import subscribe_history_restore + from database.review_changes import subscribe_review_changes + +EVENTS_URL = "/prompt_manager/review/events" +HEARTBEAT_SECONDS = 25 +# After a ``change``, the next one waits this long, collapsing the notices in between. +COALESCE_SECONDS = 0.4 +_CHANGE = b"event: change\ndata: {}\n\n" +_KEEPALIVE = b": keepalive\n\n" + + +def _ready(version: dict) -> bytes: + data = json.dumps(version, separators=(",", ":"), sort_keys=True) + return f"event: ready\ndata: {data}\n\n".encode("utf-8") + + +class _Stream: + """One open stream: a change flag set from any thread, and a close flag.""" + + def __init__(self, loop: asyncio.AbstractEventLoop) -> None: + self._loop = loop + self.changed = asyncio.Event() + self.closed = asyncio.Event() + self.ready = False + + def wake(self) -> None: + """Thread-safe: mark a change (the loop may already be gone).""" + try: + self._loop.call_soon_threadsafe(self.changed.set) + except RuntimeError: # the loop closed + pass + + async def next(self, timeout: float) -> str: + """``change``, ``closed`` or ``idle`` (nothing within ``timeout``).""" + waits = { + asyncio.ensure_future(self.changed.wait()): "change", + asyncio.ensure_future(self.closed.wait()): "closed", + } + try: + done, _ = await asyncio.wait( + waits, timeout=timeout, return_when=asyncio.FIRST_COMPLETED + ) + finally: + for waiter in waits: + waiter.cancel() + if self.closed.is_set(): + return "closed" + if done: + self.changed.clear() + return "change" + return "idle" + + async def closed_within(self, timeout: float) -> bool: + """Wait up to ``timeout``; true when the stream closed meanwhile.""" + try: + await asyncio.wait_for(self.closed.wait(), timeout) + except asyncio.TimeoutError: + return False + return True + + +class ReviewEventsMixin: + """The Needs review change stream.""" + + def _register_review_event_routes(self, routes: web.RouteTableDef) -> None: + routes.get(EVENTS_URL)(self.review_events) + + def _review_streams(self) -> set: + streams = getattr(self, "_open_review_streams", None) + if streams is None: + streams = self._open_review_streams = set() + return streams + + def review_stream_count(self) -> int: + """How many change streams have sent ``ready`` and are open (tests and + diagnostics): a queue change after this count moved is a ``change`` + event on them, never part of the version ``ready`` named.""" + return sum(1 for stream in self._review_streams() if stream.ready) + + async def close_review_events(self, app) -> None: + """End every open stream; later requests get 503 (shutdown hook).""" + self._review_events_closed = True + for stream in list(self._review_streams()): + stream.closed.set() + + async def _queue_version(self) -> dict: + """The queue version for ``ready``, read after subscribing (so a change + after this read is an event); ``{}`` when it cannot be read, and the + page then re-reads.""" + try: + return await self._run_in_executor(self.db.review_queue_version) + except Exception: + self.logger.warning("Needs review version read failed", exc_info=True) + return {} + + async def review_events(self, request: web.Request) -> web.StreamResponse: + if getattr(self, "_review_events_closed", False): + return web.json_response( + {"success": False, "error": "The server is shutting down."}, + status=503, + ) + stream = _Stream(asyncio.get_running_loop()) + db_path = self.db.model.db_path + unsubscribers = ( + subscribe_review_changes(db_path, stream.wake), + subscribe_history_restore(db_path, stream.wake), + ) + streams = self._review_streams() + streams.add(stream) + response = web.StreamResponse( + status=200, + headers={ + "Content-Type": "text/event-stream", + "Cache-Control": "no-store", + "X-Accel-Buffering": "no", + }, + ) + try: + version = await self._queue_version() + await response.prepare(request) + await response.write(_ready(version)) + stream.ready = True + while True: + outcome = await stream.next(HEARTBEAT_SECONDS) + if outcome == "closed": + break + if outcome == "idle": + await response.write(_KEEPALIVE) + continue + await response.write(_CHANGE) + if await stream.closed_within(COALESCE_SECONDS): + break + await response.write_eof() + except (ConnectionError, RuntimeError) as exc: # the tab went away + self.logger.debug(f"Needs review stream closed ({exc})") + finally: + streams.discard(stream) + for unsubscribe in unsubscribers: + unsubscribe() + return response diff --git a/tests/browser/concept-inventory.mjs b/tests/browser/concept-inventory.mjs index f7cba705..4afd5858 100644 --- a/tests/browser/concept-inventory.mjs +++ b/tests/browser/concept-inventory.mjs @@ -127,6 +127,8 @@ export async function openProduction(page,route,theme,motion='no-preference') { await expect(page.locator('#primary-nav .nav-count')).toHaveCount(3); // SAVED VIEWS' Favorites count (D51), the concept's starred images in the concept fixture. await expect(page.locator('#saved-nav a[data-route="favorites"] .nav-count')).toHaveCount(1); + // SAVED VIEWS' Needs review count (ruling R5): the exact pending count. + await expect(page.locator('#saved-nav a[data-route="review"] .nav-count')).toHaveCount(1); } /** Computed outline of the focused element, after keyboard focus so `:focus-visible` applies. */ diff --git a/tests/browser/review-concept.spec.mjs b/tests/browser/review-concept.spec.mjs new file mode 100644 index 00000000..19365e97 --- /dev/null +++ b/tests/browser/review-concept.spec.mjs @@ -0,0 +1,212 @@ +// Needs review against the approved concept (#249 PR F, plan Task F3, Mock inventory M4: app.js renderReview +// a749–760, style.css s407–415, s796–814 and their breakpoints, screenshot needs-review-tagging.png). Production runs +// on the review fixture's concept-shaped queue (workspace_review_fixture.py `concept: true`: the concept's first three +// image names and sizes, and the concept's first prompt on the open item), so every remaining difference is a recorded +// deviation applied by mapConceptReview (D3, D29, D119) or a test failure. Paired captures of every state are attached +// as artifacts; no screenshot baseline is added. +import { test, expect } from './fixtures.mjs'; +import { openConcept, openProduction, inventoryOf, mutate, saveImageTags, diffInventory, SIZES, THEMES } from './concept-inventory.mjs'; +import { reviewInventory, mapConceptReview, taggingInventory, mapConceptTagging, diffTaggingInventory } from './tagging-canonical-comparison.mjs'; + +const CONCEPT_FILE = 'alpine_00001.png'; +const CONCEPT_TAGS = ['cinematic', 'blue_hour', 'architecture']; +const SURFACE = ['h2', 'p.small', 'div.row-actions.spaced-15', 'div.field', 'section.positive.prompt-section', + 'section.negative.prompt-section', 'div.dialog-actions']; +// The fake model's server default instructions (tests/browser/server.py TestModel.default_prompt), D84. +const INSTRUCTIONS = 'Describe the image'; + +const panel = page => page.locator('#workspace-main section.review-panel'); +const tagDialog = page => page.getByRole('dialog', { name: 'Tag image', exact: true }); + +async function seed(request, data = {}) { + await expect(await request.post('/__test__/review/reset', { data: { items: 3, concept: true, ...data } })).toBeOK(); +} +/** Production's Needs review page with its first item's sampler paths loaded. */ +async function openReview(page, theme) { + await openProduction(page, 'review', theme); + await expect(panel(page).getByRole('combobox', { name: 'Sampler context', exact: true })).toBeEnabled(); + await settle(page); +} +async function settle(page) { + await page.mouse.move(0, 0); + await page.evaluate(async () => { + await document.fonts.ready; + await Promise.all([...document.images].map(image => image.decode().catch(() => {}))); + }); +} +const conceptInventory = page => page.evaluate(reviewInventory, '#main'); +const productionInventory = page => page.evaluate(reviewInventory, '#workspace-main'); +const node = (tree, prefix) => tree.tag?.startsWith(prefix) ? tree : tree.children?.map(child => node(child, prefix)).find(Boolean) ?? null; +/** Computed values of CSS tokens, read through a probe element in the open page. */ +const tokens = (page, declarations) => page.evaluate(text => { + const probe = document.createElement('i'); + probe.style.cssText = text; + document.body.append(probe); + const style = getComputedStyle(probe), values = [style.borderTopColor, style.backgroundColor]; + probe.remove(); + return values; +}, declarations); + +async function compareAt(page, request, width, height, theme) { + await seed(request); + await page.setViewportSize({ width, height }); + await openConcept(page, 'review', theme); + await settle(page); + const concept = await conceptInventory(page); + await openReview(page, theme); + const actual = await productionInventory(page); + return { concept, actual, reference: mapConceptReview(concept, actual) }; +} + +test.beforeEach(async ({ page }) => { + page.errors = []; + page.on('pageerror', error => page.errors.push(error.message)); +}); +test.afterEach(async ({ page, request }) => { + expect(page.errors).toEqual([]); + await request.post('/__test__/jobs/restart'); +}); + +for (const [width, height] of SIZES) for (const theme of THEMES) { + test(`concept and production Metadata review match at ${width}x${height} ${theme}`, async ({ page, request }) => { + const { actual, reference } = await compareAt(page, request, width, height, theme); + expect(diffInventory(reference, actual), `${width}x${height} ${theme}`).toEqual([]); + // Pinned concept values (M4), so a mapping mistake on both sides cannot pass unseen. + const main = actual.main; + expect(main.children.map(child => child.tag)).toEqual(['div.heading', 'div.notice', 'div.gallery-layout']); + const notice = node(main, 'div.notice'); + const [accent, soft] = await tokens(page, 'border-top:1px solid var(--ca-accent);background:var(--soft-blue)'); + expect([notice.borderTopColor, notice.borderLeftColor, notice.backgroundColor]).toEqual([accent, accent, soft]); + expect([notice.borderTopWidth, notice.borderTopLeftRadius, notice.paddingTop, notice.paddingLeft, notice.marginBottom]) + .toEqual(['1px', '6px', '15px', '18px', '22px']); + expect(notice.flexDirection).toBe(width <= 700 ? 'column' : 'row'); + expect(notice.children.map(child => child.children.map(item => [item.tag, item.text]))) + .toEqual([[['h2', '3 images need a decision'], ['p', 'Multiple sampler paths were found. The existing image links were preserved. Choose the correct context or leave them unresolved.']]]); + const layout = node(main, 'div.gallery-layout'); + expect(layout.gridTemplateColumns).toBe(width <= 700 ? `${layout.width}px` : `${layout.width - 22 - 310}px 310px`); + const surface = node(main, 'section.review-panel'); + expect(surface.children.map(child => child.tag)).toEqual(SURFACE); + expect(surface.children[0].text).toBe('Resolve the association'); + expect(surface.children[1].text).toBe(CONCEPT_FILE); + const actions = surface.children.at(-1); + expect(actions.children.map(button => [button.text, button.primary])).toEqual([['Leave for later', false], ['Apply association', true]]); + expect(node(surface, 'select').options.at(-1)).toEqual(['Leave unresolved', false]); + expect(node(main, 'div.image-grid').children.map(card => node(card, 'h3').text)) + .toEqual(['alpine_00001.png', 'dunes_00001.png', 'forest_00001.png']); + expect(actual.noPageScroll).toBe(true); + }); +} + +test('the gallery layout follows the concept at every breakpoint', async ({ page, request }) => { + await seed(request); + // 310px beside the queue, 275px and a 15px gap at ≤1250px, 280px at ≤950px, one column at ≤700px. + for (const [width, columns, gap] of [[1600, '310px', '22px'], [1200, '275px', '15px'], [900, '280px', '15px'], [390, null, '15px']]) { + await page.setViewportSize({ width, height: 1000 }); + await openConcept(page, 'review', 'dark'); + const concept = node((await conceptInventory(page)).main, 'div.gallery-layout'); + await openReview(page, 'dark'); + const actual = node((await productionInventory(page)).main, 'div.gallery-layout'); + expect([actual.gridTemplateColumns, actual.columnGap], `${width}px`).toEqual([concept.gridTemplateColumns, concept.columnGap]); + expect([actual.gridTemplateColumns.split(' ').at(-1), actual.columnGap], `${width}px`).toEqual([columns ?? `${actual.width}px`, gap]); + expect(actual.gridTemplateColumns.split(' ').length, `${width}px`).toBe(width <= 700 ? 1 : 2); + } +}); + +test('SAVED VIEWS rows match the concept with Needs review after Favorites', async ({ page, request }) => { + await seed(request); + await page.setViewportSize({ width: 1600, height: 1100 }); + await openConcept(page, 'review', 'dark'); + const concept = (await inventoryOf(page, '#main')).chrome.navs.find(nav => nav.id === 'saved-nav'); + await openReview(page, 'dark'); + const actual = (await inventoryOf(page, '#workspace-main')).chrome.navs.find(nav => nav.id === 'saved-nav'); + // Sample data: the concept stars three images; this queue's fixture stars none. Needs review counts the queue (3). + const reference = { ...concept, rows: concept.rows.map(row => row.text === 'Favorites' ? { ...row, count: { ...row.count, text: '0' } } : row) }; + expect(diffInventory(reference, actual)).toEqual([]); + expect(actual.rows.map(row => [row.text, row.count?.text, row.current])).toEqual([['Favorites', '0', null], ['Needs review', '3', 'page']]); + // The count's auto start margin follows the row's own text; every other count token is shared. + const { text: favoriteCount, marginInlineStart: favoriteMargin, ...favoriteStyle } = actual.rows[0].count; + const { text: reviewCount, marginInlineStart: reviewMargin, ...reviewStyle } = actual.rows[1].count; + expect(reviewStyle).toEqual(favoriteStyle); + expect(actual.rows[1].icon.d).toBe('M4 3h16v18H4zM8 7h8M8 12h8M8 17h5'); +}); + +test('review Tag dialog matches the needs-review-tagging reference', async ({ page, request }) => { + await seed(request); + await saveImageTags(request, `review/${CONCEPT_FILE}`, CONCEPT_TAGS); + await page.setViewportSize({ width: 1600, height: 1100 }); + await openConcept(page, 'review', 'dark'); + await page.locator('#main section.surface').getByRole('button', { name: 'Tag image', exact: true }).click(); + await expect(tagDialog(page)).toBeVisible(); + await settle(page); + const opened = await page.locator('dialog#modal').evaluate(taggingInventory); + await page.keyboard.press('Escape'); + await openReview(page, 'dark'); + await panel(page).getByRole('button', { name: 'Tag image', exact: true }).click(); + await expect(tagDialog(page).locator('img.tag-image-preview')).toHaveAttribute('src', /\/image-annotations\/capture\//); + await expect(tagDialog(page).locator('#manual-image-tags')).toHaveValue(CONCEPT_TAGS.join(', ')); + await settle(page); + const actual = await page.locator('dialog#modal').evaluate(taggingInventory); + const numberField = opened.modelPanel.fields.find(field => field.control.id === 'image-general-threshold'); + const mapped = mapConceptTagging(opened, { instructions: INSTRUCTIONS, chip: opened.heading.size, numberField }); + // D119: an undecided queue item names no prompt, so the heading and the preview's alt are the file name. + const reference = { ...mapped, heading: { ...mapped.heading, h3: { ...mapped.heading.h3, text: CONCEPT_FILE }, + preview: { ...mapped.heading.preview, alt: CONCEPT_FILE } } }; + expect(actual.scopeNote.text).toBe('Tags belong to this image. Resolve its prompt association separately in Needs review.'); + expect(diffTaggingInventory(reference, actual)).toEqual([]); + await page.keyboard.press('Escape'); + await expect(tagDialog(page)).toHaveCount(0); +}); + +test('the comparison notices a dropped notice border and reordered actions', async ({ page, request }) => { + const { actual, reference } = await compareAt(page, request, 1600, 1100, 'dark'); + expect(diffInventory(reference, actual)).toEqual([]); + const undoBorder = await mutate(page, { css: '#workspace-main .notice { border: 0 !important; }' }); + const borderless = await productionInventory(page); + expect(diffInventory(reference, borderless).map(item => item.path)).toEqual(expect.arrayContaining([ + 'main.children.1.borderTopWidth', 'main.children.1.borderTopStyle'])); + await undoBorder(); + await panel(page).locator('.dialog-actions').evaluate(row => row.prepend(row.lastElementChild)); + const reordered = await productionInventory(page); + expect(diffInventory(reference, reordered).map(item => item.path)).toEqual(expect.arrayContaining([ + 'main.children.2.children.1.children.6.children.0.text', 'main.children.2.children.1.children.6.children.0.primary'])); + await panel(page).locator('.dialog-actions').evaluate(row => row.prepend(row.lastElementChild)); + expect(diffInventory(reference, await productionInventory(page))).toEqual([]); +}); + +// Paired captures for the PR (no baselines): the populated queue beside the concept in both themes and widths, then +// the states the concept lacks (conflict, limited item, unavailable file, empty queue) as production captures. +test('paired Needs review captures', async ({ page, request }, testInfo) => { + test.setTimeout(120_000); + const shot = async name => { + await settle(page); + const file = testInfo.outputPath(`${name}.png`); + await page.screenshot({ path: file, fullPage: true }); + await testInfo.attach(name, { path: file, contentType: 'image/png' }); + }; + await seed(request); + for (const [width, height] of SIZES) for (const theme of THEMES) { + await page.setViewportSize({ width, height }); + await openConcept(page, 'review', theme); + await shot(`concept-review-${width}-${theme}`); + await openReview(page, theme); + await shot(`production-review-${width}-${theme}`); + } + await page.setViewportSize({ width: 1600, height: 1100 }); + await seed(request, { items: 0, conflict: true, limited: true, unavailable: true }); + await openReview(page, 'dark'); + await panel(page).getByRole('combobox', { name: 'Sampler context', exact: true }).selectOption({ index: 0 }); + const decided = page.waitForResponse(item => /\/decision$/.test(item.url())); + await panel(page).getByRole('button', { name: 'Apply association', exact: true }).click(); + expect((await decided).status()).toBe(409); + await expect(panel(page).locator('.warning-note')).toBeVisible(); + await shot('production-review-conflict'); + for (const [index, name] of [[1, 'limited'], [2, 'unavailable']]) { + await page.locator('#workspace-main .review-queue article').nth(index).getByRole('button', { name: /^Inspect / }).click(); + await expect(panel(page).locator('.warning-note')).toBeVisible(); + await shot(`production-review-${name}`); + } + await seed(request, { items: 0 }); + await openProduction(page, 'review', 'dark'); + await expect(page.locator('#workspace-main .notice h2')).toHaveText('No images need a decision'); + await shot('production-review-empty'); +}); diff --git a/tests/browser/server.py b/tests/browser/server.py index 81d0e45d..bdcaf465 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -40,6 +40,7 @@ from tests.browser.workspace_library_editor_fixture import ( ) from tests.browser.workspace_vocabulary_fixture import register_vocabulary_commands from tests.browser.workspace_tagging_fixture import register_tagging_commands +from tests.browser.workspace_review_fixture import register_review_commands CANONICAL_ROOT = Path(__file__).resolve().parents[2] CANONICAL_ASSETS = { @@ -197,6 +198,7 @@ def create_app(root: Path) -> web.Application: library_editor_commands = register_library_editor_commands(routes, api, root) register_vocabulary_commands(routes, api, root) register_tagging_commands(routes, api, root, model) + register_review_commands(routes, api, root) reveal_stub = register_reveal_stub(routes, api, root) register_activity_commands(app, routes, api) register_system_commands(app, routes, api, root) @@ -598,6 +600,8 @@ def create_app(root: Path) -> web.Application: api.db.close_all() seed() + # Open Needs review change streams end first, as on the real host. + app.on_shutdown.append(api.close_review_events) app.on_cleanup.append(cleanup) app.add_routes(routes) return app diff --git a/tests/browser/shell-concept.spec.mjs b/tests/browser/shell-concept.spec.mjs index 14976317..878b9c55 100644 --- a/tests/browser/shell-concept.spec.mjs +++ b/tests/browser/shell-concept.spec.mjs @@ -68,12 +68,12 @@ function remainingFooter(footer,spans) { const OVERVIEW_GLYPH='M3 11 12 4l9 7M5 10v10h14V10M10 20v-6h4v6'; /** - * The concept's SAVED VIEWS rows (app.js a331–335) as production lists them: Favorites with its count (D51), then the - * saved views. D50: no "Needs review" row until the tag-review slice (maintainer ruling 2026-10-21). + * The concept's SAVED VIEWS rows (app.js a331–335) as production lists them: Favorites with its count (D51), Needs + * review with the exact pending count (ruling R5), then the saved views. The concept's "3" is sample data: the concept + * fixture records no review items, so production's count reads "0". */ function savedNav(nav) { - const rows=nav.rows.filter(row=>row.text!=='Needs review'); - return {...nav,rows:rows.map((row,index)=>index===rows.length-1?{...row,gapAfter:null}:row)}; + return {...nav,rows:nav.rows.map(row=>row.text==='Needs review'?{...row,count:{...row.count,text:'0'}}:row)}; } /** Recorded shell deviations only (spec §8). Everything else in the concept inventory must equal production. */ @@ -243,8 +243,9 @@ for(const [width,height] of SIZES)test(`a saved view row matches the concept's $ await openProduction(page,'images','dark'); const actual=await save('Save','#workspace-main'); expect(diffInventory(mapShellConcept(reference).chrome.navs,actual.chrome.navs)).toEqual([]); + const referenceRows=reference.chrome.navs.find(nav=>nav.id==='saved-nav').rows; expect(actual.chrome.navs.find(nav=>nav.id==='saved-nav').rows.map(row=>[row.text,row.count])) - .toEqual([['Favorites',reference.chrome.navs.find(nav=>nav.id==='saved-nav').rows[0].count],['Golden hour',null]]); + .toEqual([['Favorites',referenceRows[0].count],['Needs review',{...referenceRows[1].count,text:'0'}],['Golden hour',null]]); }); test('the inventory reports a missing shell element by its path instead of failing',async({page})=>{ diff --git a/tests/browser/system-concept.spec.mjs b/tests/browser/system-concept.spec.mjs index 6ff9145a..a0b0adaa 100644 --- a/tests/browser/system-concept.spec.mjs +++ b/tests/browser/system-concept.spec.mjs @@ -169,10 +169,9 @@ function mapOverviewConcept(reference, overview) { const relabel = { 'Database integrity': 'Not checked', 'Positive prompts': number(overview.library.positive_prompts), 'Negative variants': number(overview.library.negative_variants), Images: number(overview.images.indexed) }; const rows = { - // D93: the real pending count; the action is unavailable (dimmed, as the Save row's D92) until the review page. + // D93: the real pending count; Review metadata opens Needs review (the concept's go-review). '3 ambiguous image links': row => ({ ...row, - title: `${number(overview.needs_review.pending)} ambiguous image link${overview.needs_review.pending === 1 ? '' : 's'}`, - action: { ...row.action, opacity: '0.55', cursor: 'not-allowed' } }), + title: `${number(overview.needs_review.pending)} ambiguous image link${overview.needs_review.pending === 1 ? '' : 's'}` }), // D93: no backup has been recorded (verified backups arrive with S8). 'Backup status': row => ({ ...row, description: 'No retained backup record.' }), // D93: there is no paging, so the shared settings are grid and density. @@ -231,11 +230,10 @@ for (const theme of THEMES) for (const [width, height] of SIZES) { }); } -// The recorded differences, by path: D93 the review row (its real count, dimmed action), the backup row, the +// The recorded differences, by path: D93 the review row (its real count), the backup row, the // preferences row and the sources row, and metric 4; D96 the statistics row; D92 the chip, the status and the dimmed // Save; D122 the nav label. (The fixture's first three metrics are the concept's own counts.) const UNMAPPED_PATHS = [ - 'columns.surfaces.0.rows.0.action.cursor', 'columns.surfaces.0.rows.0.action.opacity', 'columns.surfaces.0.rows.0.title', 'columns.surfaces.0.rows.1.description', 'columns.surfaces.1.rows.0.description', 'columns.surfaces.1.rows.1.title', 'columns.surfaces.1.rows.2.description', 'heading.actions.0', 'heading.actions.length', 'metrics.items.3.value', 'nav.label', diff --git a/tests/browser/tagging-canonical-comparison.mjs b/tests/browser/tagging-canonical-comparison.mjs index d9ff0fd5..88784248 100644 --- a/tests/browser/tagging-canonical-comparison.mjs +++ b/tests/browser/tagging-canonical-comparison.mjs @@ -140,3 +140,152 @@ export function mapConceptTagging(reference,{instructions,limit=10,chip,numberFi export function withoutChipGeometry(inventory) { return {...inventory,suggestions:{...inventory.suggestions,rows:inventory.suggestions.rows.map(({paddingTop,paddingLeft,alignItems,...row})=>row)}}; } + +/** + * Needs review (plan Task F3, Mock inventory M4: app.js renderReview a749–760, style.css s407–415, s796–814 and their + * breakpoints): the page's main element as a tree, runs in the page. Every shown box below `main` to `depth` levels is + * recorded with its tag and classes, its box relative to its parent, its fonts, edges and layout, a leaf's text, a + * button's label and icon, a select's options; a `display: contents` node is replaced by its children (D20). A flex + * row of actions also records the gaps between its children and its right inset, so a row whose button copy differs + * (D29) still compares by order, spacing and alignment. + */ +export function reviewInventory(selector) { + const main=document.querySelector(selector); + // Half pixels: the concept's 13.5px line heights add up exactly, so a box that loses one line moves by 13.5px. + const px=value=>Math.round(value*2)/2; + const css=(node,...keys)=>{const style=getComputedStyle(node);return Object.fromEntries(keys.map(key=>[key,style[key]]));}; + const rect=node=>node.getBoundingClientRect(); + const shown=node=>node.getClientRects().length>0&&getComputedStyle(node).visibility!=='hidden'; + const relative=(node,origin)=>{const a=rect(node),b=rect(origin);return {left:px(a.left-b.left),top:px(a.top-b.top),width:px(a.width),height:px(a.height)};}; + const font=node=>css(node,'fontFamily','fontSize','fontWeight','lineHeight','letterSpacing','color','textTransform','textAlign'); + const edges=node=>css(node,'paddingTop','paddingRight','paddingBottom','paddingLeft','marginTop','marginRight','marginBottom','marginLeft', + 'borderTopWidth','borderTopStyle','borderTopColor','borderRightWidth','borderRightStyle','borderRightColor','borderBottomWidth', + 'borderBottomStyle','borderBottomColor','borderLeftWidth','borderLeftStyle','borderLeftColor','borderTopLeftRadius','backgroundColor', + 'boxShadow','outlineStyle'); + const layout=node=>css(node,'display','position','flexDirection','flexWrap','justifyContent','alignItems','columnGap','rowGap', + 'gridTemplateColumns','overflowX','overflowY','objectFit','minHeight','minWidth'); + const tagOf=node=>`${node.localName}${[...node.classList].sort().map(name=>`.${name}`).join('')}`; + const textOf=node=>node.textContent.replace(/\s+/g,' ').trim(); + const boxed=node=>[...node.children].flatMap(child=>getComputedStyle(child).display==='contents'?boxed(child):[child]); + const LEAVES=new Set(['button','select','input','img','svg','textarea','progress']); + function nodeOf(node,origin,depth) { + const base={tag:tagOf(node),...relative(node,origin),...font(node),...edges(node),...layout(node)}; + const name=node.localName; + if(name==='svg')return {tag:base.tag,width:base.width,height:base.height,d:node.querySelector('path')?.getAttribute('d')??null, + ...css(node,'strokeWidth','color','verticalAlign')}; + if(name==='button')return {...base,text:textOf(node),label:node.getAttribute('aria-label'),pressed:node.getAttribute('aria-pressed'), + disabled:node.getAttribute('aria-disabled')==='true'||node.disabled,primary:node.classList.contains('ca-primary'), + children:boxed(node).filter(shown).map(child=>nodeOf(child,node,0))}; + if(name==='select')return {...base,id:node.id,options:[...node.options].map(option=>[option.textContent,option.disabled])}; + if(name==='input')return {...base,id:node.id,type:node.type,label:node.getAttribute('aria-label')}; + if(name==='img')return {...base,alt:node.getAttribute('alt')}; + const children=boxed(node).filter(shown); + const item={...base,id:node.id||null,role:node.getAttribute('role'),label:node.getAttribute('aria-label'), + text:children.length?null:textOf(node),htmlFor:node.htmlFor||null}; + if(/(^|\s)(dialog-actions|row-actions|image-card-actions)(\s|$)/.test(node.className)&&children.length) { + const boxes=children.map(rect); + item.gaps=boxes.slice(1).map((box,index)=>px(box.left-boxes[index].right)); + item.rightInset=px(rect(node).right-boxes.at(-1).right); + } + if(depth>0&&!LEAVES.has(name))item.children=children.map(child=>nodeOf(child,node,depth-1)); + return item; + } + return {main:nodeOf(main,main,8),noPageScroll:document.documentElement.scrollWidth<=innerWidth+1}; +} + +/** + * `tree` with `edit(node)` applied to every node `match` picks (a tag prefix or a predicate), depth first, new objects + * only. + */ +function editNodes(tree,match,edit) { + const children=tree.children?.map(child=>editNodes(child,match,edit)); + const node=children?{...tree,children}:tree; + const picked=typeof match==='function'?match(node):node.tag?.startsWith(match); + return picked?edit(node):node; +} +const findNode=(tree,prefix)=>tree.tag?.startsWith(prefix)?tree:tree.children?.map(child=>findNode(child,prefix)).find(Boolean)??null; + +/** + * The concept's Metadata review inventory (reviewInventory) with the recorded deviations applied, the remaining sample + * data taken from production (`actual`) only where the concept's own copy cannot say it: + * - D29: no "Sample review queue" chip in the notice; "Resolve the association" and "Apply association" without the + * prototype's "sample"; + * - D119: queue cards carry no selection checkbox and no star (the overlay row stays, empty); a card's title is its + * file name (an undecided item names no prompt) and its line is the extension only (D11 omits the unknown size and + * seed, which a queue item does not carry); the image's alt is the file name; the prompt blocks' Copy and Send names + * carry no prompt title; + * - D3: the grid keeps the Images grid's `position: relative` (its cards stay in its flow); + * - production's hooks and names: the `review-queue` / `review-panel` classes, the queue's list roles and name, the + * panel heading's id (the region's name) and the prompt sections' region names; + * - sample data: the Sampler context's candidate options (the file's own evidence; the last option, Leave unresolved, + * stays compared) and the widths that follow from copy the deviations above change (a card's text column, the + * panel's action buttons; their gaps and right inset stay compared). + * Where that copy changes the height of a box, the boxes after it move by exactly that much: the stacked notice (390) + * loses the chip and its row gap; a card whose concept line wraps (390: "1536 × 1152 · PNG · Seed 42001") takes + * production's one-line height, its row of the grid shrinks by the most any of its cards lost, and in one column the + * panel below the grid moves up by the grid's loss. The actions keep production's centred place in the card's info. + */ +export function mapConceptReview(reference,actual) { + const named=(node,id)=>({...node,id}); + const actualCards=findNode(actual.main,'div.image-grid')?.children??[]; + const actualActions=findNode(actual.main,'div.dialog-actions'); + let cardIndex=0; + let main={...reference.main,id:actual.main.id}; + let noticeLoss=0; + main=editNodes(main,'div.notice',notice=>{ + const chip=notice.children.find(child=>child.tag==='span.chip'); + if(chip&¬ice.flexDirection==='column')noticeLoss=chip.height+parseFloat(notice.rowGap); + return {...notice,height:notice.height-noticeLoss,children:notice.children.filter(child=>child!==chip)}; + }); + const noticeIndex=main.children.findIndex(child=>child.tag==='div.notice'); + main={...main,height:main.height-noticeLoss,children:main.children.map((child,index)=>index>noticeIndex?{...child,top:child.top-noticeLoss}:child)}; + const losses=[]; + main=editNodes(main,'div.image-grid',grid=>({...grid,tag:'div.image-grid.review-queue',position:'relative',role:'list',label:'Review queue'})); + main=editNodes(main,'article.media-card',card=>{ + const twin=actualCards[cardIndex++]; + const filename=findNode(card,'button.image-open').label.replace(/^Open /,''); + const art=editNodes(card.children[0],'div.image-overlays',overlays=>({...overlays,height:0,text:'',children:[]})); + const withAlt=editNodes(art,'img',img=>({...img,alt:filename})); + const info=card.children[1]; + const [copy,...rest]=info.children; + const twinCopy=twin?.children[1]?.children[0]; + const [h3,p]=copy.children; + const twinLine=twinCopy?.children[1]; + const loss=twinLine?p.height-twinLine.height:0; + losses.push({top:card.top,loss}); + const mappedCopy={...copy,width:twinCopy?.width,height:copy.height-loss,children:[{...h3,text:filename,width:twinCopy?.children[0]?.width}, + {...p,text:'PNG',width:twinLine?.width,height:p.height-loss}]}; + const [actions,...after]=rest; + const twinActions=twin?.children[1]?.children[1]; + const mappedActions=actions&&loss?{...actions,top:twinActions?.top}:actions; + return {...card,role:'listitem',height:card.height-loss, + children:[withAlt,{...info,height:info.height-loss,children:[mappedCopy,...(actions?[mappedActions]:[]),...after]},...card.children.slice(2)]}; + }); + // Each grid row loses what its most shortened card lost; later rows move up by the rows before them. + const rowLoss=new Map(); + for(const {top,loss} of losses)rowLoss.set(top,Math.max(rowLoss.get(top)??0,loss)); + const lossBefore=top=>[...rowLoss].filter(([rowTop])=>rowTopsum+loss,0); + const gridLoss=[...rowLoss.values()].reduce((sum,loss)=>sum+loss,0); + main=editNodes(main,'div.image-grid',grid=>({...grid,height:grid.height-gridLoss, + children:grid.children.map(card=>({...card,top:card.top-lossBefore(card.top)}))})); + main=editNodes(main,'div.gallery-layout',layout=>{ + if(!gridLoss||layout.gridTemplateColumns.trim().split(/\s+/).length>1)return layout; + return {...layout,height:layout.height-gridLoss,children:layout.children.map((child,index)=>index?{...child,top:child.top-gridLoss}:child)}; + }); + if(gridLoss&&findNode(main,'div.gallery-layout').gridTemplateColumns.trim().split(/\s+/).length===1)main={...main,height:main.height-gridLoss}; + main=editNodes(main,'section.surface',surface=>{ + const [h2,...rest]=surface.children; + return {...surface,tag:'section.review-panel.surface',children:[named({...h2,text:h2.text.replace('Resolve the sample association','Resolve the association')},'review-panel-title'),...rest]}; + }); + main=editNodes(main,'select',select=>{ + const twin=findNode(actual.main,'select'); + return select.id==='review-context'?{...select,options:[...twin.options.slice(0,-1),select.options.at(-1)]}:select; + }); + main=editNodes(main,node=>/^section\..*\bprompt-section\b/.test(node.tag),section=>({...section,label:section.tag.includes('positive')?'Positive prompt':'Negative prompt'})); + main=editNodes(main,node=>node.tag?.startsWith('button')&&/^(Copy|Send) (positive|negative) prompt/.test(node.label??''), + button=>({...button,label:button.label.replace(/: .+$/,'')})); + main=editNodes(main,'div.dialog-actions',actions=>({...actions,children:actions.children.map((button,index)=>({...button, + text:button.text.replace('Apply sample association','Apply association'), + left:actualActions?.children[index]?.left,width:actualActions?.children[index]?.width}))})); + return {...reference,main}; +} diff --git a/tests/browser/workspace-review.spec.mjs b/tests/browser/workspace-review.spec.mjs new file mode 100644 index 00000000..cc9b8ed3 --- /dev/null +++ b/tests/browser/workspace-review.spec.mjs @@ -0,0 +1,556 @@ +// Needs review (#249 PR F, plan Tasks F1 and F2, Mock inventory M4): the SAVED VIEWS row and its count, the Metadata +// review page over the real review routes, and Activity's Review metadata. Items are recorded by the real recorder +// (tests/browser/workspace_review_fixture.py) through the monitor's entry point; the conflict item's file was linked +// to another prompt after it was queued. An open page re-reads its queue from the review change stream (F2). +import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; +import { test, expect } from './fixtures.mjs'; + +const review = '/prompt_manager/workspace#/review'; +const main = page => page.locator('#workspace-main'); +const savedNav = page => page.getByRole('navigation', { name: 'Saved views', exact: true }); +const reviewRow = page => savedNav(page).locator('a[data-route="review"]'); +const panel = page => main(page).getByRole('region', { name: 'Resolve the association', exact: true }); +const context = page => panel(page).getByRole('combobox', { name: 'Sampler context', exact: true }); +const cards = page => main(page).locator('.review-queue article.media-card'); +const toast = page => page.locator('#workspace-toast'); +const NOTICE = 'Multiple sampler paths were found. The existing image links were preserved. Choose the correct context or leave them unresolved.'; +const EMPTY = 'Ambiguous images are added when a metadata scan or a new generation finds more than one sampler path.'; +const OPTIONS = ['Sampler 11 · Golden dunes at dusk / Soft negative, blur', 'Sampler 13 · Harbor lights at night / Soft negative, blur', + 'Leave unresolved']; + +async function seed(request, items = 3, special = {}) { + const response = await request.post('/__test__/review/reset', { data: { items, ...special } }); + await expect(response).toBeOK(); + return response.json(); +} +async function pending(request) { + return (await (await request.get('/prompt_manager/review/items?limit=1')).json()).total; +} +const links = async request => (await (await request.get('/__test__/review/links')).json()).links; +const streams = async request => (await (await request.get('/__test__/review/streams')).json()); +const apply = page => panel(page).getByRole('button', { name: 'Apply association', exact: true }); +const warning = page => panel(page).locator('.warning-note'); +/** The page with its first item's sampler paths loaded. */ +async function open(page) { + await page.goto(review); + await expect(context(page)).toBeEnabled(); +} + +test.beforeEach(async ({ page, browserName }) => { + await startCoverage(page, browserName); + page.reviewErrors = []; + page.on('pageerror', error => page.reviewErrors.push(error.message)); +}); +test.afterEach(async ({ page, browserName, request }) => { + await saveCoverage(page, browserName); + expect(page.reviewErrors).toEqual([]); + await request.post('/__test__/jobs/restart'); +}); + +test('queue shows persisted items and the nav count', async ({ page, request }) => { + await seed(request, 3); + await open(page); + // SAVED VIEWS: Favorites, then Needs review with the exact pending count (ruling R5). + await expect(savedNav(page).locator('a').nth(1)).toHaveAttribute('data-route', 'review'); + await expect(reviewRow(page)).toHaveText('Needs review3'); + await expect(reviewRow(page)).toHaveAttribute('aria-current', 'page'); + await expect(page).toHaveTitle('Review · Prompt Manager'); + await expect(page.locator('#workspace-breadcrumb')).toHaveText('Review'); + await expect(main(page).getByRole('heading', { level: 1 })).toHaveText('Metadata review'); + await expect(main(page).locator('.heading p.muted')).toHaveText('Uncertain evidence stays visible until you decide.'); + await expect(main(page).locator('.notice h2')).toHaveText('3 images need a decision'); + await expect(main(page).locator('.notice p')).toHaveText(NOTICE); + await expect(cards(page)).toHaveCount(3); + await expect(cards(page).first()).toHaveClass(/selected/); + await expect(panel(page).locator('p.small')).toHaveText('review_0000.png'); + await expect(context(page).locator('option')).toHaveText(OPTIONS); + await expect(context(page)).toHaveValue(/^[0-9a-f]{64}$/); + await expect(panel(page).locator('.prompt-section.positive .prompt-text')).toHaveText('Golden dunes at dusk'); + await expect(panel(page).locator('.prompt-section.negative .prompt-text')).toHaveText('Soft negative, blur'); + await context(page).selectOption({ label: OPTIONS[1] }); + await expect(panel(page).locator('.prompt-section.positive .prompt-text')).toHaveText('Harbor lights at night'); + await context(page).selectOption({ label: 'Leave unresolved' }); + await expect(panel(page).locator('.prompt-section.positive .prompt-text')).toHaveText('Multiple possible prompts. Source could not be determined.'); + // Info selects another item. + await cards(page).nth(1).getByRole('button', { name: 'Inspect review_0001.png', exact: true }).click(); + await expect(panel(page).locator('p.small')).toHaveText('review_0001.png'); + await expect(cards(page).nth(1)).toHaveClass(/selected/); + await expect(context(page)).toBeEnabled(); + // The card's image opens the shared viewer. + await cards(page).nth(1).getByRole('button', { name: 'Open review_0001.png', exact: true }).click(); + await expect(page.locator('#viewer-dialog')).toBeVisible(); + await page.keyboard.press('Escape'); + await expect(page.locator('#viewer-dialog')).toHaveCount(0); + // Open activity. + await main(page).getByRole('button', { name: 'Open activity', exact: true }).click(); + await expect(page).toHaveURL(/#\/activity$/); +}); + +test('apply candidate links the pair and the item leaves the queue and the count drops', async ({ page, request }) => { + await seed(request, 2); + await open(page); + await expect(reviewRow(page)).toHaveText('Needs review2'); + const decided = page.waitForResponse(item => /\/review\/items\/[^/]+\/decision$/.test(item.url())); + await panel(page).getByRole('button', { name: 'Apply association', exact: true }).click(); + expect((await decided).status()).toBe(200); + await expect(toast(page)).toHaveText('Association accepted.'); + await expect(page).toHaveURL(/#\/images$/); + await expect(reviewRow(page)).toHaveText('Needs review1'); + expect(await pending(request)).toBe(1); + // The first sampler path's pair is linked to that file, and only to it. + expect(await links(request)).toEqual([{ filename: 'review_0000.png', prompt: 'Golden dunes at dusk', + pair_id: expect.any(Number), positive: 'Golden dunes at dusk', negative: 'Soft negative, blur' }]); +}); + +test('conflict leaves links unchanged and explains', async ({ page, request }) => { + // Queued first, then linked to another prompt (before a scan or monitor pass could settle it). + await seed(request, 0, { conflict: true }); + const before = await links(request); + expect(before).toEqual([{ filename: 'conflict.png', prompt: 'Misty forest path', pair_id: null, positive: null, + negative: null }]); + await open(page); + await expect(panel(page).locator('p.small')).toHaveText('conflict.png'); + await expect(context(page).locator('option')).toHaveText(OPTIONS); + const decided = page.waitForResponse(item => /\/decision$/.test(item.url())); + await apply(page).click(); + const answer = await decided; + expect(answer.status()).toBe(409); + expect((await answer.json()).code).toBe('association-conflict'); + await expect(warning(page)).toHaveText('This image is already linked to another prompt or negative, so this sampler path cannot be attached safely. The existing links were preserved.'); + await expect(page).toHaveURL(/#\/review$/); + await expect(context(page)).toBeEnabled(); + await expect(context(page)).toHaveValue(/^[0-9a-f]{64}$/); + expect(await links(request)).toEqual(before); + expect(await pending(request)).toBe(1); + await expect(reviewRow(page)).toHaveText('Needs review1'); + // The file can still be left unresolved, which links nothing either. + await context(page).selectOption({ label: 'Leave unresolved' }); + await apply(page).click(); + await expect(toast(page)).toHaveText('Left unresolved.'); + expect(await links(request)).toEqual(before); + expect(await pending(request)).toBe(0); +}); + +test('a limited item and an unavailable file can only be left unresolved', async ({ page, request }) => { + await seed(request, 0, { limited: true, unavailable: true }); + await open(page); + await expect(cards(page)).toHaveCount(2); + await expect(panel(page).locator('p.small')).toHaveText('limited.png'); + await expect(context(page).locator('option')).toHaveText(['Leave unresolved']); + await expect(warning(page)).toHaveText('This file has too many sampler paths to list. You can leave it unresolved.'); + // The unavailable file: no current root lists it, so its media and Tag are inert. + const unavailable = cards(page).nth(1); + await expect(unavailable.locator('.image-open')).toHaveAttribute('aria-disabled', 'true'); + await expect(unavailable.locator('.image-card-actions button').last()).toHaveAttribute('aria-disabled', 'true'); + // F3: its empty media button keeps the concept's 1.33 media area, so the card lines up with its neighbours. + await expect(cards(page).first().locator('.image-open img')).toBeVisible(); + const sizes = await cards(page).evaluateAll(nodes => nodes.map(node => { + const art = node.querySelector('.image-open').getBoundingClientRect(); + return [Math.round(art.width), Math.round(art.height), Math.round(node.getBoundingClientRect().height)]; + })); + expect(sizes[1]).toEqual(sizes[0]); + await unavailable.getByRole('button', { name: 'Inspect unavailable.png', exact: true }).click(); + await expect(panel(page).locator('p.small')).toHaveText('unavailable.png'); + await expect(warning(page)).toHaveText('This file is not under a current image source, so it cannot be opened. You can leave it unresolved.'); + await expect(panel(page).getByRole('button', { name: 'Tag image', exact: true })).toHaveAttribute('aria-disabled', 'true'); + const observes = []; + page.on('request', item => { if (item.url().endsWith('/image-annotations/observe')) observes.push(item.url()); }); + await apply(page).click(); + await expect(toast(page)).toHaveText('Left unresolved.'); + expect(observes).toEqual([]); + expect(await pending(request)).toBe(1); +}); + +test('leave unresolved removes the item from pending', async ({ page, request }) => { + await seed(request, 2); + await open(page); + await context(page).selectOption({ label: 'Leave unresolved' }); + await panel(page).getByRole('button', { name: 'Apply association', exact: true }).click(); + await expect(toast(page)).toHaveText('Left unresolved.'); + await expect(page).toHaveURL(/#\/images$/); + expect(await pending(request)).toBe(1); +}); + +test('leave for later writes nothing and goes to Images', async ({ page, request }) => { + await seed(request, 2); + await open(page); + const writes = []; + page.on('request', item => { if (item.method() !== 'GET') writes.push(item.url()); }); + await panel(page).getByRole('button', { name: 'Leave for later', exact: true }).click(); + await expect(page).toHaveURL(/#\/images$/); + expect(writes).toEqual([]); + expect(await pending(request)).toBe(2); +}); + +test('tag image from review saves tags and the item remains pending', async ({ page, request }) => { + await seed(request, 1); + await open(page); + const before = await links(request); + await panel(page).getByRole('button', { name: 'Tag image', exact: true }).click(); + const dialog = page.getByRole('dialog', { name: 'Tag image', exact: true }); + await expect(dialog).toBeVisible(); + await expect(dialog.locator('p.tag-scope-note')).toHaveText('Tags belong to this image. Resolve its prompt association separately in Needs review.'); + await dialog.getByRole('textbox', { name: 'Image tags · edit manually', exact: true }).fill('dunes'); + await dialog.getByRole('button', { name: 'Save manual tags', exact: true }).click(); + await expect(toast(page)).toHaveText('1 tags saved for review_0000.png. Prompt association still awaits review.'); + await expect(dialog).toHaveCount(0); + expect(await pending(request)).toBe(1); + await expect(page).toHaveURL(/#\/review$/); + // The item is untouched: still listed, still open, nothing linked. + await expect(cards(page)).toHaveCount(1); + await expect(context(page)).toBeEnabled(); + expect(await links(request)).toEqual(before); + const [item] = (await (await request.get('/prompt_manager/review/items')).json()).items; + expect([item.state, item.revision]).toEqual(['pending', 0]); + // The card's Tag opens the same dialog. + await cards(page).first().getByRole('button', { name: 'Tag or auto-tag review_0000.png', exact: true }).click(); + await expect(dialog).toBeVisible(); + await expect(dialog.locator('p.tag-scope-note')).toBeVisible(); + await page.keyboard.press('Escape'); + await expect(dialog).toHaveCount(0); +}); + +test('an empty queue shows the empty notice only', async ({ page, request }) => { + await seed(request, 0); + await page.goto(review); + await expect(main(page).locator('.notice h2')).toHaveText('No images need a decision'); + await expect(main(page).locator('.notice p')).toHaveText(EMPTY); + await expect(main(page).locator('.gallery-layout')).toHaveCount(0); + await expect(reviewRow(page)).toHaveText('Needs review0'); +}); + +test('load more appends the next page', async ({ page, request }) => { + await seed(request, 45); + await open(page); + await expect(cards(page)).toHaveCount(40); + const continuation = main(page).locator('.infinite-continuation'); + await expect(continuation.locator('p')).toHaveText('40 of 45 images'); + await continuation.getByRole('button', { name: 'Load more images', exact: true }).click(); + await expect(cards(page)).toHaveCount(45); + await expect(continuation).toHaveCount(0); +}); + +test('a scan job with review items offers Review metadata in Activity', async ({ page, request }) => { + await seed(request, 1); + const started = await (await request.post('/__test__/jobs/start', { data: { total: 10, review_items: 2 } })).json(); + await expect(await request.post(`/__test__/jobs/${started.job.id}/finish`, { data: {} })).toBeOK(); + const plain = await (await request.post('/__test__/jobs/start', { data: { total: 10 } })).json(); + await expect(await request.post(`/__test__/jobs/${plain.job.id}/finish`, { data: {} })).toBeOK(); + await page.goto('/prompt_manager/workspace#/activity'); + const item = main(page).locator(`.activity-item[data-job-id="${started.job.id}"]`); + await expect(item.getByRole('button', { name: 'Review metadata', exact: true })).toBeVisible(); + await expect(main(page).locator(`.activity-item[data-job-id="${plain.job.id}"] button`)).toHaveCount(0); + await page.goto(`/prompt_manager/workspace#/activity/${started.job.id}`); + const actions = main(page).locator('.heading-actions button'); + await expect(actions).toHaveText(['Download report', 'Review metadata']); + await actions.nth(1).click(); + await expect(page).toHaveURL(/#\/review$/); + await expect(main(page).getByRole('heading', { level: 1 })).toHaveText('Metadata review'); +}); + +test('a failed item read says so and Retry reads it again', async ({ page, request }) => { + await seed(request, 1); + await page.route(url => /\/prompt_manager\/review\/items\/[0-9a-f-]{36}$/.test(url.pathname), + route => route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ success: false, code: 'review-failed', error: 'x' }) }), + { times: 1 }); + await page.goto(review); + await expect(panel(page).locator('.warning-note')).toHaveText('This item could not be read. Select it again to retry.'); + await expect(panel(page).getByText('Could not read prompts from this file.', { exact: true })).toBeVisible(); + await expect(panel(page).getByRole('button', { name: 'Apply association', exact: true })).toHaveAttribute('aria-disabled', 'true'); + await panel(page).getByRole('button', { name: 'Retry prompt details', exact: true }).click(); + await expect(context(page)).toBeEnabled(); + await expect(context(page).locator('option')).toHaveText(OPTIONS); + await expect(panel(page).locator('.warning-note')).toBeHidden(); +}); + +test('a failed queue read offers Retry', async ({ page, request }) => { + await seed(request, 1); + await page.route(url => url.pathname.endsWith('/prompt_manager/review/items') && url.searchParams.get('limit') === '40', + route => route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ success: false, code: 'review-failed', error: 'x' }) }), + { times: 1 }); + await page.goto(review); + await expect(main(page).getByRole('alert')).toHaveText('The review queue could not be read.'); + await main(page).getByRole('button', { name: 'Retry', exact: true }).click(); + await expect(context(page)).toBeEnabled(); + await expect(main(page).locator('.notice h2')).toHaveText('1 image needs a decision'); +}); + +test('keyboard focus labels and touch targets', async ({ page, request }) => { + await seed(request, 2); + await open(page); + const first = cards(page).first(); + for (const [name, role] of [['Open review_0000.png', 'button'], ['Inspect review_0000.png', 'button'], + ['Tag or auto-tag review_0000.png', 'button']]) await expect(first.getByRole(role, { name, exact: true })).toBeVisible(); + await expect(main(page).getByRole('list', { name: 'Review queue', exact: true })).toBeVisible(); + await expect(panel(page)).toBeVisible(); + // Keyboard: Info on the second card opens it, and the panel's controls take focus in their order. + const info = cards(page).nth(1).getByRole('button', { name: 'Inspect review_0001.png', exact: true }); + await info.focus(); + await page.keyboard.press('Enter'); + await expect(panel(page).locator('p.small')).toHaveText('review_0001.png'); + await expect(context(page)).toBeEnabled(); + await context(page).focus(); + await expect(context(page)).toBeFocused(); + const order = await panel(page).evaluate(node => [...node.querySelectorAll('button, select')] + .filter(control => control.offsetParent !== null).map(control => control.getAttribute('aria-label') || control.textContent.trim())); + expect(order[0]).toBe('Tag image'); + expect(order.slice(-2)).toEqual(['Leave for later', 'Apply association']); + // Targets: the panel's controls are 40px on a coarse pointer (the concept's coarse buttons) and 24px otherwise (the + // WCAG 2.2 minimum); the card's buttons keep the concept's compact size, never under 24px (its ≤700px rule). + const coarse = await page.evaluate(() => matchMedia('(pointer: coarse)').matches); + const sized = [ + [panel(page).getByRole('button', { name: 'Tag image', exact: true }), coarse ? 40 : 24], + [context(page), coarse ? 40 : 24], + [panel(page).getByRole('button', { name: 'Leave for later', exact: true }), coarse ? 40 : 24], + [apply(page), coarse ? 40 : 24], + [first.getByRole('button', { name: 'Inspect review_0000.png', exact: true }), 24], + [first.getByRole('button', { name: 'Tag or auto-tag review_0000.png', exact: true }), 24], + ]; + for (const [control, minimum] of sized) { + const box = await control.boundingBox(); + const label = await control.evaluate(node => node.getAttribute('aria-label') || node.textContent.trim().slice(0, 40)); + expect(box.height, label).toBeGreaterThanOrEqual(minimum); + expect(box.width, label).toBeGreaterThanOrEqual(24); + } + // Enter on Leave for later returns to Images. + await panel(page).getByRole('button', { name: 'Leave for later', exact: true }).focus(); + await page.keyboard.press('Enter'); + await expect(page).toHaveURL(/#\/images$/); +}); + +test('an open page shows items the monitor adds and keeps the open choice', async ({ page, request }) => { + await seed(request, 1); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await context(page).selectOption({ label: OPTIONS[1] }); + await expect(reviewRow(page)).toHaveText('Needs review1'); + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect(cards(page)).toHaveCount(2); + await expect(main(page).locator('.notice h2')).toHaveText('2 images need a decision'); + await expect(reviewRow(page)).toHaveText('Needs review2'); + await expect(cards(page).first()).toHaveClass(/selected/); + await expect(panel(page).locator('p.small')).toHaveText('review_0000.png'); + await expect(context(page).locator('option:checked')).toHaveText(OPTIONS[1]); + await expect(warning(page)).toBeHidden(); +}); + +test('an open page shows items a metadata scan adds', async ({ page, request }) => { + await seed(request, 1); + await expect(await request.post('/__test__/review/add', { data: { kind: 'file' } })).toBeOK(); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await expect(cards(page)).toHaveCount(1); + const started = await request.post('/prompt_manager/jobs/scan', { data: { scope: 'new' } }); + expect(started.status()).toBe(202); + await expect(cards(page)).toHaveCount(2); + await expect(cards(page).nth(1).getByRole('button', { name: 'Inspect added_0000.png', exact: true })).toBeVisible(); + await expect(reviewRow(page)).toHaveText('Needs review2'); +}); + +test('a decision in another tab leaves the open page and opens the next image', async ({ page, request, context: browserContext }) => { + await seed(request, 2); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + const other = await browserContext.newPage(); + await open(other); + await context(other).selectOption({ label: 'Leave unresolved' }); + await apply(other).click(); + await expect(other).toHaveURL(/#\/images$/); + await other.close(); + await expect(cards(page)).toHaveCount(1); + await expect(panel(page).locator('p.small')).toHaveText('review_0001.png'); + await expect(warning(page)).toHaveText('review_0000.png was decided elsewhere, so the next image is open.'); + await expect(main(page).locator('.notice h2')).toHaveText('1 image needs a decision'); + await expect(reviewRow(page)).toHaveText('Needs review1'); + await expect(context(page)).toBeEnabled(); +}); + +/** The queue reads (`?limit=40`, cursors) and count reads (`?limit=1`) the page sends from now on. */ +function reviewReads(page) { + const reads = []; + page.on('request', item => { + const url = new URL(item.url()); + if (url.pathname === '/prompt_manager/review/items') reads.push(url.searchParams.get('cursor') ? 'more' : url.search); + }); + return reads; +} + +test('a visit reads its queue once, and a change reads it once more', async ({ page, request }) => { + // F3 ruling: the stream's ready must not read a queue the first read already holds. + await seed(request, 1); + const reads = reviewReads(page); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect(cards(page)).toHaveCount(2); + await expect(reviewRow(page)).toHaveText('Needs review2'); + // The page's first read and the change's re-read; the shell's count read at start, and the row follows the page. + expect(reads.filter(search => search === '?limit=40')).toHaveLength(2); + expect(reads.filter(search => search === '?limit=1')).toHaveLength(1); +}); + +test('a ready whose version cannot be read re-reads the queue once', async ({ page, request }) => { + // The stream's ready names no readable version (an older server, a proxy that rewrote it): the page reads again. + await seed(request, 1); + const reads = reviewReads(page); + // M1: `retry` keeps the browser from reconnecting the ended stream during the test, so exactly one re-read follows. + await page.route(url => url.pathname === '/prompt_manager/review/events', route => route.fulfill({ + status: 200, headers: { 'Content-Type': 'text/event-stream', 'Cache-Control': 'no-store' }, + body: 'retry: 3600000\nevent: ready\ndata: not json\n\n' })); + let reread = 0; + const second = page.waitForResponse(item => item.url().endsWith('/prompt_manager/review/items?limit=40') && ++reread === 2); + await open(page); + await second; + await expect(cards(page)).toHaveCount(1); + expect(reads.filter(search => search === '?limit=40')).toHaveLength(2); +}); + +const live = page => main(page).locator('.heading .review-live'); +const NOT_LIVE = 'Live updates are paused: changes made elsewhere appear once the connection returns.'; + +test('a stream the browser gives up on is opened again, and the page says it is not live meanwhile', async ({ page, request }) => { + // I3: a 503 (shutdown, or a proxy while the backend restarts) fails an EventSource for good. + await seed(request, 1); + let attempts = 0; + await page.route(url => url.pathname === '/prompt_manager/review/events', route => { + attempts += 1; + if (attempts === 1) return route.fulfill({ status: 503, contentType: 'application/json', body: '{"success":false}' }); + return route.fallback(); + }); + await open(page); + await expect(live(page)).toHaveText(NOT_LIVE); + await expect(live(page)).toBeVisible(); + // The page opens the stream again (after 3 s) and is live once it sends ready. + await expect.poll(async () => (await streams(request)).streams, { timeout: 15000 }).toBe(1); + await expect(live(page)).toBeHidden(); + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect(cards(page)).toHaveCount(2); + expect(attempts).toBe(2); +}); + +/** Make the page hidden or visible as a tab switch does. */ +async function setVisible(page, visible) { + await page.evaluate(shown => { + Object.defineProperty(document, 'hidden', { configurable: true, get: () => !shown }); + Object.defineProperty(document, 'visibilityState', { configurable: true, get: () => (shown ? 'visible' : 'hidden') }); + document.dispatchEvent(new Event('visibilitychange')); + }, visible); +} + +test('a hidden tab closes its stream and a visible one reads only what changed meanwhile', async ({ page, request }) => { + // I4: background tabs must not hold one of the origin's six connections. + await seed(request, 1); + const reads = reviewReads(page); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await setVisible(page, false); + // The server drops a closed stream at its next write: this monitor addition is one. + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect.poll(async () => (await streams(request)).streams).toBe(0); + await expect(cards(page)).toHaveCount(1); + expect(reads.filter(search => search === '?limit=40')).toHaveLength(1); + // Visible again: the reopened stream's ready names a newer version, so the page reads once. + const reread = page.waitForResponse(item => item.url().endsWith('/prompt_manager/review/items?limit=40')); + await setVisible(page, true); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await reread; + await expect(cards(page)).toHaveCount(2); + await expect(live(page)).toBeHidden(); + expect(reads.filter(search => search === '?limit=40')).toHaveLength(2); +}); + +test('a re-read refreshes a card whose media or target changed', async ({ page, request }) => { + // M6 and M10: a root came or went between reads; the card follows, and a known thumbnail is its media. + await seed(request, 2); + let rewrite = false; + await page.route(url => url.pathname === '/prompt_manager/review/items' && url.search === '?limit=40', async route => { + if (!rewrite) return route.fallback(); + const response = await route.fetch(); + const body = await response.json(); + body.items[0] = { ...body.items[0], thumbnail_url: `${body.items[0].url}&thumb=1` }; + body.items[1] = { ...body.items[1], url: null, thumbnail_url: null, target: null, unavailable: true }; + return route.fulfill({ response, json: body }); + }); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + const second = cards(page).nth(1); + await expect(second.getByRole('button', { name: 'Open review_0001.png', exact: true })).not.toHaveAttribute('aria-disabled', 'true'); + rewrite = true; + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect(cards(page)).toHaveCount(3); + await expect(cards(page).first().locator('img')).toHaveAttribute('src', /&thumb=1$/); + await expect(second.getByRole('button', { name: 'Open review_0001.png', exact: true })).toHaveAttribute('aria-disabled', 'true'); + await expect(second.locator('img')).toHaveCount(0); + await expect(second.locator('.image-card-actions button').last()).toHaveAttribute('aria-disabled', 'true'); + await expect(cards(page).first()).toHaveClass(/selected/); +}); + +test('leaving during the observe releases the capture it answers', async ({ page, request }) => { + // M4: the server made the capture; the page left before the answer came. + await seed(request, 1); + await open(page); + let held; + const sent = new Promise(resolve => { held = resolve; }); + await page.route(url => url.pathname === '/prompt_manager/image-annotations/observe', async route => { + const response = await route.fetch(); + held(() => route.fulfill({ response })); + }); + await apply(page).click(); + const answer = await sent; + expect((await streams(request)).captures).toBe(1); + await page.evaluate(() => { window.location.hash = '#/tags'; }); + await expect(main(page).getByRole('heading', { level: 1 })).toHaveText('Tags & categories'); + await answer(); + await expect.poll(async () => (await streams(request)).captures).toBe(0); +}); + +test('a live re-read keeps the loaded depth and the scroll position', async ({ page, request }) => { + // F3 ruling: a queue change re-reads as many items as Load more had shown, and the page does not jump. + await seed(request, 45); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + await main(page).getByRole('button', { name: 'Load more images', exact: true }).click(); + await expect(cards(page)).toHaveCount(45); + await cards(page).last().scrollIntoViewIfNeeded(); + const scrollTop = () => cards(page).first().evaluate(node => { + for (let el = node.parentElement; el; el = el.parentElement) { + const style = getComputedStyle(el); + if (/(auto|scroll)/.test(style.overflowY) && el.scrollHeight > el.clientHeight) return el.scrollTop; + } + return document.scrollingElement.scrollTop; + }); + const before = await scrollTop(); + expect(before).toBeGreaterThan(0); + const reads = reviewReads(page); + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect(main(page).locator('.notice h2')).toHaveText('46 images need a decision'); + await expect(cards(page)).toHaveCount(46); + expect(await scrollTop()).toBe(before); + expect(reads).toEqual(['?limit=40', 'more']); + await expect(main(page).locator('.infinite-continuation')).toHaveCount(0); +}); + +test('teardown releases captures listeners and requests', async ({ page, request }) => { + await seed(request, 1); + await open(page); + await expect.poll(async () => (await streams(request)).streams).toBe(1); + const inFlight = new Set(); + page.on('request', item => { if (item.url().includes('/prompt_manager/review/')) inFlight.add(item); }); + page.on('requestfinished', item => inFlight.delete(item)); + page.on('requestfailed', item => inFlight.delete(item)); + // A decision whose answer never comes: leaving the page must still release its capture. + let held; + const sent = new Promise(resolve => { held = resolve; }); + await page.route(url => /\/decision$/.test(url.pathname), route => held(route)); + await apply(page).click(); + const route = await sent; + expect((await streams(request)).captures).toBe(1); + await page.evaluate(() => { window.location.hash = '#/tags'; }); + await expect(main(page).getByRole('heading', { level: 1 })).toHaveText('Tags & categories'); + await expect.poll(async () => (await streams(request)).captures).toBe(0); + await route.abort().catch(() => {}); + // The server drops a closed stream at its next write: a monitor addition is one. + await expect(await request.post('/__test__/review/add', { data: { kind: 'monitor' } })).toBeOK(); + await expect.poll(async () => (await streams(request)).streams).toBe(0); + await expect.poll(() => inFlight.size).toBe(0); +}); diff --git a/tests/browser/workspace-saved-views.spec.mjs b/tests/browser/workspace-saved-views.spec.mjs index ce8f09fc..85a6ecab 100644 --- a/tests/browser/workspace-saved-views.spec.mjs +++ b/tests/browser/workspace-saved-views.spec.mjs @@ -372,7 +372,7 @@ test('Favorites is the first SAVED VIEWS row, with a real count that follows eve expect(await favoritesRow(page).evaluate(link => [...link.childNodes].map(node => node.nodeType === Node.TEXT_NODE ? `text:${node.textContent}` : `${node.localName}.${node.getAttribute('class')}`))) .toEqual(['svg.icon', 'text:Favorites', 'span.nav-count']); - expect(await savedNav(page).locator('a').evaluateAll(links => links.map(link => link.dataset.route))).toEqual(['favorites']); + expect(await savedNav(page).locator('a').evaluateAll(links => links.map(link => link.dataset.route))).toEqual(['favorites', 'review']); await star(page, '1:shared/same.png'); await expect(toast(page)).toHaveText('Added to favorites.'); diff --git a/tests/browser/workspace-system.spec.mjs b/tests/browser/workspace-system.spec.mjs index 0a5f7bba..ac1559fd 100644 --- a/tests/browser/workspace-system.spec.mjs +++ b/tests/browser/workspace-system.spec.mjs @@ -168,15 +168,17 @@ test('a failed thumbnail read is unavailable, never zero', async ({ page }) => { await expect(layout.locator('.metric').nth(2).locator('strong')).toHaveText('24'); }); -test('ambiguous row reads the pending count; its action waits for the review page', async ({ page }) => { +test('ambiguous row reads the pending count and Review metadata opens Needs review', async ({ page }) => { const layout = await openOverview(page); const row = layout.locator('.operation').filter({ hasText: 'ambiguous image links' }); await expect(row.locator('h3')).toHaveText('0 ambiguous image links'); await expect(row.locator('p')).toHaveText('Review metadata before assigning a positive / negative pair.'); const review = row.getByRole('button', { name: 'Review metadata', exact: true }); - await expect(review).toHaveAttribute('aria-disabled', 'true'); - await review.click({ force: true }); - await expect(page).toHaveURL(`${workspace}#/system/overview`); + await expect(review).not.toHaveAttribute('aria-disabled'); + await review.click(); + // The concept's go-review: the Needs review page. + await expect(page).toHaveURL(`${workspace}#/review`); + await expect(page.getByRole('heading', { name: 'Metadata review', exact: true })).toBeVisible(); }); test('View backups / Appearance / Manage sources switch sections', async ({ page }) => { diff --git a/tests/browser/workspace.spec.mjs b/tests/browser/workspace.spec.mjs index 79313a3f..d84b8607 100644 --- a/tests/browser/workspace.spec.mjs +++ b/tests/browser/workspace.spec.mjs @@ -22,6 +22,8 @@ const routes = [ ['system', 'System', null], // #249 D51: SAVED VIEWS' Favorites, Images filtered to favorites. ['favorites', 'Favorites', null], + // #249 PR F (ruling R5): SAVED VIEWS' Needs review, the concept's Metadata review page (its own heading). + ['review', 'Needs review', null, 'Metadata review'], // #249 Activity-1: the concept's Activity page in the tools-and-system group. ['activity', 'Activity', null], ]; @@ -77,10 +79,10 @@ test('workspace starts with real persisted statistics without mutating data', as }); test('all shell destinations deep-link, reload and expose working existing applications', async ({ page, request }) => { - for (const [route, label, legacy] of routes) { + for (const [route, label, legacy, title = label] of routes) { await page.goto(`${workspace}#/${route}`); const main = page.locator('#workspace-main'); - await expect(main.getByRole('heading', { level: 1, name: label, exact: true })).toBeVisible(); + await expect(main.getByRole('heading', { level: 1, name: title, exact: true })).toBeVisible(); await expect(sidebarLink(page, label)).toHaveAttribute('aria-current', 'page'); if (route === 'tags' || route === 'inspect') await expect(main.locator('.handoff')).toHaveCount(0); if (route === 'inspect') await expect(main.locator('#local-file')).toBeAttached(); @@ -89,7 +91,7 @@ test('all shell destinations deep-link, reload and expose working existing appli await expect(await request.get(legacy.split('#')[0])).toBeOK(); } await page.reload(); - await expect(main.getByRole('heading', { level: 1, name: label, exact: true })).toBeVisible(); + await expect(main.getByRole('heading', { level: 1, name: title, exact: true })).toBeVisible(); } // D29 and #249 E3: the legacy gallery and Library are reached from Overview's tool cards, not from their pages. await page.goto(`${workspace}#/overview`); @@ -309,9 +311,9 @@ test('overview reports loading, offers retry after failure and renders real empt test('small screens keep navigation and the theme accessible without horizontal page overflow', async ({ page }) => { await page.setViewportSize({ width: 375, height: 812 }); await page.goto(`${workspace}#/overview`); - for (const [, label] of routes) { + for (const [, label, , title = label] of routes) { await sidebarLink(page, label).click(); - await expect(page.locator('#workspace-main h1')).toHaveText(label); + await expect(page.locator('#workspace-main h1')).toHaveText(title); } await expect(page.getByRole('combobox', { name: 'Theme', exact: true })).toBeVisible(); expect(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth)).toBe(true); diff --git a/tests/browser/workspace_activity_fixture.py b/tests/browser/workspace_activity_fixture.py index 41dba88d..4d678c1b 100644 --- a/tests/browser/workspace_activity_fixture.py +++ b/tests/browser/workspace_activity_fixture.py @@ -150,6 +150,12 @@ def register_activity_commands(app, routes, api): scope = { "files": str(data.get("files", "new")), "output_folders": count(data.get("output_folders"), 2), + # A real scan's count of its files in the Needs review queue. + **( + {"review_items": count(data["review_items"])} + if "review_items" in data + else {} + ), } groups = SCAN_GROUP_NAMES try: diff --git a/tests/browser/workspace_review_fixture.py b/tests/browser/workspace_review_fixture.py new file mode 100644 index 00000000..595904d6 --- /dev/null +++ b/tests/browser/workspace_review_fixture.py @@ -0,0 +1,368 @@ +"""Owned, deterministic Needs review items for the workspace page (#249 PR F). + +Each file is a PNG whose workflow has two samplers with their own positive +encoders and one shared negative, so the resolver finds it ``ambiguous``. Its +loader node carries ComfyUI's ``Infinity`` stamp, which the legacy pages' +parser (the scan's fallback) cannot read, so nothing links the file and it is +a Needs review case (#284: only ambiguous, unlinked files are queued). It is +recorded through the producers' entry point (``record_review_file``). No path +or SQL comes from a request. + +``POST /__test__/review/reset`` also takes the special items F2 needs, queued +after the plain ones in this order: + +- ``conflict``: an item whose file was linked to another prompt ("Misty + forest path") after it was queued (the next scan or monitor pass would + remove the item); applying a sampler path then conflicts with that link. +- ``limited``: a graph of 17 samplers, each with its own prompt, which the + recorder stores ``limited`` (more than 16 candidates, so none are stored). + An A1111 block with two negative lines is not queued any more: it offers + no candidate to choose (#284). +- ``unavailable``: a file recorded under the ``temp`` directory while it was a + gallery root; the root is then removed again, so no current root lists it. + +``concept: true`` (F3, the concept comparison) names and sizes the plain +files as the concept's first three images (app.js ``images``) and gives +their first sampler the concept's first prompt (``alpine``: its positive and +its first negative, the one the concept's panel shows), so the panel's text +wraps as the concept's does. + +``POST /__test__/review/add {kind}`` adds one file while a page is open: +``monitor`` records a new ambiguous, unlinked output from a worker thread +through ``record_review_file`` (the monitor's own entry point; the monitor +never queues an output it linked itself), ``file`` only writes one (a +metadata scan records it). ``GET +/__test__/review/links`` answers each review file's prompt and pair links, and +``GET /__test__/review/streams`` the open change streams and captures. +""" + +from __future__ import annotations + +import json +import os +import threading +from pathlib import Path +import shutil +import time +from typing import TYPE_CHECKING + +from aiohttp import web +from PIL import Image +from PIL.PngImagePlugin import PngInfo + +if TYPE_CHECKING: + from py.api import PromptManagerAPI + +POSITIVES = ("Golden dunes at dusk", "Harbor lights at night") +NEGATIVE = "Soft negative, blur" +# The prompt the conflict item's file is linked to after it was queued. +FALLBACK = "Misty forest path" +SPECIAL = ("conflict", "limited", "unavailable") +# The concept's first three images (concept app.js `images`) and first prompt. +CONCEPT_IMAGES = ( + ("alpine_00001.png", (1536, 1152)), + ("dunes_00001.png", (1024, 1536)), + ("forest_00001.png", (1280, 1280)), +) +CONCEPT_POSITIVE = ( + "A remote alpine observatory at blue hour, warm light through glass, dramatic " + "mountain peaks, cinematic composition, atmospheric mist, fine detail" +) +CONCEPT_NEGATIVE = "blurry, low quality, low resolution" +# More candidates than an item stores (16): the ``limited`` item. +LIMITED_SAMPLERS = 17 +# More than one queue page (40), so Load more has a second page. +MAX_ITEMS = 45 +_TABLES = ( + "saved_views", + "image_favorites", + "media_review_decisions", + "media_review_items", + "media_image_tags", + "media_annotation_bytes", + "media_annotation_files", + "image_prompt_pairs", + "prompt_pair_tags", + "prompt_pairs", + "generated_images", + "prompt_tags", + "tags", + "prompt_library_state", + "prompts", +) + + +def _graph( + encoder_class: str = "CLIPTextEncode", + positives: tuple = POSITIVES, + negative: str = NEGATIVE, +) -> dict: + graph = { + # ``Infinity`` (as ComfyUI writes it): the legacy parser cannot read the + # graph, so the scan's fallback links nothing and the file is queued. + "1": { + "class_type": "CheckpointLoaderSimple", + "inputs": {"ckpt_name": "m"}, + "is_changed": [float("inf")], + }, + "2": {"class_type": "CLIPTextEncode", "inputs": {"text": negative}}, + } + for index, text in enumerate(positives): + encoder, sampler = str(10 + 2 * index), str(11 + 2 * index) + graph[encoder] = {"class_type": encoder_class, "inputs": {"text": text}} + graph[sampler] = { + "class_type": "KSampler", + "inputs": { + "model": ["1", 0], + "positive": [encoder, 0], + "negative": ["2", 0], + }, + } + return graph + + +def _png( + path: Path, + colour: tuple[int, int, int], + *, + graph: dict | None = None, + parameters: str | None = None, + size: tuple[int, int] = (96, 64), +) -> None: + info = PngInfo() + if parameters is not None: + info.add_text("parameters", parameters) + else: + info.add_text("prompt", json.dumps(graph or _graph())) + path.parent.mkdir(parents=True, exist_ok=True) + with Image.new("RGB", size, colour) as image: + image.save(path, pnginfo=info) + + +def _record(api: PromptManagerAPI, path: Path, expected: str) -> None: + outcome = api.record_review_file(str(path)) + if outcome != expected: + raise RuntimeError(f"{path.name} was {outcome}, not {expected}") + + +def _stamp(path: Path, age: float) -> None: + stamp = time.time() - age + os.utime(path, (stamp, stamp)) + + +class _Monitor: + """The monitor's recorder over this server's database (one per server).""" + + def __init__(self, api: PromptManagerAPI) -> None: + self.api = api + self.lock = threading.Lock() + + def new_output(self, path: Path) -> None: + """The monitor's Needs review pass for a new output, from a worker thread.""" + outcome = [] + with self.lock: + worker = threading.Thread( + target=lambda: outcome.append(self.api.record_review_file(str(path))) + ) + worker.start() + worker.join() + if outcome != ["recorded"]: + raise RuntimeError(f"{path.name} was {outcome}, not recorded") + + +def _monitored_output(api: PromptManagerAPI, monitor: _Monitor, path: Path) -> None: + _png(path, (150, 60, 40)) + monitor.new_output(path) + + +def _linked_after_queueing(api: PromptManagerAPI, path: Path) -> None: + from utils.hashing import generate_prompt_hash + + _png(path, (150, 60, 40)) + _record(api, path, "recorded") + prompt = api.db.get_prompt_by_hash(generate_prompt_hash(FALLBACK)) + prompt_id = ( + prompt["id"] + if prompt is not None + else api.db.save_prompt(FALLBACK, prompt_hash=generate_prompt_hash(FALLBACK)) + ) + api.db.link_image_to_prompt(prompt_id, str(path)) + + +def _unavailable(api: PromptManagerAPI, root: Path, path: Path) -> None: + from py.config import GalleryConfig + + roots = list(GalleryConfig.MONITORING_DIRECTORIES) + GalleryConfig.MONITORING_DIRECTORIES = [*roots, str(root / "temp")] + try: + _record(api, path, "recorded") + finally: + GalleryConfig.MONITORING_DIRECTORIES = roots + + +def seed_review( + api: PromptManagerAPI, + root: Path, + items: int, + special: tuple = (), + monitor: _Monitor | None = None, + concept: bool = False, +) -> dict: + """Replace this server's disposable rows and files with ``items`` queued files.""" + with api.db.model.get_connection() as conn: + for table in _TABLES: + conn.execute(f"DELETE FROM {table}") + api.close_annotation_captures() + for name in ("output", "input"): + shutil.rmtree(root / name) + (root / name).mkdir() + shutil.rmtree(root / "temp" / "review", ignore_errors=True) + output = root / "output" + files = [] + for number in range(items): + relative = f"review/review_{number:04}.png" + options = {} + if concept: + name, size = CONCEPT_IMAGES[number % len(CONCEPT_IMAGES)] + relative = f"review/{name}" if number < len(CONCEPT_IMAGES) else relative + graph = _graph( + positives=(CONCEPT_POSITIVE, POSITIVES[1]), negative=CONCEPT_NEGATIVE + ) + options = {"graph": graph, "size": size} + _png(output / relative, (40 + number * 4, 90, 140), **options) + _stamp(output / relative, 60 + number) + _record(api, output / relative, "recorded") + files.append({"path": relative, "filename": Path(relative).name}) + for kind in SPECIAL: + if kind not in special: + continue + relative = f"review/{kind}.png" + path = (root / "temp" if kind == "unavailable" else output) / relative + if kind == "conflict": + _linked_after_queueing(api, path) + elif kind == "limited": + many = tuple(f"{POSITIVES[0]} {n}" for n in range(LIMITED_SAMPLERS)) + _png(path, (90, 90, 90), graph=_graph(positives=many)) + _record(api, path, "limited") + else: + _png(path, (20, 140, 90)) + _unavailable(api, root, path) + files.append({"path": relative, "filename": path.name, "kind": kind}) + api.invalidate_gallery_cache() + return { + "files": files, + "positives": list(POSITIVES), + "negative": NEGATIVE, + "fallback": FALLBACK, + } + + +def add_review_file( + api: PromptManagerAPI, root: Path, kind: str, monitor: _Monitor +) -> dict: + """One more ambiguous output: through the monitor, or only written.""" + output = root / "output" + number = ( + len(list((output / "added").glob("*.png"))) + if (output / "added").exists() + else 0 + ) + relative = f"added/added_{number:04}.png" + if kind == "monitor": + _monitored_output(api, monitor, output / relative) + else: + _png(output / relative, (200, 200, 40)) + api.invalidate_gallery_cache() + return {"path": relative, "filename": Path(relative).name} + + +_LINKS = ( + "SELECT gi.filename, p.text AS prompt, pp.id AS pair_id, pos.text AS positive, " + "neg.text AS negative FROM generated_images gi " + "JOIN prompts p ON p.id = gi.prompt_id " + "LEFT JOIN image_prompt_pairs ipp ON ipp.image_id = gi.id " + "LEFT JOIN prompt_pairs pp ON pp.id = ipp.pair_id " + "LEFT JOIN prompts pos ON pos.id = pp.positive_prompt_id " + "LEFT JOIN prompts neg ON neg.id = pp.negative_prompt_id " + "ORDER BY gi.filename, gi.id, pp.id" +) + + +def review_links(api: PromptManagerAPI) -> list: + """Every file's prompt link and pair links, as plain rows.""" + conn = api.db.model.get_connection() + return [dict(row) for row in conn.execute(_LINKS)] + + +def register_review_commands( + routes: web.RouteTableDef, api: PromptManagerAPI, root: Path +) -> None: + """Register the review fixture's commands; no path or SQL comes from a request.""" + monitors = [] + + def monitor() -> _Monitor: + if not monitors: + monitors.append(_Monitor(api)) + return monitors[0] + + @routes.post("/__test__/review/reset") + async def reset(request: web.Request) -> web.Response: + data = await request.json() if request.can_read_body else {} + if ( + request.query + or not isinstance(data, dict) + or set(data) - {"items", "concept", *SPECIAL} + ): + raise web.HTTPBadRequest() + items = data.get("items", 3) + if isinstance(items, bool) or not isinstance(items, int): + raise web.HTTPBadRequest() + if not 0 <= items <= MAX_ITEMS: + raise web.HTTPBadRequest() + if any( + not isinstance(data.get(kind, False), bool) + for kind in (*SPECIAL, "concept") + ): + raise web.HTTPBadRequest() + special = tuple(kind for kind in SPECIAL if data.get(kind)) + manifest = await api._run_in_executor( + seed_review, + api, + root, + items, + special, + monitor(), + data.get("concept", False), + ) + return web.json_response({"success": True, **manifest}) + + @routes.post("/__test__/review/add") + async def add(request: web.Request) -> web.Response: + data = await request.json() if request.can_read_body else None + if request.query or not isinstance(data, dict) or set(data) != {"kind"}: + raise web.HTTPBadRequest() + if data["kind"] not in ("monitor", "file"): + raise web.HTTPBadRequest() + added = await api._run_in_executor( + add_review_file, api, root, data["kind"], monitor() + ) + return web.json_response({"success": True, **added}) + + @routes.get("/__test__/review/links") + async def links(request: web.Request) -> web.Response: + if request.query: + raise web.HTTPBadRequest() + return web.json_response( + {"links": await api._run_in_executor(review_links, api)} + ) + + @routes.get("/__test__/review/streams") + async def streams(request: web.Request) -> web.Response: + """Open review change streams and held captures (teardown checks).""" + if request.query: + raise web.HTTPBadRequest() + store = api._captures + captures = store.stats()["captures"] if store is not None else 0 + return web.json_response( + {"streams": api.review_stream_count(), "captures": captures} + ) diff --git a/tests/js/workspace-review-state.test.js b/tests/js/workspace-review-state.test.js new file mode 100644 index 00000000..e04da784 --- /dev/null +++ b/tests/js/workspace-review-state.test.js @@ -0,0 +1,788 @@ +// Run with: node --test tests/js/workspace-review-state.test.js +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { createApiClient } = require('../../web/js/api-client.js'); + +// Only an absent module is tolerated, so RED is an assertion failure, not a crash. +const source = import('../../web/js/workspace/review-state.mjs').catch(error => { + if (error.code !== 'ERR_MODULE_NOT_FOUND') throw error; + return {}; +}); +async function load() { + const module = await source; + assert.equal(typeof module.createReviewController, 'function', 'review-state.mjs is not implemented'); + return module; +} + +const EPOCH = '11111111-1111-4111-8111-111111111111'; +const SCOPE = 'c'.repeat(64); +const SHA = 'd'.repeat(64); +const PRINT = 'e'.repeat(64); +const HASH_A = 'a'.repeat(64); +const HASH_B = 'b'.repeat(64); +const HASH_C = 'f'.repeat(64); +const ITEMS = '/prompt_manager/review/items'; +const OBSERVE = '/prompt_manager/image-annotations/observe'; +const CAPTURE = '/prompt_manager/image-annotations/capture/'; + +const flush = () => new Promise(resolve => setImmediate(resolve)); +async function settle(times = 8) { for (let i = 0; i < times; i++) await flush(); } +function response(status, body) { + return { ok: status >= 200 && status < 300, status, statusText: `Status ${status}`, json: async () => body }; +} +const fail = (status, code, error = `failure ${code}`, extra = {}) => response(status, { success: false, code, error, ...extra }); + +function item(id, overrides = {}) { + const path = `dunes/${id}.png`; + return { + id, state: 'pending', revision: 0, created_at: `2026-10-22T10:00:0${id.length}+00:00`, filename: `${id}.png`, + target: { root_index: 0, root_scope: SCOPE, path }, unavailable: false, candidate_count: 3, limited: false, + url: `/prompt_manager/images/serve/${path}?root=0&root_scope=${SCOPE}`, is_video: false, ...overrides, + }; +} +const candidate = (hash, sampler, label, positive, negative, state = 'resolved') => ({ + hash, sampler_id: sampler, state, reason: '', label, + positive: positive === null ? null : { text: positive }, negative: negative === null ? null : { text: negative }, +}); +function detailOf(entry, overrides = {}) { + return { + success: true, library_epoch: EPOCH, ...entry, total: 2, sha256: SHA, dependency_fingerprint: PRINT, + candidates: [ + candidate(HASH_C, '9', 'No positive / Soft negative', null, 'blur', 'unresolved'), + candidate(HASH_A, '12', 'Dunes / Soft negative', 'golden dunes', 'blur'), + candidate(HASH_B, '28', 'Dunes / Harsh negative', 'golden dunes', 'noise'), + ], + ...overrides, + }; +} +const page = (items, overrides = {}) => ({ success: true, library_epoch: EPOCH, total: items.length, items, next_cursor: null, ...overrides }); +const observed = (token = 'capture-1') => ({ + success: true, library_epoch: EPOCH, target: item('one').target, filename: 'one.png', title: 'one.png', tags: [], tag_revision: 0, + capture: { token, sha256: SHA, preview_url: CAPTURE + token, size: 10, width: 8, height: 8, media_type: 'image', + expires_at: '2026-10-22T12:00:00+00:00', generate: { supported: true, reason: null } }, +}); + +/** Real createApiClient over a fetch whose answers come from routes['METHOD path'](call). */ +function harness(routes = {}) { + const calls = []; + const fetch = (url, init = {}) => { + const call = { url, method: init.method, body: init.body === undefined ? undefined : JSON.parse(init.body) }; + calls.push(call); + const path = url.startsWith(CAPTURE) ? CAPTURE : url.split('?')[0]; + const route = routes[`${call.method} ${path}`]; + return new Promise((resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(new DOMException('Aborted', 'AbortError'))); + Promise.resolve().then(() => (route ? route(call) : response(200, { success: true }))).then(resolve, reject); + }); + }; + return { + api: createApiClient(fetch), calls, + of: (method, prefix) => calls.filter(call => call.method === method && call.url.startsWith(prefix)), + }; +} +const QUEUE = [item('one'), item('two')]; +function routes(extra = {}) { + return { + [`GET ${ITEMS}`]: () => response(200, page(QUEUE)), + [`GET ${ITEMS}/one`]: () => response(200, detailOf(QUEUE[0])), + [`GET ${ITEMS}/two`]: () => response(200, detailOf(QUEUE[1], { candidates: [], limited: true, candidate_count: 0 })), + [`POST ${OBSERVE}`]: () => response(200, observed()), + [`POST ${ITEMS}/one/decision`]: call => response(200, { success: true, library_epoch: EPOCH, total: 1, + item: { ...QUEUE[0], state: call.body.choice === 'unresolved' ? 'unresolved' : 'selected', revision: 1 } }), + ...extra, + }; +} +async function loaded(extra) { + const { createReviewController } = await load(); + const h = harness(routes(extra)); + const c = createReviewController({ api: h.api }); + await c.load(); + await settle(); + return { c, h }; +} + +test('notice titles for 0, 1 and 3', async () => { + const { noticeTitle } = await load(); + assert.equal(noticeTitle(0), 'No images need a decision'); + assert.equal(noticeTitle(1), '1 image needs a decision'); + assert.equal(noticeTitle(3), '3 images need a decision'); + assert.equal(noticeTitle(1234), '1,234 images need a decision'); +}); + +test('loads pending items and the exact count', async () => { + const { c, h } = await loaded(); + assert.equal(c.state.status, 'ready'); + assert.equal(c.state.total, 2); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two']); + const [list] = h.of('GET', ITEMS); + assert.equal(list.url, `${ITEMS}?limit=40`); + // The first item is selected, as the concept selects its first queue image. + assert.equal(c.state.selectedId, 'one'); + assert.ok(Object.isFrozen(c.state)); +}); + +test('selecting a card loads detail and option labels, first workable preselected, unusable disabled', async () => { + const { c } = await loaded(); + assert.equal(c.state.detailStatus, 'ready'); + assert.deepEqual(c.state.options.map(option => [option.label, option.disabled]), [ + ['Sampler 9 · No positive / Soft negative', true], + ['Sampler 12 · Dunes / Soft negative', false], + ['Sampler 28 · Dunes / Harsh negative', false], + ['Leave unresolved', false], + ]); + assert.equal(c.state.options.at(-1).value, 'unresolved'); + assert.equal(c.state.choice, HASH_A); + assert.deepEqual(c.state.prompts, { positive: { status: 'resolved', text: 'golden dunes' }, negative: { status: 'resolved', text: 'blur' } }); + c.choose(HASH_C); + assert.equal(c.state.choice, HASH_A, 'a disabled option cannot be chosen'); + c.choose(HASH_B); + assert.equal(c.state.prompts.negative.text, 'noise'); + c.choose('unresolved'); + assert.equal(c.state.prompts.positive.status, 'ambiguous'); + await c.select('two'); + await settle(); + assert.equal(c.state.selectedId, 'two'); + assert.deepEqual(c.state.options.map(option => option.label), ['Leave unresolved']); + assert.equal(c.state.choice, 'unresolved'); + assert.match(c.state.warning, /too many sampler paths/i); +}); + +test('apply sends the selected candidate hash or unresolved with a fresh capture', async () => { + const { c, h } = await loaded(); + c.choose(HASH_B); + const done = await c.apply(); + assert.equal(done, true); + const [observe] = h.of('POST', OBSERVE); + assert.deepEqual(observe.body, { target: QUEUE[0].target }); + const [decision] = h.of('POST', `${ITEMS}/one/decision`); + assert.deepEqual(decision.body, { expected_epoch: EPOCH, expected_revision: 0, capture_token: 'capture-1', sha256: SHA, + dependency_fingerprint: PRINT, choice: HASH_B }); + assert.equal(c.state.phase, 'done'); + assert.equal(c.state.total, 1); + assert.equal(c.state.toast, 'Association accepted.'); + // The server releases a used capture itself. + assert.equal(h.of('DELETE', CAPTURE).length, 0); + + const again = await loaded(); + again.c.choose('unresolved'); + assert.equal(await again.c.apply(), true); + const [unresolved] = again.h.of('POST', `${ITEMS}/one/decision`); + assert.equal(unresolved.body.choice, 'unresolved'); + assert.equal(unresolved.body.capture_token, 'capture-1'); + assert.equal(again.c.state.toast, 'Left unresolved.'); +}); + +test('an unavailable file can only be left unresolved, without a capture', async () => { + const gone = item('one', { target: null, unavailable: true, url: null }); + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page([gone])), + [`GET ${ITEMS}/one`]: () => response(200, detailOf(gone)), + }); + assert.ok(c.state.options.slice(0, -1).every(option => option.disabled)); + assert.equal(c.state.choice, 'unresolved'); + assert.match(c.state.warning, /not under a current image source/i); + assert.equal(await c.apply(), true); + assert.equal(h.of('POST', OBSERVE).length, 0); + const [decision] = h.of('POST', `${ITEMS}/one/decision`); + assert.equal(decision.body.capture_token, null); + assert.equal(decision.body.sha256, SHA); +}); + +test('association conflict keeps the selection and shows the inline warning', async () => { + const message = 'This image is already linked to another prompt or negative, so this sampler path cannot be attached safely.'; + const { c, h } = await loaded({ [`POST ${ITEMS}/one/decision`]: () => fail(409, 'association-conflict', message) }); + assert.equal(await c.apply(), false); + assert.equal(c.state.selectedId, 'one'); + assert.equal(c.state.choice, HASH_A); + assert.equal(c.state.phase, 'idle'); + assert.equal(c.state.warning, message); + // The refused decision's capture is released. + assert.equal(h.of('DELETE', CAPTURE).length, 1); + assert.equal(h.of('GET', `${ITEMS}?`).length, 1, 'no reload'); +}); + +test('storage-changed requires reload', async () => { + let lists = 0; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => { lists += 1; return response(200, page(QUEUE)); }, + [`POST ${ITEMS}/one/decision`]: () => fail(409, 'storage-changed', 'Prompt storage was restored; reload the review queue.'), + }); + assert.equal(await c.apply(), false); + await settle(); + assert.equal(lists, 2, 'the queue is read again under the new epoch'); + assert.equal(c.state.warning, 'Prompt storage was restored; reload the review queue.'); + assert.equal(c.state.canRetry, false); + assert.equal(h.of('DELETE', CAPTURE).length, 1); +}); + +test('lost response keeps the selection and retry resends once', async () => { + let attempts = 0; + const { c, h } = await loaded({ + [`POST ${ITEMS}/one/decision`]: call => { + attempts += 1; + if (attempts === 1) return Promise.reject(new TypeError('Failed to fetch')); + return response(200, { success: true, library_epoch: EPOCH, total: 1, item: { ...QUEUE[0], state: 'selected', revision: 1 } }); + }, + }); + assert.equal(await c.apply(), false); + assert.equal(c.state.selectedId, 'one'); + assert.equal(c.state.canRetry, true); + assert.match(c.state.warning, /may not have been saved/i); + assert.equal(h.of('DELETE', CAPTURE).length, 0, 'the capture is kept for the retry'); + const [first] = h.of('POST', `${ITEMS}/one/decision`); + assert.equal(await c.retry(), true); + const sent = h.of('POST', `${ITEMS}/one/decision`); + assert.equal(sent.length, 2); + assert.deepEqual(sent[1].body, first.body, 'the identical decision is resent'); + assert.equal(h.of('POST', OBSERVE).length, 1, 'no second capture'); + assert.equal(await c.retry(), false, 'nothing left to resend'); + assert.equal(h.of('POST', `${ITEMS}/one/decision`).length, 2); +}); + +test('a retry answered by review-conflict reloads the queue', async () => { + let attempts = 0, lists = 0; + const { c } = await loaded({ + [`GET ${ITEMS}`]: () => { lists += 1; return response(200, page(lists === 1 ? QUEUE : [QUEUE[1]])); }, + [`POST ${ITEMS}/one/decision`]: () => { + attempts += 1; + if (attempts === 1) return Promise.reject(new TypeError('Failed to fetch')); + return fail(409, 'review-conflict', 'This item was decided or changed since it was opened. Reload the queue.'); + }, + }); + await c.apply(); + assert.equal(await c.retry(), false); + await settle(); + assert.equal(lists, 2); + assert.equal(c.state.selectedId, 'two'); +}); + +test('a review-conflict on a still pending item reads its detail again', async () => { + let details = 0, decisions = 0; + const { c, h } = await loaded({ + [`GET ${ITEMS}/one`]: () => { details += 1; return response(200, detailOf({ ...QUEUE[0], revision: details - 1 })); }, + [`POST ${ITEMS}/one/decision`]: () => { + decisions += 1; + return decisions === 1 ? fail(409, 'review-conflict', 'This item was decided or changed since it was opened. Reload the queue.') + : response(200, { success: true, library_epoch: EPOCH, total: 1, item: { ...QUEUE[0], state: 'selected', revision: 2 } }); + }, + }); + assert.equal(await c.apply(), false); + await settle(); + assert.equal(details, 2, 'the item is read again'); + assert.equal(c.state.detail.revision, 1); + assert.equal(c.state.warning, 'This item was decided or changed since it was opened. Reload the queue.'); + assert.equal(await c.apply(), true); + assert.equal(h.of('POST', `${ITEMS}/one/decision`).at(-1).body.expected_revision, 1); +}); + +test('leave for later writes nothing', async () => { + const { c, h } = await loaded(); + c.leave(); + assert.equal(c.state.phase, 'done'); + assert.equal(c.state.toast, null); + assert.equal(h.of('POST', '/').length, 0); + assert.equal(h.of('DELETE', '/').length, 0); +}); + +test('load more pages with the cursor and keeps the selection', async () => { + const more = [item('three')]; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: call => response(200, call.url.includes('cursor=') + ? page(more, { total: 3 }) : page(QUEUE, { total: 3, next_cursor: 'abc_-1' })), + }); + assert.equal(c.state.nextCursor, 'abc_-1'); + await c.loadMore(); + assert.equal(h.of('GET', ITEMS).at(-1).url, `${ITEMS}?limit=40&cursor=abc_-1`); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two', 'three']); + assert.equal(c.state.nextCursor, null); + assert.equal(c.state.selectedId, 'one'); +}); + +test('a failed queue read is an error state and load retries', async () => { + let lists = 0; + const { c } = await loaded({ [`GET ${ITEMS}`]: () => { lists += 1; return lists === 1 ? fail(500, 'review-failed') : response(200, page(QUEUE)); } }); + assert.equal(c.state.status, 'error'); + await c.load(); + await settle(); + assert.equal(c.state.status, 'ready'); +}); + +test('tag save toast carries the review suffix and the item stays pending', async () => { + const { c, h } = await loaded({ + ['GET /prompt_manager/autotag/models']: () => response(200, { success: true, models: {}, default_prompt: 'x', + wd14_general_threshold: 0.35, wd14_character_threshold: 0.85, suggestion_limit: 10 }), + ['PUT /prompt_manager/image-annotations/tags']: call => response(200, { success: true, library_epoch: EPOCH, sha256: SHA, + tags: call.body.tags, tag_revision: 1, replayed: false }), + }); + const { createImageTaggingController } = await import('../../web/js/workspace/image-tagging-state.mjs'); + const before = h.calls.filter(call => call.url.startsWith(ITEMS)).length; + // The Tag dialog opened from Needs review is the shared controller with the review source. + const tagging = createImageTaggingController({ api: h.api, target: QUEUE[0].target, source: 'review' }); + assert.equal(tagging.state.showScopeNote, true); + await tagging.open(); + await settle(); + tagging.setManual('dunes'); + await tagging.save(); + await settle(); + assert.equal(tagging.state.toast, '1 tags saved for one.png. Prompt association still awaits review.'); + tagging.dispose(); + assert.equal(h.calls.filter(call => call.url.startsWith(ITEMS)).length, before, 'a tag save neither decides nor re-reads the queue'); + assert.deepEqual(c.state.items.map(entry => [entry.id, entry.state]), [['one', 'pending'], ['two', 'pending']]); + assert.equal(c.state.phase, 'idle'); +}); + +test('dispose releases a kept capture and stops reads', async () => { + const { c, h } = await loaded({ [`POST ${ITEMS}/one/decision`]: () => Promise.reject(new TypeError('Failed to fetch')) }); + await c.apply(); + c.dispose(); + await settle(); + const released = h.of('DELETE', CAPTURE); + assert.equal(released.length, 1); + assert.equal(released[0].url, CAPTURE + 'capture-1'); + assert.equal(await c.retry(), false); +}); + +// F2: an open page re-reads its queue when the server says it changed (the review change stream). +function refreshOf(c) { + assert.equal(typeof c.refresh, 'function', 'refresh is not implemented'); + return c.refresh(); +} +test('refresh re-reads the queue quietly and keeps the selection and choice', async () => { + let queue = QUEUE; + const { c, h } = await loaded({ [`GET ${ITEMS}`]: () => response(200, page(queue)) }); + c.choose(HASH_B); + const statuses = []; + c.subscribe(state => statuses.push(state.status)); + queue = [...QUEUE, item('three')]; + await refreshOf(c); + await settle(); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two', 'three']); + assert.equal(c.state.total, 3); + assert.equal(c.state.selectedId, 'one'); + assert.equal(c.state.choice, HASH_B); + assert.equal(c.state.warning, ''); + assert.ok(statuses.every(status => status === 'ready'), `no loading state: ${statuses}`); + assert.equal(h.of('GET', `${ITEMS}/one`).length, 1, 'the open item is not read again'); +}); + +test('refreshes asked for during a refresh collapse into one more read', async () => { + let release; + const held = new Promise(resolve => { release = resolve; }); + let lists = 0; + const { c, h } = await loaded({ [`GET ${ITEMS}`]: async () => { + lists += 1; + if (lists === 2) await held; + return response(200, page(QUEUE)); + } }); + const first = refreshOf(c); + await settle(); + refreshOf(c); + refreshOf(c); + release(); + await first; + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 3, 'the load, the refresh and one collapsed re-read'); + assert.equal(c.state.status, 'ready'); +}); + +test('a refresh after the open item was decided elsewhere opens the next one and says so', async () => { + let queue = QUEUE; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page(queue)), + [`GET ${ITEMS}/three`]: () => response(200, detailOf(item('three'))), + }); + queue = [item('three')]; + await refreshOf(c); + await settle(); + assert.equal(c.state.selectedId, 'three'); + assert.equal(c.state.detailStatus, 'ready'); + assert.equal(c.state.warning, 'one.png was decided elsewhere, so the next image is open.'); + assert.equal(h.of('GET', `${ITEMS}/three`).length, 1); +}); + +test('a refresh to an empty queue clears the selection', async () => { + let queue = QUEUE; + const { c } = await loaded({ [`GET ${ITEMS}`]: () => response(200, page(queue)) }); + queue = []; + await refreshOf(c); + await settle(); + assert.equal(c.state.total, 0); + assert.deepEqual(c.state.items, []); + assert.equal(c.state.selectedId, null); + assert.equal(c.state.status, 'ready'); +}); + +test('a failed refresh keeps the queue and says so', async () => { + let lists = 0; + const { c } = await loaded({ [`GET ${ITEMS}`]: () => { lists += 1; return lists === 1 ? response(200, page(QUEUE)) : fail(500, 'review-failed'); } }); + await refreshOf(c); + await settle(); + assert.equal(c.state.status, 'ready'); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two']); + assert.equal(c.state.warning, 'The review queue could not be read again. Reload the page to see the latest images.'); +}); + +test('dispose during a decision releases its capture', async () => { + const { c, h } = await loaded({ [`POST ${ITEMS}/one/decision`]: () => new Promise(() => {}) }); + const applying = c.apply(); + await settle(); + assert.equal(c.state.phase, 'applying'); + c.dispose(); + assert.equal(await applying, false); + await settle(); + assert.deepEqual(h.of('DELETE', CAPTURE).map(call => call.url), [CAPTURE + 'capture-1']); + await refreshOf(c); + assert.equal(h.of('GET', `${ITEMS}?`).length, 1, 'a disposed controller reads nothing'); +}); + +test('a change heard during the first read re-reads once after it, and a failed first read stays an error', async () => { + const { createReviewController } = await load(); + let release; + let lists = 0; + const h = harness(routes({ [`GET ${ITEMS}`]: async () => { + lists += 1; + if (lists === 1) await new Promise(resolve => { release = resolve; }); + return response(200, page(QUEUE)); + } })); + const c = createReviewController({ api: h.api }); + const first = c.load(); + await settle(); + refreshOf(c); + assert.equal(lists, 1, 'the first read is not interrupted'); + release(); + await first; + await settle(); + assert.equal(lists, 2, 'one re-read after the first read'); + assert.equal(c.state.status, 'ready'); + + const failing = harness(routes({ [`GET ${ITEMS}`]: () => fail(500, 'review-failed') })); + const broken = createReviewController({ api: failing.api }); + const reading = broken.load(); + refreshOf(broken); + await reading; + await settle(); + assert.equal(broken.state.status, 'error'); + assert.equal(failing.of('GET', `${ITEMS}?`).length, 1, 'Retry is the way back, not a re-read'); +}); + +// F3 rulings: a live re-read keeps the loaded depth, and the stream's `ready` reads nothing when the first read is current. + +/** A keyset pager over `source()`: pages of 40 with an index cursor, each answering `version()`. */ +function pager(source, version = () => 0, { onPage } = {}) { + return call => { + const query = new URL(call.url, 'http://x').searchParams; + const cursor = query.get('cursor'); + const all = source(); + const from = cursor ? Number(cursor.slice(1)) : 0; + const answer = onPage?.(cursor); + if (answer) return answer; + const items = all.slice(from, from + 40); + const next = from + 40 < all.length ? `c${from + 40}` : null; + return response(200, page(items, { total: all.length, next_cursor: next, review_revision: version() })); + }; +} +const many = (count, prefix = 'i') => Array.from({ length: count }, (_, index) => item(`${prefix}${String(index).padStart(3, '0')}`)); +function syncOf(c, version) { + assert.equal(typeof c.sync, 'function', 'sync is not implemented'); + return c.sync(version); +} +const version = revision => ({ library_epoch: EPOCH, review_revision: revision }); + +test('a refresh keeps the loaded depth: it re-reads as many items as were loaded', async () => { + let queue = many(100); + const { c, h } = await loaded({ [`GET ${ITEMS}`]: pager(() => queue) }); + await c.loadMore(); + assert.equal(c.state.items.length, 80); + const before = h.of('GET', `${ITEMS}?`).length; + queue = [...queue, item('late')]; + await refreshOf(c); + await settle(); + assert.equal(c.state.items.length, 80, 'the Load more page is kept'); + assert.deepEqual(c.state.items.map(entry => entry.id), queue.slice(0, 80).map(entry => entry.id)); + assert.equal(c.state.total, 101); + assert.equal(c.state.nextCursor, 'c80', 'Load more continues after the kept depth'); + assert.deepEqual(h.of('GET', `${ITEMS}?`).slice(before).map(call => call.url), + [`${ITEMS}?limit=40`, `${ITEMS}?limit=40&cursor=c40`]); + assert.equal(c.state.selectedId, 'i000'); +}); + +test('a refresh after decisions elsewhere fills the loaded depth from what is left', async () => { + let queue = many(85); + const { c } = await loaded({ [`GET ${ITEMS}`]: pager(() => queue) }); + await c.loadMore(); + await c.loadMore(); + assert.equal(c.state.items.length, 85); + queue = queue.filter((_, index) => index % 10 !== 5); + await refreshOf(c); + await settle(); + assert.equal(c.state.items.length, queue.length); + assert.equal(c.state.nextCursor, null); +}); + +test('a deep refresh whose next page meets a restore reads again from the first page', async () => { + let restored = false; + let queue = many(60); + const { c, h } = await loaded({ [`GET ${ITEMS}`]: pager(() => queue, () => 0, { + onPage: cursor => { + if (cursor && restored) { restored = false; return fail(409, 'storage-changed', 'The library was restored.'); } + return null; + }, + }) }); + await c.loadMore(); + const before = h.of('GET', `${ITEMS}?`).length; + restored = true; + queue = many(60, 'r'); + await refreshOf(c); + await settle(); + assert.deepEqual(h.of('GET', `${ITEMS}?`).slice(before).map(call => call.url), + [`${ITEMS}?limit=40`, `${ITEMS}?limit=40&cursor=c40`, `${ITEMS}?limit=40`, `${ITEMS}?limit=40&cursor=c40`]); + assert.equal(c.state.items.length, 60); + assert.equal(c.state.items[0].id, 'r000'); + assert.notEqual(c.state.warning, 'The review queue could not be read again. Reload the page to see the latest images.', + 'a restore met mid-read is not a failed re-read'); + assert.equal(c.state.status, 'ready'); +}); + +test('ready naming the version the first read answered reads nothing more', async () => { + const { c, h } = await loaded({ [`GET ${ITEMS}`]: pager(() => QUEUE, () => 4) }); + await syncOf(c, version(4)); + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 1, 'one list read per visit'); + assert.equal(h.of('GET', `${ITEMS}/one`).length, 1); +}); + +test('ready naming a newer version, another epoch or no version re-reads once', async () => { + const { c, h } = await loaded({ [`GET ${ITEMS}`]: pager(() => QUEUE, () => 4) }); + await syncOf(c, version(5)); + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 2); + await syncOf(c, { library_epoch: '22222222-2222-4222-8222-222222222222', review_revision: 4 }); + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 3); + await syncOf(c, {}); + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 4, 'an unknown version is read, as before versions existed'); +}); + +test('ready heard during the first read re-reads only when that read is older', async () => { + const { createReviewController } = await load(); + for (const [readRevision, readyRevision, reads] of [[4, 4, 1], [5, 4, 1], [4, 5, 2]]) { + let release; + let lists = 0; + const answer = pager(() => QUEUE, () => (lists === 1 ? readRevision : readyRevision)); + const h = harness(routes({ [`GET ${ITEMS}`]: async call => { + lists += 1; + if (lists === 1) await new Promise(resolve => { release = resolve; }); + return answer(call); + } })); + const c = createReviewController({ api: h.api }); + const first = c.load(); + await settle(); + syncOf(c, version(readyRevision)); + release(); + await first; + await settle(); + assert.equal(lists, reads, `first read r${readRevision}, ready r${readyRevision}`); + c.dispose(); + } +}); + +test('a change heard during the first read still re-reads after a current ready', async () => { + const { createReviewController } = await load(); + let release; + let lists = 0; + const h = harness(routes({ [`GET ${ITEMS}`]: async call => { + lists += 1; + if (lists === 1) await new Promise(resolve => { release = resolve; }); + return pager(() => QUEUE, () => 4)(call); + } })); + const c = createReviewController({ api: h.api }); + const first = c.load(); + await settle(); + syncOf(c, version(4)); + refreshOf(c); + release(); + await first; + await settle(); + assert.equal(lists, 2); +}); + +// Final review round (I1, I2, M3–M5, M7). +test('I1: a change heard during a refused Apply is re-read once the phase is idle again', async () => { + let queue = [item('one')], releaseDecision; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page(queue, { review_revision: queue.length })), + [`POST ${ITEMS}/one/decision`]: () => new Promise(resolve => { + releaseDecision = () => resolve(fail(409, 'association-conflict', 'conflict')); + }), + }); + const applying = c.apply(); + await settle(); + assert.equal(c.state.phase, 'applying'); + queue = [item('one'), item('two')]; // the monitor records an item while the decision is in flight + c.refresh(); + await settle(); + assert.equal(h.of('GET', `${ITEMS}?`).length, 1, 'no read while the decision is in flight'); + releaseDecision(); + await applying; + await settle(); + assert.equal(c.state.phase, 'idle'); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two']); + assert.equal(c.state.total, 2); + assert.equal(c.state.warning, 'conflict', 'the refusal stays the warning'); + assert.equal(h.of('GET', `${ITEMS}?`).length, 2, 'exactly one owed re-read'); +}); + +test('I1: a change heard during an Apply whose observe is refused is re-read too', async () => { + let queue = [item('one')], releaseObserve; + const { c } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page(queue)), + [`POST ${OBSERVE}`]: () => new Promise(resolve => { + releaseObserve = () => resolve(fail(409, 'capture-capacity', 'busy')); + }), + }); + const applying = c.apply(); + await settle(); + queue = [item('one'), item('two')]; + c.refresh(); + releaseObserve(); + await applying; + await settle(); + assert.deepEqual(c.state.items.map(entry => entry.id), ['one', 'two']); +}); + +const forty = Array.from({ length: 41 }, (_, i) => item(`i${String(i).padStart(2, '0')}`)); +function deepRoutes(state) { + return { + [`GET ${ITEMS}`]: call => { + const cursor = new URL(call.url, 'http://x').searchParams.get('cursor'); + if (cursor) { + if (state.moreGate) return state.moreGate.then(() => state.more ?? response(200, page([forty[40]], { total: 41 }))); + return state.more ?? response(200, page([forty[40]], { total: 41 })); + } + const body = response(200, page(state.first, { total: state.first.length + 1, review_revision: state.revision ?? 1, next_cursor: 'cur' })); + return state.gate ? state.gate.then(() => state.fail ?? body) : body; + }, + [`GET ${ITEMS}/i00`]: () => response(200, detailOf(forty[0])), + }; +} + +test('I2: Load more during a re-read waits for it and continues from the re-read queue', async () => { + const state = { first: forty.slice(0, 40) }; + const { c } = await loaded(deepRoutes(state)); + // Another tab decides i05; the re-read is slow and the user clicks Load more meanwhile. + let open; + state.first = forty.slice(0, 40).filter(entry => entry.id !== 'i05'); + state.gate = new Promise(resolve => { open = resolve; }); + const refreshing = c.refresh(); + await settle(); + const more = c.loadMore(); + await settle(); + assert.equal(c.state.more, 'loading', 'the click shows as loading while the re-read runs'); + open(); + await Promise.all([refreshing, more]); + await settle(); + const ids = c.state.items.map(entry => entry.id); + assert.ok(!ids.includes('i05'), 'the re-read was applied'); + assert.equal(ids.at(-1), 'i40', 'Load more appended the next page after the re-read'); + assert.equal(c.state.more, 'idle'); +}); + +test('I2: a re-read started during Load more takes over the click and reads one page deeper', async () => { + const state = { first: forty.slice(0, 40) }; + const { c, h } = await loaded(deepRoutes(state)); + let openMore; + state.moreGate = new Promise(resolve => { openMore = resolve; }); + const more = c.loadMore(); + await settle(); + state.moreGate = null; + state.first = forty.slice(0, 40).filter(entry => entry.id !== 'i05'); + await c.refresh(); + openMore(); + await more; + await settle(); + const ids = c.state.items.map(entry => entry.id); + assert.ok(!ids.includes('i05')); + assert.equal(ids.at(-1), 'i40', 'the aborted Load more page is part of the re-read'); + assert.equal(c.state.more, 'idle'); + assert.equal(h.of('GET', `${ITEMS}?`).length, 4, 'load, the aborted more, and the re-read of two pages'); +}); + +test('M7: a failed re-read that took over Load more clears its loading state', async () => { + const state = { first: forty.slice(0, 40) }; + const { c } = await loaded(deepRoutes(state)); + state.moreGate = new Promise(() => {}); + c.loadMore(); + await settle(); + assert.equal(c.state.more, 'loading'); + state.moreGate = null; + state.gate = Promise.resolve(); + state.fail = fail(500, 'review-failed'); + await c.refresh(); + await settle(); + assert.equal(c.state.more, 'idle', 'Load more can be clicked again'); + assert.equal(c.state.warning, COPY_REFRESH_FAILED); +}); +const COPY_REFRESH_FAILED = 'The review queue could not be read again. Reload the page to see the latest images.'; + +test('M3: Apply fences on the epoch of the detail it decides', async () => { + const OTHER = '22222222-2222-4222-8222-222222222222'; + const { c, h } = await loaded({ [`POST ${OBSERVE}`]: () => response(200, { ...observed(), library_epoch: OTHER }) }); + assert.equal(await c.apply(), false); + await settle(); + assert.equal(h.of('POST', `${ITEMS}/one/decision`).length, 0, 'a restored storage sends no decision'); + assert.deepEqual(h.of('DELETE', CAPTURE).map(call => call.url), [CAPTURE + 'capture-1']); + assert.equal(h.of('GET', `${ITEMS}?`).length, 2, 'the queue is read again'); + assert.equal(h.of('GET', `${ITEMS}/one`).length, 2, 'the open item is read again under the new storage'); + assert.equal(c.state.phase, 'idle'); +}); + +test('M3: the decision carries the detail epoch', async () => { + const { c, h } = await loaded(); + assert.equal(await c.apply(), true); + assert.equal(h.of('POST', `${ITEMS}/one/decision`)[0].body.expected_epoch, EPOCH); +}); + +test('M3: a quiet re-read under another epoch reads the open item again', async () => { + const OTHER = '22222222-2222-4222-8222-222222222222'; + let epoch = EPOCH; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page(QUEUE, { library_epoch: epoch })), + [`GET ${ITEMS}/one`]: () => response(200, detailOf(QUEUE[0], { library_epoch: epoch })), + }); + epoch = OTHER; + await c.refresh(); + await settle(); + assert.equal(h.of('GET', `${ITEMS}/one`).length, 2); + assert.equal(c.state.detail.library_epoch, OTHER); +}); + +test('M4: leaving during the observe releases the capture it then answers', async () => { + let answer; + const { c, h } = await loaded({ [`POST ${OBSERVE}`]: () => new Promise(resolve => { answer = resolve; }) }); + const applying = c.apply(); + await settle(); + c.dispose(); + answer(response(200, observed('late'))); + assert.equal(await applying, false); + await settle(); + assert.deepEqual(h.of('DELETE', CAPTURE).map(call => call.url), [CAPTURE + 'late']); + assert.equal(h.of('POST', `${ITEMS}/one/decision`).length, 0); +}); + +test('M5: an unanswered decision that landed finishes as recorded, not as decided elsewhere', async () => { + let queue = QUEUE; + const { c, h } = await loaded({ + [`GET ${ITEMS}`]: () => response(200, page(queue)), + [`POST ${ITEMS}/one/decision`]: () => Promise.reject(new TypeError('Failed to fetch')), + }); + await c.apply(); + assert.equal(c.state.canRetry, true); + queue = [QUEUE[1]]; // the server did record it: its change arrives + await c.refresh(); + await settle(); + assert.equal(c.state.phase, 'done'); + assert.equal(c.state.toast, 'Association accepted.'); + assert.doesNotMatch(c.state.warning, /decided elsewhere/); + assert.equal(h.of('DELETE', CAPTURE).length, 0, 'the server used the capture'); +}); diff --git a/tests/js/workspace-state.test.js b/tests/js/workspace-state.test.js index c05714bb..a6779df2 100644 --- a/tests/js/workspace-state.test.js +++ b/tests/js/workspace-state.test.js @@ -4,7 +4,7 @@ const source = import('../../web/js/workspace/state.mjs'); test('workspace routes resolve only supported deep links', async () => { const { resolveRoute } = await source; - for (const route of ['overview', 'images', 'library', 'tags', 'inspect', 'system', 'activity']) { + for (const route of ['overview', 'images', 'library', 'tags', 'inspect', 'system', 'activity', 'review']) { assert.equal(resolveRoute(`#/${route}`).id, route); } for (const hash of ['', '#', '#/unknown', '#/images/other', '#/