Skip to content

Showcase the public library and fix catalog presentation and desiderata credits - #472

Merged
fabiodalez-dev merged 20 commits into
mainfrom
fix/public-library-showcase
Oct 12, 2026
Merged

fabiodalez-dev merged 20 commits into
mainfrom
fix/public-library-showcase

Conversation

@fabiodalez-dev

@fabiodalez-dev fabiodalez-dev commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

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.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: fabiodalez-dev/Pinakes/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 266e9b21-73a9-4374-b994-1882fe46f529

📥 Commits

Reviewing files that changed from the base of the PR and between 48293e1 and 92e0b1a.


⛔ Files ignored due to path filters (1)
  • docs/demo/catalogo.jpg is excluded by !**/*.jpg

📒 Files selected for processing (6)
  • app/Services/UnifiedCatalogService.php
  • app/Views/frontend/partials/pk-book-card.php
  • docs/demo/README.md
  • docs/demo/verification.md
  • public/assets/pinakes-2026.css
  • tests/mobile-collections.integration.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.



📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

Il PR documenta la demo pubblica della biblioteca e aggiunge dati di riferimento, schermate e verifiche. Modifica inoltre l’ordinamento recente del catalogo, la posizione dei badge sulle schede e i riepiloghi dei crediti dei desiderata.

Changes

Demo pubblica della biblioteca

Layer / File(s) Summary
Dati di catalogo e periodici
docs/demo/catalogue-sources.json, docs/demo/periodical-sources.json
Aggiunge 58 record bibliografici, 10 numeri di periodici e 22 articoli con i relativi dati di fonte.
Descrizione, schermate e verifiche
README.md, docs/demo/README.md, docs/demo/verification.md
Documenta la demo, le risorse, le fonti e i diritti. Aggiunge schermate desktop e mobile e registra controlli, problemi e stato delle verifiche.

Autori nei desiderata

Layer / File(s) Summary
Riepilogo autori e contributori
storage/plugins/desiderata/DesiderataPlugin.php, storage/plugins/mobile-api/src/Controllers/CollectionsController.php, storage/plugins/mobile-api/src/Controllers/OpenApiController.php, tests/mobile-collections.integration.php
I riepiloghi in wanted(), donationPrefill() e nell’API includono solo autori principali e co-autori, ordinati per ruolo e ordine di credito. Il dettaglio dell’API aggiunge tutti i contributori con ID, nome e ruolo; i test verificano entrambi i comportamenti.

Priorità dei libri nel catalogo

Layer / File(s) Summary
Ordinamento per novità
app/Services/UnifiedCatalogService.php, tests/mobile-collections.integration.php, docs/demo/README.md
Con catalog.books_first attiva, l’ordinamento recente antepone i libri agli articoli prima della paginazione. I test verificano anche che gli articoli restino inclusi e che gli altri ordinamenti non cambino.

Presentazione delle schede libro

Layer / File(s) Summary
Badge e preferiti sulle copertine
app/Views/frontend/partials/pk-book-card.php, public/assets/pinakes-2026.css
Aggiorna la posizione dei badge di stato e dei preferiti nella griglia e nelle schede legacy. La vista a lista annulla il posizionamento inferiore del preferito.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature


Merge Risk: ⚪ Minimal · up to 92e0b

No additional issue has been established that would prevent merging after normal checks.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. (3 skipped: 3 u…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed Il titolo descrive in modo chiaro i principali cambiamenti: presentazione della biblioteca pubblica, catalogo e crediti delle desiderata.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@fabiodalez-dev
fabiodalez-dev marked this pull request as ready for review October 12, 2026 10:38

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 autore subqueries in wanted() and donationPrefill() are now limited to principale/co-autore roles, ordered with the principal first and then by ordine_credito. Both expressions are identical, so the two surfaces still agree. The role filter matches the convention core already uses in BookRepository, NotificationService and LoanRepository, and php -l passes.
  • Root README: adds a "Live demo" section and replaces the docs/catalog.png gallery entry with the new docs/demo/*.jpg screenshots. docs/catalog.png stays in the repo, which is correct because tests/issue-163-author-photo.spec.js still fetches it.
  • Showcase docs: adds docs/demo/README.md, verification.md, and the source manifests. catalogue-sources.json has 58 entries: 50 catalogue books and 8 wanted. periodical-sources.json has 10 issues and 22 articles. Both files parse as valid JSON, and the counts match the README.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread docs/demo/README.md
Comment thread docs/demo/verification.md Outdated
@fabiodalez-dev fabiodalez-dev changed the title Document the public library showcase and fix desiderata credits Document the public library showcase and align desiderata credits Oct 12, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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() in CollectionsController now uses the same role-filtered autore expression as wanted() and donationPrefill(). autore_principale_nome is filtered by role too, so the web_url slug matches the list. List, detail and donation prefill now give the same author.
  • Contributor credits: added a detail-only contributors array (id, display name with pseudonym, role) that keeps every role, including translators, as #237 requires. The order is principal, then co-author, then the rest by ordine_credito. OpenApiController documents it as an optional WantedBook property, so existing clients keep working.
  • Integration coverage: tests/mobile-collections.integration.php now 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 has author: null and keeps its contributor. New autori rows go into $authorIds, so the existing finally block cleans them up. php -l passes; the suite itself needs a disposable MySQL server and was not run here.
  • Showcase docs: emeroteca.jpg and club.jpg are now in the docs/demo/README.md gallery. The session-log paragraph is gone from verification.md, and its table row and closing paragraph now describe the aligned API behaviour and how it was validated.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@fabiodalez-dev fabiodalez-dev changed the title Document the public library showcase and align desiderata credits Showcase the public library and fix catalog presentation and desiderata credits Oct 12, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 before LIMIT/OFFSET. It does this only when the sort is newest and the installation setting catalog.books_first is '1'. The setting defaults to '0', so other installations keep the mixed order. Explicit sorts are unchanged, and so is the author page, which uses publication_desc.
  • Card status and wishlist row: the status pill and the heart now sit in a 48px row at the bottom of .pk-card__panel instead of covering the artwork. The body.pk panel rule beats the ≤520px padding shorthand, 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 is display: contents, its status is position: static, and the heart now sets bottom: 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/oldest orders. The original catalog.books_first value is restored afterwards, and emeroteca was added to the plugin states that get restored. php -l passes; the suite needs a disposable MySQL server and was not run here.
  • Showcase docs: added a note on the demo's catalog.books_first=1 setting and two verification bullets, and changed the check count to 63.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread docs/demo/README.md Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__panel is now position: static and 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 at left: 64px (98px when there is a media badge), heart at bottom: 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.md is now in English, which addresses the earlier note. verification.md now 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.jpg was refreshed. The pk-book-card.php header comment now describes the new layout.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread public/assets/pinakes-2026.css Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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__panel is position: relative again 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 are position: static, come later in the file and are at least as specific, and the heart there gets bottom: auto.
  • Details row: restored to main's styling (display: block, 13px link). The card gap stays at 10px.
  • Docs: the verification.md layout bullet now says the controls sit on the cover. The Android unit-test counts went from 198 to 200, and the header comment in pk-book-card.php was updated to match.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread public/assets/pinakes-2026.css

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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-card flex gap went 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 in profile/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 to display: grid with its own column-gap/row-gap.
  • Docs: the verification.md layout bullet now says titles sit 18px from the cover, below the shadow. catalogo.jpg was re-captured with the new spacing.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.php and frontend-pdf-script.php partials. Each PDF now renders a <dialog> whose iframe has only data-src. The script checks for showModal, then binds click and Space handlers once at document level. Opening a PDF moves its dialog to <body> and loads the frame. Closing clears src, removes the scroll lock and returns focus to the toggle. Modifier-clicks, and browsers without showModal, keep the plain link behaviour.
  • Single legacy PDF: "Leggi PDF" in frontend-buttons.php is now a direct link to the PDF instead of a <button>, and its inline toggle script is gone. frontend-pdf-viewer.php now just includes the two partials with id="pdf-viewer-container". In book-detail.php, the buttons are output before the player, so the script finds the link when it runs.
  • Attachment list: frontend-attachments.php now includes one lightbox per PDF, using the existing unique digital-pdf-viewer-N ids, 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-frame rules and added the full-viewport dialog styles: blurred backdrop, safe-area padding, and a two-row toolbar at ≤600px. Updated the plugin README and the verification.md PDF bullet.
  • Tests: digital-attachments-445.unit.php now checks for two modal dialogs, no preloaded src, and escaping of the single-PDF title. It passes 57 checks here, and frontend-layout-variants.unit.php still 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread storage/plugins/digital-library/views/frontend-pdf-lightbox.php

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 five locale/*.json files, next to the existing Chiudi keys. The translations are Danish, German, English and French, and it_IT maps the key to itself. All five files still parse, and python3 scripts/ci-check-locales.py passes 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.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.html adds the title, a meta CSP that allows only same-origin, blob and data connections, and the pinakes-viewer.css light skin. A local .htaccess supplies the .mjs/.wasm MIME types, and PINAKES.md records 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. A webviewerloaded handler 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, and PDFViewerApplicationOptions is set on the frame's window.
  • Release packaging: .gitignore now un-ignores the vendored build/ directory. The global build/ rule had kept pdf.mjs and pdf.worker.mjs out of releases. .gitattributes marks 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.html loads 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 and verification.md describe 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread storage/plugins/digital-library/views/frontend-pdf-script.php Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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__frame now caps its width with width: min(1280px, calc(100% - clamp(32px, 8vw, 144px))) instead of a separate max-width: 1280px. The result is the same, but a global iframe { max-width: … } rule can no longer change the cap or the side margins. The ≤600px rule still sets width: calc(100% - 24px), so phones keep their 12px margins.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 webviewerloaded handler in frontend-pdf-script.php now also sets spreadModeOnLoad to 0 (SpreadMode.NONE) and scrollModeOnLoad to 0 (ScrollMode.VERTICAL). Both options exist in the vendored 6.4.299 AppOptions and default to -1. When either is set to a known value, viewer.mjs no longer turns the PDF's own /PageLayout (for example TwoColumnLeft) into a two-page spread. PDFs therefore always open as one vertical column of pages, which stays readable on narrow screens.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 keydown listener on the PDF.js frame document no longer checks whether the search bar, the menu or an inner dialog is open. It now calls preventDefault() 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 frame src.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread storage/plugins/digital-library/assets/js/pdf-reader.js Fixed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues in f30cc86 and d4caf02. One earlier thread is still open, because these commits don't change pinakes-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> in frontend-pdf-script.php to storage/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 as application/javascript; that route already allows js and checks the resolved path stays inside the folder. The CSP's script-src 'self' allows it. The file is tracked and not caught by the plugin's .gitignore allow-list, so release archives and the bundled-plugin update both include it. When one page includes the partial more than once, window.digitalPdfLightboxBound still stops the handlers being bound twice.
  • CodeQL scope: paths-ignore now 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.php now checks that pdf-reader.js exists. It passes all 67 checks here, frontend-layout-variants.unit.php still passes, and node --check accepts the new script. The PDF bullet in verification.md now covers the fallback for a PDF on another domain that doesn't allow CORS, with the count updated to 67. pdf.jpg was re-captured.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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() in pdf-reader.js now parses data-src with new URL(). A malformed URL, or any scheme other than http:/https: (for example javascript:, data:, file: or blob:), stops the dialog from opening. Same-origin PDFs also require a same-origin data-reader. This always holds, because url() outputs a path for it. The reader URL is now built with searchParams.set('file', …) and URL.hash instead of joining strings, so query parameters inside the PDF link stay inside one file value. The final URL's scheme is checked again before it is assigned to src.
  • JavaScript regression tests: added tests/digital-pdf-source.test.cjs. It runs the shipped script in node:vm with a small stub DOM and checks the resulting iframe src. It has 11 tests: 5 unsafe URLs, 3 bad readers, same-origin zoom, the native path for an external PDF, and query encoding. The ci-quality step now runs node --test tests/*.test.cjs, which also picks up the existing CSRF test. All 18 tests pass here. URLSearchParams.size is 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__blank padding 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.md now mentions the 11 JavaScript regression tests.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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-filters uses, and stops at the bottom of the hero. It works because #book-cover-container now stretches to the full height of the flex line. The grid's align-items: flex-start and the column's margin: 0 auto don'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, html and body now use overflow-x: clip !important instead of main.css's hidden !important. The new selector is more specific, so it wins, and body stops 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.md lists this as intended behaviour, checked at 1280 × 500.
  • Docs: added an Italian bullet to verification.md describing the sticky-cover checks, matching the rest of that file.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@fabiodalez-dev
fabiodalez-dev merged commit 2d986c3 into main Oct 12, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants