Repository navigation
Showcase the public library and fix catalog presentation and desiderata credits - #472
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No additional issue has been established that would prevent merging after normal checks. Pre-merge checks |
|
There was a problem hiding this comment.
ℹ️ No critical issues. Two documentation rough edges are noted inline.
Reviewed changes
This run reviewed the desiderata credit fix and the new public-showcase documentation at a7c8ce6.
- Desiderata credits: the
autoresubqueries inwanted()anddonationPrefill()are now limited toprincipale/co-autoreroles, ordered with the principal first and then byordine_credito. Both expressions are identical, so the two surfaces still agree. The role filter matches the convention core already uses inBookRepository,NotificationServiceandLoanRepository, andphp -lpasses. - Root README: adds a "Live demo" section and replaces the
docs/catalog.pnggallery entry with the newdocs/demo/*.jpgscreenshots.docs/catalog.pngstays in the repo, which is correct becausetests/issue-163-author-photo.spec.jsstill fetches it. - Showcase docs: adds
docs/demo/README.md,verification.md, and the source manifests.catalogue-sources.jsonhas 58 entries: 50 catalogue books and 8 wanted.periodical-sources.jsonhas 10 issues and 22 articles. Both files parse as valid JSON, and the counts match the README.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues found. Both earlier documentation notes are addressed.
Reviewed changes
This run reviewed 48293e1, which follows the prior review of a7c8ce6.
- Mobile API author summary:
wantedDetail()inCollectionsControllernow uses the same role-filteredautoreexpression aswanted()anddonationPrefill().autore_principale_nomeis filtered by role too, so theweb_urlslug matches the list. List, detail and donation prefill now give the sameauthor. - Contributor credits: added a detail-only
contributorsarray (id, displaynamewith pseudonym,role) that keeps every role, including translators, as #237 requires. The order is principal, then co-author, then the rest byordine_credito.OpenApiControllerdocuments it as an optionalWantedBookproperty, so existing clients keep working. - Integration coverage:
tests/mobile-collections.integration.phpnow checks that list, detail and prefill agree on authors. It also checks that a translator search still finds the book, that contributor roles keep their order, that pseudonyms are shown, and that a translator-only record hasauthor: nulland keeps its contributor. Newautorirows go into$authorIds, so the existingfinallyblock cleans them up.php -lpasses; the suite itself needs a disposable MySQL server and was not run here. - Showcase docs:
emeroteca.jpgandclub.jpgare now in thedocs/demo/README.mdgallery. The session-log paragraph is gone fromverification.md, and its table row and closing paragraph now describe the aligned API behaviour and how it was validated.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues. One minor documentation note is inline.
Reviewed changes
This run covers 93d33dc. The previous review was of 48293e1.
- Books-first catalogue ordering:
UnifiedCatalogService::page()sorts books ahead of articles in the UNION beforeLIMIT/OFFSET. It does this only when the sort isnewestand the installation settingcatalog.books_firstis'1'. The setting defaults to'0', so other installations keep the mixed order. Explicit sorts are unchanged, and so is the author page, which usespublication_desc. - Card status and wishlist row: the status pill and the heart now sit in a 48px row at the bottom of
.pk-card__panelinstead of covering the artwork. Thebody.pkpanel rule beats the ≤520pxpaddingshorthand, so the row also appears on phones. When a media badge is present, the status moves right to make room for it. List view is unaffected because its panel isdisplay: contents, its status isposition: static, and the heart now setsbottom: auto. - Integration coverage: added six checks for this ordering. They cover the setting off and on, totals, the page boundary between books and articles, and the
title_asc/oldestorders. The originalcatalog.books_firstvalue is restored afterwards, andemerotecawas added to the plugin states that get restored.php -lpasses; the suite needs a disposable MySQL server and was not run here. - Showcase docs: added a note on the demo's
catalog.books_first=1setting and two verification bullets, and changed the check count to 63.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
Important
On grid cards, the wishlist heart is now covered by the "Details" row and can't be clicked with a mouse or touch. Details inline.
Reviewed changes
This run covers 4f6b88f. The previous review was of 93d33dc.
- Card footer controls:
.pk-card__panelis nowposition: staticand no longer lifts on hover. As a result, the status pill, media badge and heart are positioned against the whole.pk-card. They sit in the "Details" row at the foot of the card: status atleft: 64px(98pxwhen there is a media badge), heart atbottom: 0; right: 0. The card gap went from 14px to 10px and the Details link is smaller. The 40px top padding on blank books in "Classica" cards was removed, since the badge no longer sits on the cover. - Showcase docs: the books-first paragraph in
docs/demo/README.mdis now in English, which addresses the earlier note.verification.mdnow has an updated layout bullet and marks the ebook-only availability issue as fixed in Pinakes-Android PR #44 (the web detail view is still open). It also gets a paragraph summarising the Android validation. - Catalogue screenshot:
docs/demo/catalogo.jpgwas refreshed. Thepk-book-card.phpheader comment now describes the new layout.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues. The wishlist heart is clickable again. One small visual note is inline.
Reviewed changes
This run covers db65dae and c22ce5f. The previous review was of 4f6b88f.
- Card overlays back on the cover:
.pk-card__panelisposition: relativeagain and keeps main's hover lift. The status pill, media badge, heart and overlay link are positioned against the panel again, as on main. Compared with main, the only change is that they now sit at the bottom of the cover instead of the top. This removes the overlap with the Details row from the last review, and that thread is resolved. - Tinted panels: in
body.pk-cards-tinted, each overlay offset is 22px larger. This keeps the controls on the artwork, not in the panel padding. The list view still wins: its status and media rules areposition: static, come later in the file and are at least as specific, and the heart there getsbottom: auto. - Details row: restored to main's styling (
display: block, 13px link). The card gap stays at 10px. - Docs: the
verification.mdlayout bullet now says the controls sit on the cover. The Android unit-test counts went from 198 to 200, and the header comment inpk-book-card.phpwas updated to match.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues found in the two new commits. The earlier note about the status pill covering the publisher line on blank covers is still open, because these commits don't change that area.
Reviewed changes
This run covers 1c96e12 and 92e0b1a. The previous review was of c22ce5f.
- Card spacing: the base
.pk-cardflexgapwent from 10px to 18px, so titles clear the book shadow. All three card templates (pk-book-card.php,article-card.php, and the wishlist cards inprofile/wishlist.php) have exactly two flex children, the panel and.pk-card__body. The new gap therefore only sets the cover-to-title spacing and does not touch the Details row. List view is unchanged: it switches the card todisplay: gridwith its owncolumn-gap/row-gap. - Docs: the
verification.mdlayout bullet now says titles sit 18px from the cover, below the shadow.catalogo.jpgwas re-captured with the new spacing.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
Important
The new lightbox adds a UI string that is in none of the locale files, so ci-quality fails its locale check. Details inline. Otherwise the lightbox looks correct.
Reviewed changes
This run covers eb56f06. The previous review was of 92e0b1a.
- Shared PDF reader: added the
frontend-pdf-lightbox.phpandfrontend-pdf-script.phppartials. Each PDF now renders a<dialog>whose iframe has onlydata-src. The script checks forshowModal, then binds click and Space handlers once at document level. Opening a PDF moves its dialog to<body>and loads the frame. Closing clearssrc, removes the scroll lock and returns focus to the toggle. Modifier-clicks, and browsers withoutshowModal, keep the plain link behaviour. - Single legacy PDF: "Leggi PDF" in
frontend-buttons.phpis now a direct link to the PDF instead of a<button>, and its inline toggle script is gone.frontend-pdf-viewer.phpnow just includes the two partials withid="pdf-viewer-container". Inbook-detail.php, the buttons are output before the player, so the script finds the link when it runs. - Attachment list:
frontend-attachments.phpnow includes one lightbox per PDF, using the existing uniquedigital-pdf-viewer-Nids, instead of the inline viewer below the card. The old toggle code was removed. - Styles and docs: removed the old
#pdf-viewer-container/.pdf-viewer-framerules and added the full-viewport dialog styles: blurred backdrop, safe-area padding, and a two-row toolbar at ≤600px. Updated the plugin README and theverification.mdPDF bullet. - Tests:
digital-attachments-445.unit.phpnow checks for two modal dialogs, no preloadedsrc, and escaping of the single-PDF title. It passes 57 checks here, andfrontend-layout-variants.unit.phpstill passes. The Playwright spec checks full-viewport bounds, Escape, focus return and frame cleanup.
The earlier thread about the status pill covering the publisher line on blank covers is still open, because this commit does not change pinakes-2026.css.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues found. The missing locale key from the last review is fixed, and that thread is resolved.
Reviewed changes
This run covers beb653f. The previous review was of eb56f06.
- Lightbox action localized: added
"Apri in una nuova scheda"to all fivelocale/*.jsonfiles, next to the existingChiudikeys. The translations are Danish, German, English and French, andit_ITmaps the key to itself. All five files still parse, andpython3 scripts/ci-check-locales.pypasses here. - PDF screenshot: re-captured
docs/demo/pdf.jpg. It now shows the fullscreen dialog toolbar, with the "Apri in una nuova scheda" and "Chiudi" actions above the embedded reader.
The earlier thread about the status pill covering the publisher line on blank covers is still open. This commit does not change pinakes-2026.css.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
Important
In the new reader, pressing Esc to close the search bar or the menu also closes the whole lightbox. Details inline.
Reviewed changes
This run covers 8373a81 and bda864a. The previous review was of beb653f.
- Self-hosted PDF.js reader: vendored the PDF.js 6.4.299 legacy distribution under
public/assets/vendor/pdfjs-6.4.299/. The engine, worker and viewer are unchanged.viewer.htmladds the title, a meta CSP that allows only same-origin, blob and data connections, and thepinakes-viewer.csslight skin. A local.htaccesssupplies the.mjs/.wasmMIME types, andPINAKES.mdrecords where the files came from and the archive's SHA-256. - Lightbox integration: same-origin PDFs now open in
viewer.html?file=…#zoom=auto. Other-origin PDFs keep the browser's reader, so no proxy is needed and PDF.js's origin check is not bypassed. Awebviewerloadedhandler finds the matching iframe and sets the viewer options before startup: preferences, scripting, the alt-text model and annotation editing are disabled, and the light theme is forced. Every option name exists in 6.4.299, andPDFViewerApplicationOptionsis set on the frame'swindow. - Release packaging:
.gitignorenow un-ignores the vendoredbuild/directory. The globalbuild/rule had keptpdf.mjsandpdf.worker.mjsout of releases..gitattributesmarks the directory as vendored and turns off whitespace checks for it. - Frame layout: the iframe is now centred, with side margins from
clamp(32px, 8vw, 144px), a maximum width of 1280px and a 1px border. Phones get 12px margins. - Tests and docs: the unit test checks that the required vendored assets exist and that
viewer.htmlloads no remote script or stylesheet. The spec opens the uploaded PDF in dark mode. It checks that the reader and PDF are same-origin, the PDF's content type, that a page canvas renders, the light colour scheme and background, and the side margins. The plugin README andverification.mddescribe the new reader.
The earlier thread about the status pill covering the publisher line on blank covers is still open, because these commits do not change pinakes-2026.css.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues in
92d930a. Two earlier threads are still open because this commit doesn't touch them: Esc closes the whole lightbox when it should only close the PDF.js search bar or menu, and the status pill covers the publisher line on blank covers.
Reviewed changes
This run covers 92d930a. The previous review was of bda864a.
- PDF frame width:
.digital-pdf-lightbox__framenow caps its width withwidth: min(1280px, calc(100% - clamp(32px, 8vw, 144px)))instead of a separatemax-width: 1280px. The result is the same, but a globaliframe { max-width: … }rule can no longer change the cap or the side margins. The ≤600px rule still setswidth: calc(100% - 24px), so phones keep their 12px margins.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues in
f5c0b1f. Two earlier threads are still open because this commit doesn't touch them: Esc closes the whole lightbox when it should only close the PDF.js search bar or menu, and the status pill covers the publisher line on blank covers.
Reviewed changes
This run covers f5c0b1f. The previous review was of 92d930a.
- Single-page reader layout: the
webviewerloadedhandler infrontend-pdf-script.phpnow also setsspreadModeOnLoadto0(SpreadMode.NONE) andscrollModeOnLoadto0(ScrollMode.VERTICAL). Both options exist in the vendored 6.4.299AppOptionsand default to-1. When either is set to a known value,viewer.mjsno longer turns the PDF's own/PageLayout(for exampleTwoColumnLeft) into a two-page spread. PDFs therefore always open as one vertical column of pages, which stays readable on narrow screens.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues in
be58817. The Esc thread is resolved: Esc now always closes the lightbox on purpose. One earlier thread is still open because this commit doesn't touch it: the status pill covers the publisher line on blank covers.
Reviewed changes
This run covers be58817. The previous review was of f5c0b1f.
- Esc always exits the reader: the Esc
keydownlistener on the PDF.js frame document no longer checks whether the search bar, the menu or an inner dialog is open. It now callspreventDefault()and closes the lightbox straight away. The code comment states this intent, so there is no longer a gap between what the code says and what it does. As a result, Esc from the search bar, the>>menu or a PDF.js dialog (for example the password prompt) closes the whole reader. Closing also clears the framesrc.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues in
f30cc86andd4caf02. One earlier thread is still open, because these commits don't changepinakes-2026.css: on blank covers, the status pill covers the publisher line.
Reviewed changes
This run covers f30cc86 and d4caf02. The previous review was of be58817.
- Reader script moved to its own file: the lightbox logic moved unchanged from the inline
<script>infrontend-pdf-script.phptostorage/plugins/digital-library/assets/js/pdf-reader.js. The partial now outputs only a deferred<script src>with a?v=<filemtime>cache-buster. The existing/plugins/digital-library/assets/{type}/{filename}route serves it asapplication/javascript; that route already allowsjsand checks the resolved path stays inside the folder. The CSP'sscript-src 'self'allows it. The file is tracked and not caught by the plugin's.gitignoreallow-list, so release archives and the bundled-plugin update both include it. When one page includes the partial more than once,window.digitalPdfLightboxBoundstill stops the handlers being bound twice. - CodeQL scope:
paths-ignorenow skips only the four unmodified PDF.js modules (pdf.mjs,pdf.worker.mjs,pdf.sandbox.mjs,web/viewer.mjs). The app's own reader script is still scanned. - Tests and docs:
digital-attachments-445.unit.phpnow checks thatpdf-reader.jsexists. It passes all 67 checks here,frontend-layout-variants.unit.phpstill passes, andnode --checkaccepts the new script. The PDF bullet inverification.mdnow covers the fallback for a PDF on another domain that doesn't allow CORS, with the count updated to 67.pdf.jpgwas re-captured.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues found. The earlier note about the status pill covering the publisher line on blank covers is fixed, so no Pullfrog threads are left open.
Reviewed changes
This run covers 22ba156 and 76d53b6. The previous review was of d4caf02.
- PDF URLs are checked before the dialog opens:
openViewer()inpdf-reader.jsnow parsesdata-srcwithnew URL(). A malformed URL, or any scheme other thanhttp:/https:(for examplejavascript:,data:,file:orblob:), stops the dialog from opening. Same-origin PDFs also require a same-origindata-reader. This always holds, becauseurl()outputs a path for it. The reader URL is now built withsearchParams.set('file', …)andURL.hashinstead of joining strings, so query parameters inside the PDF link stay inside onefilevalue. The final URL's scheme is checked again before it is assigned tosrc. - JavaScript regression tests: added
tests/digital-pdf-source.test.cjs. It runs the shipped script innode:vmwith a small stub DOM and checks the resulting iframesrc. It has 11 tests: 5 unsafe URLs, 3 bad readers, same-origin zoom, the native path for an external PDF, and query encoding. Theci-qualitystep now runsnode --test tests/*.test.cjs, which also picks up the existing CSRF test. All 18 tests pass here.URLSearchParams.sizeis supported on CI's Node 22. - Blank-cover publisher line: added
body.pk .pk-card .pk-book__blank { padding-bottom: 40px; }, which keeps the publisher line clear of the status pill in both themes. Its specificity (0,3,1) beats the base.pk-book__blankpadding and loses to the list-view override (0,4,1), so list thumbnails are unchanged. It also applies to the wishlist and article cards, which use the same bottom overlays. - Docs: the PDF bullet in
verification.mdnow mentions the 11 JavaScript regression tests.
claude-opus-5-5 | 𝕏
There was a problem hiding this comment.
✅ No new issues in
e57d946, and no Pullfrog threads are open.
Reviewed changes
This run covers e57d946. The previous review was of 76d53b6.
- Sticky cover on desktop: on the book page, the cover now stays visible while the long info and digital-content column scrolls past. This only applies at 1024px and wider with a fine hover pointer. The cover sticks at
top: 88px, the same offset.pk-filtersuses, and stops at the bottom of the hero. It works because#book-cover-containernow stretches to the full height of the flex line. The grid'salign-items: flex-startand the column'smargin: 0 autodon't block this. At 1024px the cover (340px), the gap (64px) and the info column (460px) still fit on one line. - Horizontal overflow: on pages containing
.pk-bookhero,htmlandbodynow useoverflow-x: clip !importantinstead ofmain.css'shidden !important. The new selector is more specific, so it wins, andbodystops being a scroll container that would trap the sticky cover. The selector only matches the book detail page. The PDF lightbox scroll lock (html.digital-pdf-open { overflow: hidden }) still works, because only the x axis is!important. - Short viewports:
max-inline-size: calc((100dvh - 112px) * 2 / 3)makes the cover narrower so it fits in the viewport, and centres it in its column.verification.mdlists this as intended behaviour, checked at 1280 × 500. - Docs: added an Italian bullet to
verification.mddescribing the sticky-cover checks, matching the rest of that file.
claude-opus-5-5 | 𝕏

Desiderata and donation forms included translators and other contributors in the author field. The wanted list, donation prefill and mobile detail now select principal authors and co-authors in credit order. The mobile detail also exposes all contributors with their roles and pseudonyms, preserving the behavior requested in #237. The additive field is documented in OpenAPI; old clients remain compatible.
The README presents the public Biblioteca delle idee libere installation, with a complete 13-image gallery and source manifests for 50 catalogue books, 8 wanted books, 3 periodicals, 22 articles, 11 PDFs and 2 complete audiobooks. Holdings and activities are illustrative; the documentation explains digital-edition provenance and remaining installation findings. Both documentation findings from Pullfrog are addressed.
The recent catalogue view can foreground books through the opt-in installation setting
catalog.books_first=1, sorting the mixed UNION before pagination while preserving articles and explicit alphabetical/oldest orders. It is enabled on the demo; other installations retain the current mixed order. Card titles sit 18px below their covers to clear the book shadow; availability badges and wishlist hearts overlay the bottom of the cover, with no added empty space between covers and titles. The catalogue screenshot is refreshed.PDF reading now opens a native modal dialog across the entire viewport rather than inside the book column. Single PDFs and multiple attachments share the same lazy-loaded reader, with a light translucent blurred backdrop, responsive toolbar, visible close action, Escape support, restored focus and document cleanup on close. The direct PDF link remains available without JavaScript or modal support, and an open-in-new-tab action remains visible. Same-origin PDFs now use bundled Mozilla PDF.js 6.4.299 (legacy build) with a fixed light theme, adaptive lateral margins and automatic zoom and a single-page vertical layout. The viewer, worker, fonts, WASM and translations are served locally with no CDN. The upstream archive digest is verified; core modules remain unmodified. Its CSP restricts fetching to the local origin, blobs and data, and the versioned directory supplies correct module/WASM MIME types. Remote PDFs retain native reading without an origin-check bypass or a proxy.
Validation: PHP syntax checks and PHPStan level 5 pass. On a separate disposable MySQL server, 63 collection/API integration checks and 5 desiderata facet checks pass. New cases verify list/detail/donation consistency, preserved contributor roles and pseudonyms, translator-only records, and search by translator. Six new SQL checks cover book priority, the pagination boundary, counts, opt-out, alphabetical and oldest orders. Live API checks across all six demo pages find exactly 50 books followed by 22 articles, without omissions or duplicates. Browser geometry checks at 1512px and 390px show both controls inside the bottom of each cover, no overlap between controls, and no horizontal overflow. Read-only checks on the deployed demo confirm the corrected summary and retained translator; public OpenAPI returns the documented contributor field.
The digital-attachments unit suite passes 67 checks; the existing browser regression now covers fullscreen bounds, Escape, focus return and lazy-frame cleanup. Local browser checks cover single/multiple/supplement PDFs, desktop, 390px portrait and 844px landscape; no dialog overflow. The live reader renders all 94 Malatesta pages with selectable text, navigation, zoom and search (66 matches). Seven HTTPS contracts pass for local module/WASM MIME, localization, same-origin access, partial PDF responses (206), and no reflected/wildcard CORS on viewer assets. A second local origin without CORS verifies native fallback and close cleanup. All 408 viewer assets are tracked and required engine/worker files are present in Git archives.
Android client fixes are in Pinakes-Android #44, validated with 200 debug and 200 release unit tests, 22 Android 15 instrumented tests, lint and a release R8 build. No merge or release is performed as part of this work.
PHPStan now passes across the complete project after fixing the PDF partial’s variable annotations. The lightbox integration lives in a standalone, self-hosted JavaScript file so CodeQL analyzes application code; only the four verified, unmodified upstream PDF.js modules are excluded from analysis, consistent with the existing vendor policy.
The iframe boundary now revalidates canonical HTTP(S) URLs and rejects off-origin reader URLs before opening a dialog. Eleven production-script regressions cover executable/malformed URLs, external viewers, encoding and native external-PDF fallback; all pass. These run alongside the existing seven CSRF concurrency regressions in CI.
Generated covers reserve space above the bottom overlays, keeping the publisher visible. Browser measurements show a 7.7px gap in both classic/tinted themes and at 390px; list thumbnails retain their 8px padding and hidden publisher footer. CodeQL passes after DOM URL validation.
La copertina del dettaglio libro segue lo scroll solo su desktop con mouse/trackpad e si ferma alla fine della sezione multimediale. Verificati desktop normale e basso, tablet e telefono, e blocco/ripristino dello scroll del lightbox PDF. La CSS è già allineata sull’installazione dimostrativa.