fix: genre filter 500 error and clickable genre hierarchy#91
Open
fabiodalez-dev wants to merge 18 commits intomainfrom
Open
fix: genre filter 500 error and clickable genre hierarchy#91fabiodalez-dev wants to merge 18 commits intomainfrom
fabiodalez-dev wants to merge 18 commits intomainfrom
Conversation
Add missing LEFT JOIN for sottogenere (sg) table alias in catalog, catalogAPI, and getFilterOptions queries. The buildWhereConditions method references sg.nome but four queries were missing the JOIN, causing a 500 error when filtering by genre. Also make each genre level in book detail page a separate clickable link (Prosa > Narrativa > Fiaba) instead of a single link to the deepest genre, both in the hero area and details section. Fixes #90
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds plaintext descrizione storage and description-inclusive search, propagates subgenre joins through catalog queries, introduces configurable social sharing (UI, settings, route, partial), and adds an inline lazy-loading PDF viewer + plugin hook and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (browser)
participant Page as Book Detail Page
participant Controller as FrontendController
participant Plugin as DigitalLibraryPlugin
participant DB as Server/DB
participant Iframe as PDF iframe
User->>Page: Click "Show PDF" toggle
Page->>Controller: request share/OG data & PDF render scaffold
Controller->>Plugin: call renderPdfViewer(book)
Plugin->>DB: validate file_url + check extension (pdf)
DB-->>Plugin: returns validated URL
Plugin->>Page: insert/show viewer container (data-pdf-src set)
Note right of Page: MutationObserver watches visibility
Page->>Iframe: set iframe.src = data-pdf-src (lazy load)
Iframe-->>User: PDF content loads in iframe
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
- Add l.descrizione to header search autocomplete (searchBooks and searchBooksWithDetails) so the search bar matches book descriptions - Move descrizione column right after sottotitolo in CSV export for more logical column ordering - Strip HTML tags from description in CSV export to prevent malformed rows when description contains HTML with newlines Fixes #83
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/SearchController.php`:
- Around line 238-241: The query is matching raw HTML in descrizione, causing
tag names/attributes to be searchable; instead create and maintain a plaintext
column (e.g., descrizione_plain) populated when saving/updating records
(strip_tags on the TinyMCE content), then change the SQL in the SearchController
query (the SELECT/WHERE that currently references l.descrizione and the
bind_param call) and in searchBooksWithDetails() to use descrizione_plain in the
LIKE comparisons; ensure inserts/updates set descrizione_plain and run any
backfill to populate existing rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6bff75a-2ced-46bc-ab33-3d655b6f3e25
📒 Files selected for processing (2)
app/Controllers/LibriController.phpapp/Controllers/SearchController.php
- Add inline PDF viewer using browser's native PDF rendering (iframe) - Fix ePub download: opens in new tab instead of forcing download - Add descrizione_plain column for HTML-free search (CodeRabbit fix) - Migration 0.4.9.9: add column + backfill strip_tags for MySQL 8.0+ - BookRepository: auto-populate descrizione_plain on insert/update - SearchController: use COALESCE(descrizione_plain, descrizione) in LIKE - Digital Library Plugin bumped to v1.3.0 - Translations added for IT, EN, DE
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/SearchController.php`:
- Around line 230-241: SearchController::books() uses a WHERE that only checks
isbn10/isbn13/ean/titolo/sottotitolo and thus misses description-only matches;
update the prepared statement in SearchController::books() (the SQL assigned to
$stmt and its bind_param) to include COALESCE(l.descrizione_plain,
l.descrizione) LIKE ? (same predicate as the other helper) and ensure the
bind_param signature still supplies six identical $s parameters; apply the same
change for the analogous block referenced around lines 351-354 so both endpoints
perform the same description-inclusive search.
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 28-38: The migration lacks a real backfill for MySQL 5.7 installs
so existing rows keep HTML in descrizione_plain; add a fallback SQL path in
migrate_0.4.9.9.sql that creates a small stored function (e.g.,
strip_tags_sql(TEXT)) which removes HTML tags (loop removing text between '<'
and '>') and then runs UPDATE `libri` SET `descrizione_plain` =
strip_tags_sql(`descrizione`) WHERE `descrizione` IS NOT NULL AND `descrizione`
!= '' AND `descrizione_plain` IS NULL; after the UPDATE DROP the function. This
will ensure the same backfill behavior for older MySQL/MariaDB rows that
BookRepository (create/update code around the existing lines you referenced)
only handles at write time and prevents COALESCE(l.descrizione_plain,
l.descrizione) from returning raw HTML for legacy records.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 168-174: registerHooks() currently treats hooks as unique by
(plugin_id, hook_name) so adding the PDF viewer (callback_method renderPdfViewer
in DigitalLibraryPlugin) is skipped as a duplicate of the audio player; update
the uniqueness check and insert logic inside registerHooks() to consider
callback_class and callback_method as part of the uniqueness key (or query) so
multiple callbacks can register for the same hook_name, and apply the same fix
for the other registration block referenced around the second occurrence (the
entries at ~200-203).
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 23-29: The toggle buttons (e.g., the button with
id="btn-toggle-pdf-viewer") must expose their controlled region and state to
assistive tech: add an aria-controls attribute referencing the id of the PDF
viewer container (e.g., "pdf-viewer") and set aria-expanded="false" initially,
then update aria-expanded to "true"/"false" in the JavaScript toggle handler
whenever you show/hide the viewer; repeat the same pattern for the
audiobook/player toggle buttons found in the other blocks (the buttons in the
50-56, 113-126, and 153-173 regions) to reference their player container ids and
update aria-expanded in their respective toggle functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 802cd942-4f78-45de-af90-2f27cdd4165d
📒 Files selected for processing (11)
app/Controllers/SearchController.phpapp/Models/BookRepository.phpinstaller/database/migrations/migrate_0.4.9.9.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.php
The registerHooks() uniqueness check used (plugin_id, hook_name) which prevented multiple callbacks on the same hook_name (e.g. both renderAudioPlayer and renderPdfViewer on book.detail.digital_player). Now checks (plugin_id, hook_name, callback_method) so new hooks are auto-registered on the first page load after update — no manual plugin deactivation/reactivation needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
168-174:⚠️ Potential issue | 🟠 Major
renderPdfViewerstill won't register on fresh installs.Adding a second
book.detail.digital_playerhook here exposes the existing dedupe bug inregisterHooks(): the lookup still matches only(plugin_id, hook_name), so the earlierrenderAudioPlayerrow makes this new callback look like a duplicate and it never gets inserted.Suggested fix
- $stmt = $this->db->prepare(" - SELECT id FROM plugin_hooks - WHERE plugin_id = ? AND hook_name = ? - "); + $stmt = $this->db->prepare(" + SELECT id FROM plugin_hooks + WHERE plugin_id = ? + AND hook_name = ? + AND callback_class = ? + AND callback_method = ? + "); @@ - $stmt->bind_param("is", $this->pluginId, $hook['hook_name']); + $stmt->bind_param( + "isss", + $this->pluginId, + $hook['hook_name'], + $hook['callback_class'], + $hook['callback_method'] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 168 - 174, The hook registration dedupe in registerHooks() is only comparing (plugin_id, hook_name) so adding 'book.detail.digital_player' for callback_method renderPdfViewer is skipped because renderAudioPlayer already registered the same hook_name; update registerHooks() to include the callback identity (e.g., callback_class and callback_method) in the existence check (and any unique key/lookup) so duplicates are only suppressed when plugin_id, hook_name, callback_class AND callback_method match; ensure the insert logic and any DB uniqueness constraints align with the new 4-field check so renderPdfViewer can be inserted alongside renderAudioPlayer.storage/plugins/digital-library/views/frontend-buttons.php (1)
23-29:⚠️ Potential issue | 🟡 MinorExpose the toggle targets and state to assistive tech.
The PDF and audiobook buttons still toggle controlled regions without
aria-controls/aria-expanded, and the handlers never update the expanded state. Screen readers won't get the open/closed status.♿ Proposed fix
<button type="button" id="btn-toggle-pdf-viewer" class="btn btn-outline-danger btn-lg" + aria-controls="pdf-viewer-container" + aria-expanded="false" title="<?= __("Leggi PDF") ?>"> @@ <button type="button" id="btn-toggle-audiobook" class="btn btn-outline-dark btn-lg" + aria-controls="audiobook-player-container" + aria-expanded="false" title="<?= __("Ascolta l'audiobook") ?>"> @@ var setButtonState = function(isOpen) { toggleBtn.innerHTML = isOpen ? closeContent : openContent; + toggleBtn.setAttribute('aria-expanded', isOpen ? 'true' : 'false'); };Also applies to: 50-56, 113-126, 153-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/views/frontend-buttons.php` around lines 23 - 29, The PDF toggle button (id "btn-toggle-pdf-viewer") and the other similar toggle buttons must expose their controlled region and expanded state to assistive tech: add an aria-controls attribute pointing to the id of the region they show/hide (e.g., "pdf-viewer" or the actual region id used in the markup) and set aria-expanded="false" initially; then update the click handler (the JS that toggles the viewer/audioplayer) to flip aria-expanded between "true" and "false" whenever it shows or hides the region and ensure the controlled region updates aria-hidden accordingly. Apply the same changes to the other toggle buttons mentioned in the file so all toggle handlers and buttons consistently manage aria-controls and aria-expanded.
🧹 Nitpick comments (1)
storage/plugins/digital-library/views/frontend-buttons.php (1)
109-115: Usejson_encode()with escape flags when embedding translated strings in inline JavaScript.The
__()helper returns raw text without escaping. While current locale values ("Read PDF", "Close Viewer", etc.) are safe, future translations containing',\, or</script>would break the JS string literal or become an injection sink. The recommended pattern keeps icon HTML and labels separate:Safer pattern
- var openContent = '<i class="fas fa-book-reader me-2"></i>' + '<?= __("Leggi PDF") ?>'; - var closeContent = '<i class="fas fa-times me-2"></i>' + '<?= __("Chiudi Visualizzatore") ?>'; + var openLabel = <?= json_encode(__("Leggi PDF"), JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT) ?>; + var closeLabel = <?= json_encode(__("Chiudi Visualizzatore"), JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT) ?>; + var openContent = '<i class="fas fa-book-reader me-2"></i>' + openLabel; + var closeContent = '<i class="fas fa-times me-2"></i>' + closeLabel;Also applies to lines 149–155.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/views/frontend-buttons.php` around lines 109 - 115, The inline JS builds strings with unescaped translations (openContent, closeContent, used by setButtonState and toggleBtn) which can break or enable injection if translations contain quotes or tags; change to keep icon HTML separate from the label and emit the translated strings using PHP's json_encode with safe flags (e.g. JSON_HEX_TAG|JSON_HEX_APOS|JSON_HEX_QUOT|JSON_HEX_AMP) so the label values are safely encoded into JS, then concatenate icon HTML and the encoded label in JS; apply the same fix to the other occurrences referenced around the block at lines 149–155.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 32-38: The version check in the migration uses `@mysql_version` and
`@has_regexp_replace` to decide whether to use REGEXP_REPLACE, but it currently
treats MariaDB 10.x as MySQL and will run REGEXP_REPLACE on MariaDB (which fails
on MariaDB 10.0.0–10.0.4); update the `@has_regexp_replace` assignment to
explicitly exclude MariaDB (e.g., add a NOT LIKE '%MariaDB%' clause or similar
check) so that REGEXP_REPLACE is only used for upstream MySQL versions
(variables: `@mysql_version`, `@has_regexp_replace`, and the SET `@sql` that uses
REGEXP_REPLACE).
In `@locale/it_IT.json`:
- Around line 85-86: Replace the platform-specific Italian strings with
browser-agnostic phrases: update the value for the key "Usa Ctrl+F per cercare
nel documento" to something like "Usa la funzione di ricerca del browser per
trovare testo nel documento" and update the value for "Premi F11 o usa il
pulsante nel viewer per la visualizzazione a schermo intero" to "Usa il
controllo di schermo intero del viewer o del browser" in locale/it_IT.json, and
apply the same exact new copy to the corresponding entries in locale/en_US.json
so both locales use the generic wording.
---
Duplicate comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 168-174: The hook registration dedupe in registerHooks() is only
comparing (plugin_id, hook_name) so adding 'book.detail.digital_player' for
callback_method renderPdfViewer is skipped because renderAudioPlayer already
registered the same hook_name; update registerHooks() to include the callback
identity (e.g., callback_class and callback_method) in the existence check (and
any unique key/lookup) so duplicates are only suppressed when plugin_id,
hook_name, callback_class AND callback_method match; ensure the insert logic and
any DB uniqueness constraints align with the new 4-field check so
renderPdfViewer can be inserted alongside renderAudioPlayer.
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 23-29: The PDF toggle button (id "btn-toggle-pdf-viewer") and the
other similar toggle buttons must expose their controlled region and expanded
state to assistive tech: add an aria-controls attribute pointing to the id of
the region they show/hide (e.g., "pdf-viewer" or the actual region id used in
the markup) and set aria-expanded="false" initially; then update the click
handler (the JS that toggles the viewer/audioplayer) to flip aria-expanded
between "true" and "false" whenever it shows or hides the region and ensure the
controlled region updates aria-hidden accordingly. Apply the same changes to the
other toggle buttons mentioned in the file so all toggle handlers and buttons
consistently manage aria-controls and aria-expanded.
---
Nitpick comments:
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 109-115: The inline JS builds strings with unescaped translations
(openContent, closeContent, used by setButtonState and toggleBtn) which can
break or enable injection if translations contain quotes or tags; change to keep
icon HTML separate from the label and emit the translated strings using PHP's
json_encode with safe flags (e.g.
JSON_HEX_TAG|JSON_HEX_APOS|JSON_HEX_QUOT|JSON_HEX_AMP) so the label values are
safely encoded into JS, then concatenate icon HTML and the encoded label in JS;
apply the same fix to the other occurrences referenced around the block at lines
149–155.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e5a43a8-b253-4915-8332-d38bd9c9edf7
📒 Files selected for processing (14)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Models/BookRepository.phpapp/Views/frontend/book-detail.phpinstaller/database/migrations/migrate_0.4.9.9.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.php
The manual-upgrade script's splitSqlStatements() parser could not handle
the nested escaped quotes ('' inside SET @SQL = IF(...)) used for the
COMMENT clause and REGEXP_REPLACE backfill. Simplified by:
- Removing COMMENT clause from ALTER TABLE
- Removing SQL-level backfill (PHP app handles it via BookRepository)
- Using 'SELECT 1' instead of 'DO 0' with comments as fallback
Tested: MySQL CLI (clean + idempotent), splitSqlStatements parser simulation
There was a problem hiding this comment.
♻️ Duplicate comments (1)
installer/database/migrations/migrate_0.4.9.9.sql (1)
14-16:⚠️ Potential issue | 🟡 MinorDocumentation inaccurate: no "on first access" backfill exists.
Line 15-16 states "Existing rows are backfilled on first access by the application," but the PHP code in
BookRepositoryonly populatesdescrizione_plainduring explicitINSERT(createBasic, lines 285-288) andUPDATE(updateBasic, lines 615-618) operations. There is no read-time lazy backfill logic.Books that existed before this migration and are never subsequently edited will permanently have
NULLindescrizione_plain, causingCOALESCE(l.descrizione_plain, l.descrizione)to fall back to raw HTML—defeating the purpose of the column for those rows.Correct the comment to accurately reflect the behavior:
📝 Proposed documentation fix
-- Backfill note: MySQL lacks strip_tags(), so the PHP application handles --- backfill via BookRepository on each insert/update. Existing rows are backfilled --- on first access by the application. +-- backfill via BookRepository on each insert/update. Existing rows remain NULL +-- until explicitly edited; SearchController falls back to raw descrizione via +-- COALESCE when descrizione_plain is NULL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.4.9.9.sql` around lines 14 - 16, Update the migration comment to remove the incorrect claim about lazy backfill on read and accurately state that only explicit INSERT/UPDATE paths perform HTML-stripping: mention that BookRepository populates descrizione_plain during createBasic (lines ~285-288) and updateBasic (lines ~615-618) and that existing pre-migration rows will remain NULL unless updated; adjust the text in migrate_0.4.9.9.sql to reflect this behavior and warn that a separate backfill script is required to populate descrizione_plain for historical rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 14-16: Update the migration comment to remove the incorrect claim
about lazy backfill on read and accurately state that only explicit
INSERT/UPDATE paths perform HTML-stripping: mention that BookRepository
populates descrizione_plain during createBasic (lines ~285-288) and updateBasic
(lines ~615-618) and that existing pre-migration rows will remain NULL unless
updated; adjust the text in migrate_0.4.9.9.sql to reflect this behavior and
warn that a separate backfill script is required to populate descrizione_plain
for historical rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 472cae78-e505-4ea0-a167-365673c355bd
📒 Files selected for processing (2)
installer/database/migrations/migrate_0.4.9.9.sqlstorage/plugins/digital-library/DigitalLibraryPlugin.php
- SearchController::books() now includes description in search (COALESCE) - Add aria-controls/aria-expanded to PDF viewer and audiobook toggles - Use json_encode(JSON_HEX_TAG) for PHP→JS translation strings - Replace platform-specific keyboard hints with browser-agnostic text - Update README changelog for v0.4.9.9, move v0.4.9.8 to Previous Releases - Document upgrade test learnings in updater.md (splitSqlStatements limits, manual-upgrade.php bundled plugins gap, migration best practices)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
BookRepository::getById() now populates descrizione_plain via strip_tags() for pre-migration rows on first access. Fixes inaccurate migration comment that claimed this backfill already existed.
Add configurable share buttons (Facebook, X, WhatsApp, Telegram, LinkedIn, Reddit, Pinterest, Email, Copy Link + Web Share API) to the public book detail page. Providers are managed via a new Settings > Sharing tab with live preview. Includes migration for existing installs and translations for IT/EN/DE.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/views/frontend-buttons.php (1)
40-49:⚠️ Potential issue | 🟡 MinorMisleading icon for non-PDF ebook downloads.
Line 46 uses
fa-file-pdficon for non-PDF ebooks (ePub, etc.), which is semantically incorrect and could confuse users.🧹 Suggested fix
<a href="<?= htmlspecialchars(url($book['file_url']), ENT_QUOTES, 'UTF-8') ?>" target="_blank" class="btn btn-outline-danger btn-lg" title="<?= __("Scarica l'eBook in formato digitale") ?>"> - <i class="fas fa-file-pdf me-2"></i> + <i class="fas fa-book me-2"></i> <?= __("Scarica eBook") ?> </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/views/frontend-buttons.php` around lines 40 - 49, The download button shown when $hasEbook is true uses the PDF-specific icon class (fa-file-pdf) which is misleading for non-PDF formats; update the icon in the anchor (the <a> for the eBook download) to a more generic ebook/read icon such as fa-book, fa-book-reader, or fa-file-alt so the visual matches ePub/other formats — locate the anchor with class "btn btn-outline-danger btn-lg" and replace the <i class="fas fa-file-pdf ..."> reference accordingly.
♻️ Duplicate comments (1)
installer/database/migrations/migrate_0.4.9.9.sql (1)
14-17:⚠️ Potential issue | 🟠 MajorLegacy rows still never get a one-time backfill.
This migration explicitly leaves existing
descrizione_plainvaluesNULLuntil each record is read or updated. That means untouched books still fall back to raw HTML during search, so the tag-matching bug survives for older rows. A migration-time backfill is still needed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.4.9.9.sql` around lines 14 - 17, Add a one-time backfill in the migration to populate descrizione_plain for existing rows instead of leaving them NULL: run an UPDATE that sets descrizione_plain = descrizione with HTML tags stripped (e.g. using REGEXP_REPLACE(descrizione, '<[^>]+>', '') on MySQL 8+ or an equivalent tag-stripping expression) for rows where descrizione_plain IS NULL, so SearchController's COALESCE fallback isn't relied on for legacy rows; reference descrizione_plain, descrizione, BookRepository/getById() and SearchController in the change so reviewers know this complements the runtime backfill logic.
🧹 Nitpick comments (4)
installer/database/data_en_US.sql (1)
279-282: Duplicatecms.events_page_enabledentry in the same INSERT block.Line 279 re-inserts
cms.events_page_enabledwhich was already inserted at line 190. While theON DUPLICATE KEY UPDATEclause prevents errors, this redundancy is confusing and could lead to maintenance issues if the two entries diverge.Consider removing the duplicate entry at line 279:
🧹 Suggested fix
-- CMS settings (already exists, but adding for completeness) -('cms', 'events_page_enabled', '1', 'Enable events page in frontend', NOW()), - -- Social sharing settings ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Enabled social sharing providers on book detail page', NOW())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_en_US.sql` around lines 279 - 282, Remove the duplicate insertion of the setting with key 'cms.events_page_enabled' from the INSERT block (the second occurrence that appears alongside the social sharing row) so only the original row remains; locate the INSERT ... VALUES block containing ('cms', 'events_page_enabled', ...) and delete the redundant tuple (the later one near the social sharing entries) while leaving the ON DUPLICATE KEY UPDATE clause intact to preserve idempotency.installer/database/data_it_IT.sql (1)
328-331: Same duplicate entry issue; description language inconsistency.
- Line 328 duplicates
cms.events_page_enabledalready inserted at line 7.- The sharing description is in English ("Enabled social sharing providers on book detail page") while some other settings in this file use Italian descriptions (e.g., line 7 uses "Abilita o disabilita la pagina degli eventi nel frontend").
Consider removing the duplicate and translating the description for consistency:
🧹 Suggested fix
-- CMS settings (already exists, but adding for completeness) -('cms', 'events_page_enabled', '1', 'Enable events page in frontend', NOW()), - -- Social sharing settings -('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Enabled social sharing providers on book detail page', NOW()) +('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Provider di condivisione social abilitati sulla pagina dettaglio libro', NOW())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_it_IT.sql` around lines 328 - 331, Remove the duplicate tuple that inserts the setting for cms.events_page_enabled (keep the original entry with Italian description), and update the description for the ('sharing','enabled_providers',...) row to Italian for consistency (e.g., replace "Enabled social sharing providers on book detail page" with an Italian phrase such as "Provider di condivisione social abilitati nella pagina dei dettagli del libro"); ensure only one cms.events_page_enabled entry remains and that sharing.enabled_providers retains the same key and value but with the translated description.installer/database/data_de_DE.sql (1)
278-281: Duplicatecms.events_page_enabledentry.Same issue as the other locale files: line 278 re-inserts
cms.events_page_enabled. The German description for the sharing setting is correctly localized.🧹 Suggested fix
-- CMS settings -('cms', 'events_page_enabled', '1', 'Veranstaltungsseite im Frontend aktivieren', NOW()), - -- Social sharing settings ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Aktivierte Social-Sharing-Anbieter auf der Buchdetailseite', NOW())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_de_DE.sql` around lines 278 - 281, Duplicate insertion of the key cms.events_page_enabled appears in installer/database/data_de_DE.sql; remove the redundant tuple ('cms', 'events_page_enabled', '1', 'Veranstaltungsseite im Frontend aktivieren', NOW()) so the file only defines cms.events_page_enabled once and keep the subsequent sharing tuple ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', ...) intact; verify no other duplicate rows for cms.events_page_enabled remain in this locale file.updater.md (1)
474-476: Consider adding a forward reference.The COMMENT clause removal in this example is explained later in Rule 6 (line 491). Consider adding a brief inline comment here like
-- (COMMENT omitted; see Rule 6 below)to guide readers encountering this example before the full explanation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater.md` around lines 474 - 476, Add a short forward-reference inline comment to the SQL example where SET `@sql` = IF(`@exists` = 0, 'CREATE TABLE t ( col ENUM(''a'', ''b'') )', 'SELECT 1'); appears; specifically insert a brief note after the ENUM/column definition such as -- (COMMENT omitted; see Rule 6 below) so readers encountering the CREATE TABLE t / ENUM example know the COMMENT clause removal is explained later in Rule 6.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: getFilterOptions() currently builds $options['generi'] and the
$queryGeneri / cnt counts only from l.genere_id, so third-level subgenres
(l.sottogenere_id) and their ancestor path are missing from the generated tree;
update the logic in getFilterOptions() to include l.sottogenere_id and its
ancestor ids when populating $queryGeneri and when computing cnt so that
getDisplayGenres() can find the active subgenre node (reference
variables/functions: getFilterOptions(), $options['generi'], $queryGeneri, cnt,
l.sottogenere_id, getDisplayGenres()).
In `@app/Models/BookRepository.php`:
- Around line 96-108: Replace raw strip_tags usage when populating
descrizione_plain with a normalized helper: add a private method
toPlainTextDescription(?string $html): ?string in the BookRepository class that
converts paragraph/BR tags to newlines, strips tags, decodes HTML entities,
collapses excess whitespace and trims (as suggested), and then call this helper
wherever descrizione_plain is derived from descrizione (the lazy backfill block
using $row['descrizione'] -> $plain and the other two places that currently call
strip_tags). Ensure you bind and store the returned value from
toPlainTextDescription instead of the direct strip_tags result so CSV export and
search see the normalized plain text.
In `@app/Views/frontend/book-detail.php`:
- Line 1549: Remove the legacy hardcoded sidebar share card markup and its
accompanying JS handler from book-detail.php so the included partial
(partials/social-sharing.php) is the single source of truth; ensure any provider
filtering logic uses the same config key (sharing.enabled_providers) and move or
consolidate provider-specific JS into the partial rather than leaving another
handler later in book-detail.php to avoid duplicate controls and mismatched
provider visibility.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 158-161: navigator.clipboard.writeText(url) is called without a
rejection handler so failures (e.g., NotAllowedError) are silent and the
textarea fallback isn't reached; add a .catch(...) to the promise returned by
navigator.clipboard.writeText in the same block that calls showCopyFeedback(btn)
so that on rejection you call the existing textarea fallback logic (the code
path that falls back to selecting/copying via a textarea) or otherwise invoke
the same failure handler used for the non-clipboard branch; reference
navigator.clipboard.writeText, showCopyFeedback, and the textarea fallback
routine to ensure the catch routes failures into the fallback path and
optionally logs the error.
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 19-28: Update the base schema so fresh installs include the new
column: add the column definition "descrizione_plain TEXT DEFAULT NULL AFTER
descrizione" to the libri table DDL in installer/database/schema.sql (matching
the column added by migrate_0.4.9.9.sql) so the libri table includes
descrizione_plain on initial import.
In `@README.md`:
- Around line 31-36: The README announces "Digital Library Plugin v1.3.0" but
the plugin inventory still references "digital-library-v1.2.0.zip"; update the
inventory entry to the new artifact name (replace "digital-library-v1.2.0.zip"
with "digital-library-v1.3.0.zip") and ensure any checksum/version fields tied
to that filename are updated to match the v1.3.0 release so the document is
consistent with the "Digital Library Plugin v1.3.0" header.
In `@updater.md`:
- Around line 760-767: Move the existing warning about bundled plugins to the
top of the manual-upgrade section (before the "Usage:" heading) and present it
as a prominent callout with a clear ⚠️ marker so it cannot be missed; in the
docs note that manual-upgrade.php does not call
Updater.php::updateBundledPlugins() and therefore preserves storage/plugins/,
and add a short subsection describing a minimal helper (or CLI step) that runs
in the same release ZIP to list which storage/plugins/<plugin-name>/ directories
differ and therefore must be copied manually; finally add a short "Long-term"
note recommending extracting splitSqlStatements() into a shared utility so
manual-upgrade.php can safely reuse Updater.php logic in the future.
---
Outside diff comments:
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 40-49: The download button shown when $hasEbook is true uses the
PDF-specific icon class (fa-file-pdf) which is misleading for non-PDF formats;
update the icon in the anchor (the <a> for the eBook download) to a more generic
ebook/read icon such as fa-book, fa-book-reader, or fa-file-alt so the visual
matches ePub/other formats — locate the anchor with class "btn
btn-outline-danger btn-lg" and replace the <i class="fas fa-file-pdf ...">
reference accordingly.
---
Duplicate comments:
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 14-17: Add a one-time backfill in the migration to populate
descrizione_plain for existing rows instead of leaving them NULL: run an UPDATE
that sets descrizione_plain = descrizione with HTML tags stripped (e.g. using
REGEXP_REPLACE(descrizione, '<[^>]+>', '') on MySQL 8+ or an equivalent
tag-stripping expression) for rows where descrizione_plain IS NULL, so
SearchController's COALESCE fallback isn't relied on for legacy rows; reference
descrizione_plain, descrizione, BookRepository/getById() and SearchController in
the change so reviewers know this complements the runtime backfill logic.
---
Nitpick comments:
In `@installer/database/data_de_DE.sql`:
- Around line 278-281: Duplicate insertion of the key cms.events_page_enabled
appears in installer/database/data_de_DE.sql; remove the redundant tuple ('cms',
'events_page_enabled', '1', 'Veranstaltungsseite im Frontend aktivieren', NOW())
so the file only defines cms.events_page_enabled once and keep the subsequent
sharing tuple ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', ...)
intact; verify no other duplicate rows for cms.events_page_enabled remain in
this locale file.
In `@installer/database/data_en_US.sql`:
- Around line 279-282: Remove the duplicate insertion of the setting with key
'cms.events_page_enabled' from the INSERT block (the second occurrence that
appears alongside the social sharing row) so only the original row remains;
locate the INSERT ... VALUES block containing ('cms', 'events_page_enabled',
...) and delete the redundant tuple (the later one near the social sharing
entries) while leaving the ON DUPLICATE KEY UPDATE clause intact to preserve
idempotency.
In `@installer/database/data_it_IT.sql`:
- Around line 328-331: Remove the duplicate tuple that inserts the setting for
cms.events_page_enabled (keep the original entry with Italian description), and
update the description for the ('sharing','enabled_providers',...) row to
Italian for consistency (e.g., replace "Enabled social sharing providers on book
detail page" with an Italian phrase such as "Provider di condivisione social
abilitati nella pagina dei dettagli del libro"); ensure only one
cms.events_page_enabled entry remains and that sharing.enabled_providers retains
the same key and value but with the translated description.
In `@updater.md`:
- Around line 474-476: Add a short forward-reference inline comment to the SQL
example where SET `@sql` = IF(`@exists` = 0, 'CREATE TABLE t ( col ENUM(''a'',
''b'') )', 'SELECT 1'); appears; specifically insert a brief note after the
ENUM/column definition such as -- (COMMENT omitted; see Rule 6 below) so readers
encountering the CREATE TABLE t / ENUM example know the COMMENT clause removal
is explained later in Rule 6.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85197963-13e5-44b2-aaea-aad94cdf5398
📒 Files selected for processing (21)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phpupdater.md
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Controllers/SearchController.php
- locale/it_IT.json
- locale/en_US.json
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
198-245:⚠️ Potential issue | 🟠 MajorMake hook registration atomic.
Line 203 still relies on a read-then-insert check, but
plugin_hookshas no unique constraint for hook identity. IfregisterHooks()runs concurrently, duplicate rows can still be inserted and the same player callback will execute twice on the book page. This should be backed by a unique index on(plugin_id, hook_name, callback_class, callback_method)and an upsert/insert-ignore write path instead of a separate existence check.🛠️ Suggested direction
- SELECT id FROM plugin_hooks - WHERE plugin_id = ? AND hook_name = ? AND callback_method = ? + -- add a UNIQUE KEY in a migration/schema update: + -- (plugin_id, hook_name, callback_class, callback_method) - INSERT INTO plugin_hooks - (plugin_id, hook_name, callback_class, callback_method, priority, is_active) - VALUES (?, ?, ?, ?, ?, ?) + INSERT INTO plugin_hooks + (plugin_id, hook_name, callback_class, callback_method, priority, is_active) + VALUES (?, ?, ?, ?, ?, ?) + ON DUPLICATE KEY UPDATE + priority = VALUES(priority), + is_active = VALUES(is_active)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 198 - 245, registerHooks() currently does a read-then-insert against plugin_hooks which allows race-condition duplicates; add a unique index on (plugin_id, hook_name, callback_class, callback_method) (via migration or plugin activation SQL) and replace the SELECT + INSERT flow with a single upsert/insert-ignore prepared statement (e.g. INSERT ... ON DUPLICATE KEY UPDATE or INSERT IGNORE) using the same bound parameters as the current INSERT block to make registration atomic, remove the initial SELECT branch, and preserve existing error logging and statement/connection cleanup for the new upsert statement.
♻️ Duplicate comments (4)
installer/database/migrations/migrate_0.4.9.9.sql (1)
14-28:⚠️ Potential issue | 🟠 MajorBackfill existing descriptions in the migration.
Right now legacy rows keep
descrizione_plain = NULLuntil they are edited or loaded throughBookRepository::getById(). BecauseSearchControllerfalls back toCOALESCE(l.descrizione_plain, l.descrizione), untouched books will still be searched against raw TinyMCE HTML, so tag names/attributes can keep matching indefinitely. This needs a migration-time backfill, not only lazy PHP population.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.4.9.9.sql` around lines 14 - 28, The migration currently only adds the new column descrizione_plain on table libri but does not backfill legacy rows, so SearchController still searches raw HTML; update the migration to, after adding descrizione_plain, run an UPDATE that sets descrizione_plain for all rows where descrizione_plain IS NULL by stripping HTML from descrizione (e.g. using MySQL 8's REGEXP_REPLACE(descrizione, '<[^>]+>', '') or another safe HTML-stripping expression), trimming the result and leaving NULLs alone; reference the libri table and descrizione_plain column in the migration so legacy rows are populated at migration-time rather than relying on BookRepository::getById() lazy backfill.app/Controllers/FrontendController.php (1)
955-955:⚠️ Potential issue | 🟠 MajorSubgenre selections still drop out of the sidebar tree.
These joins fix the
sg.nomereferences in the editor/availability branches, butgetFilterOptions()still builds the genre tree and itscntvalues froml.genere_idonly. A click on asottogenerelink from the book detail page now filters the result set, butgetDisplayGenres()still cannot resolve that selected node and falls back to level 0. Please includel.sottogenere_idand its ancestor ids inqueryGeneriand its count subquery as well.Also applies to: 987-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 955, getFilterOptions() and the queryGeneri/count subquery only consider l.genere_id so sottogenere selections drop out; update queryGeneri (and its inner count subquery) to include l.sottogenere_id and the ancestor IDs of that sottogenere so counts and selected-node resolution reflect sottogenere clicks. Specifically, modify the queryGeneri SQL generation used by getFilterOptions() and the COUNT(...) subquery to add l.sottogenere_id alongside l.genere_id, and ensure getDisplayGenres() can match the selected node by checking those sottogenere ancestor ids as well (references: queryGeneri, getFilterOptions(), getDisplayGenres(), l.sottogenere_id, cnt).app/Views/frontend/partials/social-sharing.php (1)
158-161:⚠️ Potential issue | 🟡 MinorHandle
writeText()rejection and fall back.Lines 158-161 only handle the success path. If clipboard permission is denied or the page loses activation, the promise rejects and the textarea fallback never runs, so copy-link silently fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 158 - 161, The clipboard writeText success path currently calls showCopyFeedback(btn) but ignores rejections; update the navigator.clipboard.writeText(url) call to attach a .catch handler that invokes the existing textarea fallback copy logic (create a temporary <textarea>, set its value to url, append, select, document.execCommand('copy'), then remove) and ensure showCopyFeedback(btn) is called on fallback success (and optionally on failure to provide consistent UI). Specifically modify the navigator.clipboard.writeText usage to navigator.clipboard.writeText(url).then(() => showCopyFeedback(btn)).catch(() => { /* run textarea fallback copy and call showCopyFeedback(btn) on success */ });app/Views/frontend/book-detail.php (1)
1549-1550:⚠️ Potential issue | 🟠 MajorRemove the legacy share UI before including the partial.
Line 1549 now adds a second sharing implementation because the old sidebar card is still present at Lines 2018-2043 and its handler still runs at Lines 2542-2643. The legacy controls also ignore
sharing.enabled_providers, so provider visibility can drift from the new partial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/book-detail.php` around lines 1549 - 1550, Remove the legacy share UI and its associated JS handler so the new partial (included via the social-sharing.php include) isn't duplicated and so provider visibility follows sharing.enabled_providers; specifically delete the old sidebar share card markup (the legacy sidebar card block) and remove the corresponding legacy share event handler code (the legacy share handler that binds to those controls), ensuring only the social-sharing.php partial and its handlers remain active.
🧹 Nitpick comments (4)
app/Support/ConfigStore.php (1)
107-109: Make the sharing-provider contract harder to misuse.
sharing.enabled_providersstays a raw CSV string here, butapp/Views/frontend/partials/social-sharing.phpconsumes an array. A small parser/dedicated accessor would prevent callers from accidentally passing the raw string through and iterating one character at a time.Also applies to: 573-578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/ConfigStore.php` around lines 107 - 109, The 'sharing.enabled_providers' config is stored as a raw CSV string but consumers like app/Views/frontend/partials/social-sharing.php expect an array; add a dedicated accessor on the ConfigStore (e.g., getSharingEnabledProviders or get('sharing.enabled_providers_array')) that parses the CSV into a trimmed, deduplicated array (ignore empty values) and use that accessor everywhere the array is required (replace direct access to 'sharing.enabled_providers' in the view and other call sites referenced around lines 573-578). Ensure the parser handles null/empty inputs and caches or normalizes the result so callers cannot accidentally iterate the raw string.app/Views/settings/index.php (1)
595-606: Avoid duplicating the provider registry in the view.The provider slugs and metadata now live here, while validation lives separately in
SettingsController::updateSharingSettings(). Extract a shared source of truth so labels, icons, and allowed values cannot drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/settings/index.php` around lines 595 - 606, The view defines the $allProviders registry which is duplicated against the validation in SettingsController::updateSharingSettings(); extract a single source of truth (either a config key like sharing.providers or a small provider registry class e.g. App\Support\SharingProviders with a static all()/slugs() API) and move the provider metadata there (labels, icons, colors, slugs). Update app/Views/settings/index.php to load providers from that shared source instead of the hardcoded $allProviders, and update SettingsController::updateSharingSettings() to derive its allowed values from the same shared source (use the registry’s slugs() or keys) so labels/icons and validation cannot drift.locale/en_US.json (1)
4042-4056: Avoid re-declaring existing locale keys.This block redefines keys that already exist earlier in the file (
"Anteprima","Condividi su Facebook","Condividi su WhatsApp","Copia link","Link copiato!","Condividi"). JSON loaders keep only one value per key, so this silently shadows the earlier entries and makes future translation drift easy to miss. Keep only the genuinely new keys here and reuse the existing shared ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` around lines 4042 - 4056, This block re-declares existing locale keys causing silent shadowing; remove the duplicated entries ("Anteprima", "Condividi su Facebook", "Condividi su WhatsApp", "Copia link", "Link copiato!", "Condividi") from this snippet and keep only the genuinely new keys (e.g. "Nessun pulsante selezionato", "Salva impostazioni condivisione", "Impostazioni di condivisione aggiornate.") so the file reuses the earlier translations instead of redefining them; verify by searching for the listed keys elsewhere and delete the duplicates here to avoid overriding the original entries.app/Views/frontend/partials/social-sharing.php (1)
16-80: Centralize the provider catalog.This array duplicates the provider slugs, icons, and colors already defined in
app/Views/settings/index.phpLines 596-606. Keeping two copies will eventually drift the settings UI and the frontend bar; move the provider map into one shared config/helper and have both views read from it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 16 - 80, The providers array defined as $providers in app/Views/frontend/partials/social-sharing.php duplicates the provider catalog in the settings view (settings/index.php); extract this catalog into a single shared source (e.g., a new config file or helper function such as getSocialProviders() or SocialProviders::all()) and have both views pull the provider map from that shared symbol instead of redefining it; update social-sharing.php to call the new shared provider accessor (replace the local $providers) and update settings/index.php to read from the same shared symbol, removing the duplicated hard-coded array in both views.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/SettingsController.php`:
- Around line 903-912: The current saving logic allows duplicate or
differently-cased slugs from the request to be stored (e.g. "email,email,X");
normalize $selected by trimming and lowercasing each entry, compute the
intersection with $allowedSlugs, then remove duplicates (e.g. with array_unique)
while preserving the canonical allowed order before imploding to $value; update
the same variables ($selected, $valid, $value) and continue to call
$repository->set('sharing','enabled_providers', $value) and
ConfigStore::set('sharing.enabled_providers', $value).
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 116-125: The Web Share button (class social-share-webapi) is
rendered unconditionally and will appear whenever navigator.share exists,
bypassing the $sharingProviders config; change the template to only output this
button when the admin has enabled a corresponding provider (e.g. check for a
'webshare' provider slug or that $sharingProviders contains 'webshare') so that
the DOM only includes the Web Share button when it's present in
$sharingProviders (and still keep the navigator.share runtime check in JS).
In `@installer/database/data_it_IT.sql`:
- Around line 328-331: The Italian seed leaves the description text in English
for the system_settings rows; update the two tuples so their description strings
are translated to Italian: change the description for key 'events_page_enabled'
("Enable events page in frontend") to an Italian equivalent and change the
description for key 'enabled_providers' ("Enabled social sharing providers on
book detail page") to Italian, keeping the setting keys, names and values
unchanged; ensure you update only the description field in the INSERT seed so
Italian installs show localized text.
In `@locale/it_IT.json`:
- Around line 81-103: The new PDF viewer and sharing localization keys (e.g.,
"Leggi PDF", "Condividi su Facebook", "Link copiato!") are present in it_IT but
missing in the other locale files; add the full set of 23 keys to the English
and German locale files and the routes_it locale so they no longer fall back to
Italian, using natural English phrases for en_US, appropriate German
translations for de_DE, and the same Italian strings for routes_it_IT, and
ensure each JSON file remains valid (proper commas/quotes) and keys match
exactly as shown in the it_IT snippet.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 321-325: In renderPdfViewer (method renderPdfViewer) the extension
check uses pathinfo on the raw URL so queries or signed params like
/file.pdf?download=1 won't match; replace that check by extracting the URL path
with parse_url($fileUrl, PHP_URL_PATH) and then run strtolower(pathinfo($path,
PATHINFO_EXTENSION)) === 'pdf' to detect PDFs, and apply the identical change to
the duplicate check in
storage/plugins/digital-library/views/frontend-pdf-viewer.php so both locations
normalize the URL path before checking the extension.
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 42-45: The anchor that opens $book['file_url'] with
target="_blank" in frontend-buttons.php must include rel="noopener noreferrer"
to prevent window.opener access; update the <a> that uses url($book['file_url'])
(the anchor with class "btn btn-outline-danger btn-lg") to add rel="noopener
noreferrer" for non-PDF external links (or always add it) so external targets
cannot access the originating window.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 198-245: registerHooks() currently does a read-then-insert against
plugin_hooks which allows race-condition duplicates; add a unique index on
(plugin_id, hook_name, callback_class, callback_method) (via migration or plugin
activation SQL) and replace the SELECT + INSERT flow with a single
upsert/insert-ignore prepared statement (e.g. INSERT ... ON DUPLICATE KEY UPDATE
or INSERT IGNORE) using the same bound parameters as the current INSERT block to
make registration atomic, remove the initial SELECT branch, and preserve
existing error logging and statement/connection cleanup for the new upsert
statement.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: getFilterOptions() and the queryGeneri/count subquery only consider
l.genere_id so sottogenere selections drop out; update queryGeneri (and its
inner count subquery) to include l.sottogenere_id and the ancestor IDs of that
sottogenere so counts and selected-node resolution reflect sottogenere clicks.
Specifically, modify the queryGeneri SQL generation used by getFilterOptions()
and the COUNT(...) subquery to add l.sottogenere_id alongside l.genere_id, and
ensure getDisplayGenres() can match the selected node by checking those
sottogenere ancestor ids as well (references: queryGeneri, getFilterOptions(),
getDisplayGenres(), l.sottogenere_id, cnt).
In `@app/Views/frontend/book-detail.php`:
- Around line 1549-1550: Remove the legacy share UI and its associated JS
handler so the new partial (included via the social-sharing.php include) isn't
duplicated and so provider visibility follows sharing.enabled_providers;
specifically delete the old sidebar share card markup (the legacy sidebar card
block) and remove the corresponding legacy share event handler code (the legacy
share handler that binds to those controls), ensuring only the
social-sharing.php partial and its handlers remain active.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 158-161: The clipboard writeText success path currently calls
showCopyFeedback(btn) but ignores rejections; update the
navigator.clipboard.writeText(url) call to attach a .catch handler that invokes
the existing textarea fallback copy logic (create a temporary <textarea>, set
its value to url, append, select, document.execCommand('copy'), then remove) and
ensure showCopyFeedback(btn) is called on fallback success (and optionally on
failure to provide consistent UI). Specifically modify the
navigator.clipboard.writeText usage to
navigator.clipboard.writeText(url).then(() => showCopyFeedback(btn)).catch(() =>
{ /* run textarea fallback copy and call showCopyFeedback(btn) on success */ });
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 14-28: The migration currently only adds the new column
descrizione_plain on table libri but does not backfill legacy rows, so
SearchController still searches raw HTML; update the migration to, after adding
descrizione_plain, run an UPDATE that sets descrizione_plain for all rows where
descrizione_plain IS NULL by stripping HTML from descrizione (e.g. using MySQL
8's REGEXP_REPLACE(descrizione, '<[^>]+>', '') or another safe HTML-stripping
expression), trimming the result and leaving NULLs alone; reference the libri
table and descrizione_plain column in the migration so legacy rows are populated
at migration-time rather than relying on BookRepository::getById() lazy
backfill.
---
Nitpick comments:
In `@app/Support/ConfigStore.php`:
- Around line 107-109: The 'sharing.enabled_providers' config is stored as a raw
CSV string but consumers like app/Views/frontend/partials/social-sharing.php
expect an array; add a dedicated accessor on the ConfigStore (e.g.,
getSharingEnabledProviders or get('sharing.enabled_providers_array')) that
parses the CSV into a trimmed, deduplicated array (ignore empty values) and use
that accessor everywhere the array is required (replace direct access to
'sharing.enabled_providers' in the view and other call sites referenced around
lines 573-578). Ensure the parser handles null/empty inputs and caches or
normalizes the result so callers cannot accidentally iterate the raw string.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 16-80: The providers array defined as $providers in
app/Views/frontend/partials/social-sharing.php duplicates the provider catalog
in the settings view (settings/index.php); extract this catalog into a single
shared source (e.g., a new config file or helper function such as
getSocialProviders() or SocialProviders::all()) and have both views pull the
provider map from that shared symbol instead of redefining it; update
social-sharing.php to call the new shared provider accessor (replace the local
$providers) and update settings/index.php to read from the same shared symbol,
removing the duplicated hard-coded array in both views.
In `@app/Views/settings/index.php`:
- Around line 595-606: The view defines the $allProviders registry which is
duplicated against the validation in
SettingsController::updateSharingSettings(); extract a single source of truth
(either a config key like sharing.providers or a small provider registry class
e.g. App\Support\SharingProviders with a static all()/slugs() API) and move the
provider metadata there (labels, icons, colors, slugs). Update
app/Views/settings/index.php to load providers from that shared source instead
of the hardcoded $allProviders, and update
SettingsController::updateSharingSettings() to derive its allowed values from
the same shared source (use the registry’s slugs() or keys) so labels/icons and
validation cannot drift.
In `@locale/en_US.json`:
- Around line 4042-4056: This block re-declares existing locale keys causing
silent shadowing; remove the duplicated entries ("Anteprima", "Condividi su
Facebook", "Condividi su WhatsApp", "Copia link", "Link copiato!", "Condividi")
from this snippet and keep only the genuinely new keys (e.g. "Nessun pulsante
selezionato", "Salva impostazioni condivisione", "Impostazioni di condivisione
aggiornate.") so the file reuses the earlier translations instead of redefining
them; verify by searching for the listed keys elsewhere and delete the
duplicates here to avoid overriding the original entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80d8742c-4873-4fca-a92e-47c3f8f92a6a
📒 Files selected for processing (25)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phpupdater.md
installer/database/data_it_IT.sql
Outdated
Comment on lines
+328
to
+331
| ('cms', 'events_page_enabled', '1', 'Enable events page in frontend', NOW()), | ||
|
|
||
| -- Social sharing settings | ||
| ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Enabled social sharing providers on book detail page', NOW()) |
There was a problem hiding this comment.
Translate the new setting descriptions in the Italian seed.
These description values are stored verbatim in system_settings, so the changed rows will keep English copy on Italian installs.
🌐 Suggested fix
-('cms', 'events_page_enabled', '1', 'Enable events page in frontend', NOW()),
+('cms', 'events_page_enabled', '1', 'Abilita la pagina eventi nel frontend', NOW()),
...
-('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Enabled social sharing providers on book detail page', NOW())
+('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Provider di condivisione social abilitati nella pagina dettaglio del libro', NOW())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ('cms', 'events_page_enabled', '1', 'Enable events page in frontend', NOW()), | |
| -- Social sharing settings | |
| ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Enabled social sharing providers on book detail page', NOW()) | |
| ('cms', 'events_page_enabled', '1', 'Abilita la pagina eventi nel frontend', NOW()), | |
| -- Social sharing settings | |
| ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email', 'Provider di condivisione social abilitati nella pagina dettaglio del libro', NOW()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/database/data_it_IT.sql` around lines 328 - 331, The Italian seed
leaves the description text in English for the system_settings rows; update the
two tuples so their description strings are translated to Italian: change the
description for key 'events_page_enabled' ("Enable events page in frontend") to
an Italian equivalent and change the description for key 'enabled_providers'
("Enabled social sharing providers on book detail page") to Italian, keeping the
setting keys, names and values unchanged; ensure you update only the description
field in the INSERT seed so Italian installs show localized text.
- Remove legacy hardcoded share card + JS from book-detail.php - Place configurable sharing partial in sidebar (card style) - Add .catch() to clipboard.writeText() for NotAllowedError fallback - Remove duplicate cms.events_page_enabled from installer SQL - Translate sharing description to Italian in data_it_IT.sql
The column was added via migrate_0.4.9.9.sql but missing from the base schema used by fresh installs. This caused the book search API to fail (COALESCE on non-existent column), breaking loan creation.
Installs between 0.4.9.9 and 0.5.0 may lack the column because schema.sql was not updated when the migration was introduced. The idempotent ALTER ensures upgraders get the column too.
- Add Threads, Bluesky, Tumblr, Pocket, VK, LINE, SMS providers - Fix og:type from 'website' to 'book' on book detail page - Fix og:url pointing to homepage instead of book URL - Add og:locale, book:isbn, book:author, book:release_date meta tags - Update Threads domain from threads.net to threads.com - Add translations for new providers (IT, EN, DE) - Update allowedSlugs in SettingsController - Document migrate_0.5.0 in updater.md and migrations README
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
app/Models/BookRepository.php (1)
96-108:⚠️ Potential issue | 🟠 MajorNormalize
descrizione_plainwith a helper instead of rawstrip_tags().These paths still flatten block content (
<p>Foo</p><p>Bar</p>→FooBar) and leave entities like&encoded, so search and CSV can still diverge from what users actually read.Suggested fix
- $plain = strip_tags((string)$row['descrizione']); + $plain = $this->toPlainTextDescription((string) $row['descrizione']); ... - $addField('descrizione_plain', 's', $raw !== null && $raw !== '' ? strip_tags((string)$raw) : $raw); + $addField('descrizione_plain', 's', $this->toPlainTextDescription($raw)); ... - $addSet('descrizione_plain', 's', $raw !== null && $raw !== '' ? strip_tags((string)$raw) : $raw); + $addSet('descrizione_plain', 's', $this->toPlainTextDescription($raw));private function toPlainTextDescription(?string $html): ?string { if ($html === null || $html === '') { return $html; } $text = preg_replace('/<(\/p|br\s*\/?)>/i', "\n", $html); $text = html_entity_decode(strip_tags((string) $text), ENT_QUOTES | ENT_HTML5, 'UTF-8'); $text = preg_replace("/[ \t]+/", ' ', (string) $text); $text = preg_replace("/\n{3,}/", "\n\n", (string) $text); return trim((string) $text); }Also applies to: 299-302, 629-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 96 - 108, Replace the inline strip_tags-based backfill with a reusable normalizer: create a private helper method (e.g. toPlainTextDescription(?string $html): ?string) and call it wherever descrizione_plain is computed/filled (the lazy backfill block that checks $this->hasColumn('descrizione_plain') and the other occurrences around lines noted). The helper should convert block tags to newlines, strip tags, html_entity_decode with ENT_QUOTES|ENT_HTML5 and UTF-8, collapse repeated whitespace and limit consecutive newlines, then trim and return the cleaned string; use that helper instead of raw strip_tags((string)$row['descrizione']) when assigning $plain and when populating descrizione_plain elsewhere.app/Controllers/SettingsController.php (1)
903-909:⚠️ Potential issue | 🟡 MinorCanonicalize and dedupe
sharing_providersbefore saving.This still preserves duplicate request values, so a crafted POST can store entries like
email,email. Normalize, unique, and intersect against the allowed list in canonical order before imploding.Suggested fix
$allowedSlugs = ['facebook', 'x', 'whatsapp', 'telegram', 'linkedin', 'reddit', 'pinterest', 'threads', 'bluesky', 'tumblr', 'pocket', 'vk', 'line', 'sms', 'email', 'copylink']; $selected = $data['sharing_providers'] ?? []; if (!is_array($selected)) { $selected = []; } - $valid = array_values(array_intersect($selected, $allowedSlugs)); + $selected = array_map( + static fn($slug) => strtolower(trim((string) $slug)), + $selected + ); + $valid = array_values(array_intersect($allowedSlugs, array_unique($selected))); $value = implode(',', $valid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SettingsController.php` around lines 903 - 909, The current logic in SettingsController that builds $value from $data['sharing_providers'] (variables $allowedSlugs, $selected, $valid, $value) allows duplicates and preserves request ordering; change it to normalize and dedupe the input before intersecting with the allowed list and imploding: ensure you convert each incoming slug to a canonical form (trim and lowercase), remove duplicates (array_unique), then intersect against $allowedSlugs in $allowedSlugs order and reindex (array_values) before joining into the final $value so duplicates like "email,email" cannot be stored.app/Controllers/FrontendController.php (1)
955-955:⚠️ Potential issue | 🟠 MajorThe filter tree still omits third-level subgenres.
These new
sgjoins fix the downstream publisher/availability queries, butgetFilterOptions()still builds$options['generi']andcntfroml.genere_idonly. A book saved asgenere_id = Narrativaandsottogenere_id = Fiabawill filter byFiaba, yetgetDisplayGenres()cannot findFiabain the generated hierarchy and falls back to level 0. Please includel.sottogenere_idand its ancestor chain in$queryGenerias well.Also applies to: 987-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 955, getFilterOptions() currently only adds l.genere_id to $queryGeneri so $options['generi'] and cnt miss books saved with sottogenere_id or deeper levels; update getFilterOptions() to also include l.sottogenere_id and walk its ancestor chain (parents up to root) into $queryGeneri so getDisplayGenres() can find Fiaba and other third-level subgenres and correctly build the hierarchy; ensure any existing logic that builds cnt uses the extended $queryGeneri set and reference the symbols getFilterOptions(), $queryGeneri, $options['generi'], cnt, getDisplayGenres(), and l.sottogenere_id when making the changes.storage/plugins/digital-library/views/frontend-buttons.php (1)
42-45:⚠️ Potential issue | 🟠 MajorAdd
relto the non-PDF_blanklink.
file_urlcan point to an external location, and this branch opens it in a new tab withoutrel="noopener noreferrer", leavingwindow.openeravailable to the destination.🔐 Minimal fix
<a href="<?= htmlspecialchars(url($book['file_url']), ENT_QUOTES, 'UTF-8') ?>" target="_blank" + rel="noopener noreferrer" class="btn btn-outline-danger btn-lg" title="<?= __("Scarica l'eBook in formato digitale") ?>">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/views/frontend-buttons.php` around lines 42 - 45, The anchor in storage/plugins/digital-library/views/frontend-buttons.php that renders the external download link (the <a> using <?= htmlspecialchars(url($book['file_url']) ...) ?> and target="_blank") should include rel="noopener noreferrer" to prevent window.opener being available to the destination; update the anchor rendering for the non-PDF download branch (the link that uses $book['file_url']) to add rel="noopener noreferrer" whenever target="_blank" is used.README.md (1)
31-36:⚠️ Potential issue | 🟡 MinorUpdate the plugin inventory entry too.
This section announces Digital Library Plugin
v1.3.0, but the plugin list later in the README still referencesdigital-library-v1.2.0.zip, which will confuse installers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 31 - 36, The README announces Digital Library Plugin v1.3.0 but the plugin inventory still points to digital-library-v1.2.0.zip; update the inventory entry by replacing the old filename "digital-library-v1.2.0.zip" with "digital-library-v1.3.0.zip" (and adjust any associated link text, download URL, or checksum entries that reference the 1.2.0 artifact) so the listed installer matches the announced v1.3.0 release.storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
321-325:⚠️ Potential issue | 🟡 MinorNormalize the URL before testing for
pathinfo()is being applied to the full URL here, so/file.pdf?download=1or signed storage URLs are treated as non-PDF and the inline viewer never mounts. Parse the path first, and keep the same normalization instorage/plugins/digital-library/views/frontend-buttons.phpandstorage/plugins/digital-library/views/frontend-pdf-viewer.phpso button rendering stays aligned.💡 Minimal fix
public function renderPdfViewer(array $book): void { - $fileUrl = $book['file_url'] ?? ''; - if (!empty($fileUrl) && strtolower(pathinfo($fileUrl, PATHINFO_EXTENSION)) === 'pdf') { + $fileUrl = (string) ($book['file_url'] ?? ''); + $filePath = (string) (parse_url($fileUrl, PHP_URL_PATH) ?? ''); + if ($filePath !== '' && strtolower(pathinfo($filePath, PATHINFO_EXTENSION)) === 'pdf') { include __DIR__ . '/views/frontend-pdf-viewer.php'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 321 - 325, The PDF check in renderPdfViewer uses pathinfo on the raw URL which fails for querystrings or signed URLs (e.g. /file.pdf?download=1); update renderPdfViewer(array $book) to parse the URL path first (use parse_url($fileUrl, PHP_URL_PATH) or equivalent) and run strtolower(pathinfo($path, PATHINFO_EXTENSION)) === 'pdf' against that path, and apply the same normalization (extracting the URL path before checking extension) in storage/plugins/digital-library/views/frontend-buttons.php and storage/plugins/digital-library/views/frontend-pdf-viewer.php so button rendering and viewer mounting remain consistent.app/Views/frontend/partials/social-sharing.php (1)
169-178:⚠️ Potential issue | 🟡 MinorGate the Web Share button behind the enabled-provider list.
This button is rendered outside the
$sharingProvidersloop, so any browser withnavigator.sharegets an extra share action even when admins did not enable it. Please add a dedicated provider slug (and matching admin checkbox) or suppress this DOM node unless it is configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 169 - 178, The Web Share API button (the button with class "social-share-webapi" and data attributes data-share-title/data-share-url) is rendered unconditionally outside of the $sharingProviders loop, causing it to appear even when not enabled; update the template to only output this DOM node when the Web Share provider is enabled (e.g., add a provider slug like "webapi" to the providers list and corresponding admin checkbox, then wrap the button with the same conditional used for other providers such as checking in_array('webapi', $sharingProviders) or the existing provider-enabled boolean) so the button is suppressed unless configured in $sharingProviders/admin settings.
🧹 Nitpick comments (2)
storage/plugins/digital-library/assets/css/digital-library.css (1)
217-251: Consider deduplicating the shared viewer container styles.Lines 217-251 copy the audiobook container/card styling almost verbatim, and the same pairing is now repeated again in the responsive and reduced-motion sections. A shared selector or utility class will make future tweaks land consistently in both viewers.
♻️ Possible consolidation
-#audiobook-player-container { +#audiobook-player-container, +#pdf-viewer-container { animation: slideDown 0.3s ease-out; } -#audiobook-player-container .card { +#audiobook-player-container .card, +#pdf-viewer-container .card { border: 2px solid `#e5e7eb`; border-radius: 16px; transition: box-shadow 0.3s ease, transform 0.3s ease; background: linear-gradient(135deg, `#ffffff` 0%, `#f9fafb` 100%); } -#audiobook-player-container .card:hover { +#audiobook-player-container .card:hover, +#pdf-viewer-container .card:hover { box-shadow: 0 8px 24px rgba(0, 0, 0, 0.12); transform: translateY(-2px); } - -#pdf-viewer-container { - animation: slideDown 0.3s ease-out; -} - -#pdf-viewer-container .card { - border: 2px solid `#e5e7eb`; - border-radius: 16px; - transition: box-shadow 0.3s ease, transform 0.3s ease; - background: linear-gradient(135deg, `#ffffff` 0%, `#f9fafb` 100%); -} - -#pdf-viewer-container .card:hover { - box-shadow: 0 8px 24px rgba(0, 0, 0, 0.12); - transform: translateY(-2px); -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/assets/css/digital-library.css` around lines 217 - 251, The CSS duplicates container/card/viewer styles across `#pdf-viewer-container` (and its .card, .pdf-viewer-frame, and .btn-danger rules); extract the common rules into a shared selector or utility class (e.g., .viewer-container and .viewer-card or a .viewer-btn-danger) and replace the duplicated blocks with that shared class in the stylesheet so both the PDF and audiobook viewers reference the same definitions; update hover/focus, responsive, and reduced-motion blocks to use the shared selector to ensure single-source styling for border, border-radius, background, transitions, box-shadow, frame sizing, and danger-button colors.updater.md (1)
761-768: Bundled plugin warning is documented but could be more prominent.The warning about bundled plugins not being updated by
manual-upgrade.phpis now documented, addressing the previous review concern. However, as suggested in the past review, moving this warning before the "Usage:" section (around line 748) with a more prominent marker would help users notice it before starting the upgrade process.This is a minor documentation improvement that can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater.md` around lines 761 - 768, Move the bundled-plugins warning block (the paragraph that states "⚠️ IMPORTANT: Bundled plugins are NOT updated by `manual-upgrade.php`" and the following bullets referring to `Updater.php`, `updateBundledPlugins()`, and `storage/plugins/<plugin-name>/`) so it appears before the "Usage:" section and make it visually more prominent (e.g., keep the ⚠️ marker and consider bolding or adding a short headline) so users see it prior to running manual-upgrade.php; ensure references to `manual-upgrade.php`, `Updater.php`, and `updateBundledPlugins()` remain intact and the guidance about extracting from the release ZIP is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locale/en_US.json`:
- Around line 4042-4063: The new sharing block reintroduces duplicate JSON
keys—specifically "Anteprima", "Condividi", "Condividi su Facebook", "Condividi
su WhatsApp", and "Link copiato!"—which must be removed; edit the block so you
only add the new provider-specific entries (e.g., "Condividi su X", "Condividi
su Telegram", "Condividi su LinkedIn", "Condividi su Reddit", "Condividi su
Pinterest", "Condividi su Threads", "Condividi su Bluesky", "Condividi su
Tumblr", "Salva su Pocket", "Condividi su VK", "Condividi su LINE", "Invia via
SMS", "Invia per email", "Copia link") and delete the duplicated keys listed
above, leaving the original occurrences elsewhere in the file intact.
In `@storage/plugins/digital-library/views/frontend-pdf-viewer.php`:
- Around line 11-14: The extension check using pathinfo on $fileUrl can be
thrown off by query strings (e.g. /file.pdf?token=...), so before calling
pathinfo or comparing the extension in the block that uses $fileUrl, strip any
query string or fragment (everything from '?' or '#') from $fileUrl, then run
strtolower(pathinfo($cleanUrl, PATHINFO_EXTENSION)) === 'pdf' (keep the existing
empty($fileUrl) guard); update the variable used in the check (the $fileUrl
reference that goes into pathinfo) so valid PDFs with query params are accepted.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: getFilterOptions() currently only adds l.genere_id to $queryGeneri
so $options['generi'] and cnt miss books saved with sottogenere_id or deeper
levels; update getFilterOptions() to also include l.sottogenere_id and walk its
ancestor chain (parents up to root) into $queryGeneri so getDisplayGenres() can
find Fiaba and other third-level subgenres and correctly build the hierarchy;
ensure any existing logic that builds cnt uses the extended $queryGeneri set and
reference the symbols getFilterOptions(), $queryGeneri, $options['generi'], cnt,
getDisplayGenres(), and l.sottogenere_id when making the changes.
In `@app/Controllers/SettingsController.php`:
- Around line 903-909: The current logic in SettingsController that builds
$value from $data['sharing_providers'] (variables $allowedSlugs, $selected,
$valid, $value) allows duplicates and preserves request ordering; change it to
normalize and dedupe the input before intersecting with the allowed list and
imploding: ensure you convert each incoming slug to a canonical form (trim and
lowercase), remove duplicates (array_unique), then intersect against
$allowedSlugs in $allowedSlugs order and reindex (array_values) before joining
into the final $value so duplicates like "email,email" cannot be stored.
In `@app/Models/BookRepository.php`:
- Around line 96-108: Replace the inline strip_tags-based backfill with a
reusable normalizer: create a private helper method (e.g.
toPlainTextDescription(?string $html): ?string) and call it wherever
descrizione_plain is computed/filled (the lazy backfill block that checks
$this->hasColumn('descrizione_plain') and the other occurrences around lines
noted). The helper should convert block tags to newlines, strip tags,
html_entity_decode with ENT_QUOTES|ENT_HTML5 and UTF-8, collapse repeated
whitespace and limit consecutive newlines, then trim and return the cleaned
string; use that helper instead of raw strip_tags((string)$row['descrizione'])
when assigning $plain and when populating descrizione_plain elsewhere.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 169-178: The Web Share API button (the button with class
"social-share-webapi" and data attributes data-share-title/data-share-url) is
rendered unconditionally outside of the $sharingProviders loop, causing it to
appear even when not enabled; update the template to only output this DOM node
when the Web Share provider is enabled (e.g., add a provider slug like "webapi"
to the providers list and corresponding admin checkbox, then wrap the button
with the same conditional used for other providers such as checking
in_array('webapi', $sharingProviders) or the existing provider-enabled boolean)
so the button is suppressed unless configured in $sharingProviders/admin
settings.
In `@README.md`:
- Around line 31-36: The README announces Digital Library Plugin v1.3.0 but the
plugin inventory still points to digital-library-v1.2.0.zip; update the
inventory entry by replacing the old filename "digital-library-v1.2.0.zip" with
"digital-library-v1.3.0.zip" (and adjust any associated link text, download URL,
or checksum entries that reference the 1.2.0 artifact) so the listed installer
matches the announced v1.3.0 release.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 321-325: The PDF check in renderPdfViewer uses pathinfo on the raw
URL which fails for querystrings or signed URLs (e.g. /file.pdf?download=1);
update renderPdfViewer(array $book) to parse the URL path first (use
parse_url($fileUrl, PHP_URL_PATH) or equivalent) and run
strtolower(pathinfo($path, PATHINFO_EXTENSION)) === 'pdf' against that path, and
apply the same normalization (extracting the URL path before checking extension)
in storage/plugins/digital-library/views/frontend-buttons.php and
storage/plugins/digital-library/views/frontend-pdf-viewer.php so button
rendering and viewer mounting remain consistent.
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 42-45: The anchor in
storage/plugins/digital-library/views/frontend-buttons.php that renders the
external download link (the <a> using <?=
htmlspecialchars(url($book['file_url']) ...) ?> and target="_blank") should
include rel="noopener noreferrer" to prevent window.opener being available to
the destination; update the anchor rendering for the non-PDF download branch
(the link that uses $book['file_url']) to add rel="noopener noreferrer" whenever
target="_blank" is used.
---
Nitpick comments:
In `@storage/plugins/digital-library/assets/css/digital-library.css`:
- Around line 217-251: The CSS duplicates container/card/viewer styles across
`#pdf-viewer-container` (and its .card, .pdf-viewer-frame, and .btn-danger rules);
extract the common rules into a shared selector or utility class (e.g.,
.viewer-container and .viewer-card or a .viewer-btn-danger) and replace the
duplicated blocks with that shared class in the stylesheet so both the PDF and
audiobook viewers reference the same definitions; update hover/focus,
responsive, and reduced-motion blocks to use the shared selector to ensure
single-source styling for border, border-radius, background, transitions,
box-shadow, frame sizing, and danger-button colors.
In `@updater.md`:
- Around line 761-768: Move the bundled-plugins warning block (the paragraph
that states "⚠️ IMPORTANT: Bundled plugins are NOT updated by
`manual-upgrade.php`" and the following bullets referring to `Updater.php`,
`updateBundledPlugins()`, and `storage/plugins/<plugin-name>/`) so it appears
before the "Usage:" section and make it visually more prominent (e.g., keep the
⚠️ marker and consider bolding or adding a short headline) so users see it prior
to running manual-upgrade.php; ensure references to `manual-upgrade.php`,
`Updater.php`, and `updateBundledPlugins()` remain intact and the guidance about
extracting from the release ZIP is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0edfcaa7-e240-41fd-b543-fd07b9c91425
📒 Files selected for processing (29)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jsupdater.md
locale/en_US.json
Outdated
Comment on lines
+4042
to
+4063
| "Anteprima": "Preview", | ||
| "Nessun pulsante selezionato": "No buttons selected", | ||
| "Salva impostazioni condivisione": "Save sharing settings", | ||
| "Impostazioni di condivisione aggiornate.": "Sharing settings updated.", | ||
| "Condividi su Facebook": "Share on Facebook", | ||
| "Condividi su X": "Share on X", | ||
| "Condividi su WhatsApp": "Share on WhatsApp", | ||
| "Condividi su Telegram": "Share on Telegram", | ||
| "Condividi su LinkedIn": "Share on LinkedIn", | ||
| "Condividi su Reddit": "Share on Reddit", | ||
| "Condividi su Pinterest": "Share on Pinterest", | ||
| "Condividi su Threads": "Share on Threads", | ||
| "Condividi su Bluesky": "Share on Bluesky", | ||
| "Condividi su Tumblr": "Share on Tumblr", | ||
| "Salva su Pocket": "Save to Pocket", | ||
| "Condividi su VK": "Share on VK", | ||
| "Condividi su LINE": "Share on LINE", | ||
| "Invia via SMS": "Send via SMS", | ||
| "Invia per email": "Send by email", | ||
| "Copia link": "Copy link", | ||
| "Link copiato!": "Link copied!", | ||
| "Condividi": "Share" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json
from collections import Counter
from pathlib import Path
keys = [
"Anteprima",
"Condividi",
"Condividi su Facebook",
"Condividi su WhatsApp",
"Copia link",
"Link copiato!",
]
pairs = json.loads(
Path("locale/en_US.json").read_text(encoding="utf-8"),
object_pairs_hook=list,
)
counts = Counter(k for k, _ in pairs)
for key in keys:
print(f"{key}: {counts[key]}")
PYRepository: fabiodalez-dev/Pinakes
Length of output: 172
Remove the duplicated locale keys from the new sharing block.
Lines 4042-4063 re-add Anteprima, Condividi, Condividi su Facebook, Condividi su WhatsApp, and Link copiato!, which already exist earlier in the same JSON object. Duplicate JSON keys are parser-dependent and make this locale file brittle for tooling and future edits. Keep the existing shared keys and add only the new provider-specific ones.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locale/en_US.json` around lines 4042 - 4063, The new sharing block
reintroduces duplicate JSON keys—specifically "Anteprima", "Condividi",
"Condividi su Facebook", "Condividi su WhatsApp", and "Link copiato!"—which must
be removed; edit the block so you only add the new provider-specific entries
(e.g., "Condividi su X", "Condividi su Telegram", "Condividi su LinkedIn",
"Condividi su Reddit", "Condividi su Pinterest", "Condividi su Threads",
"Condividi su Bluesky", "Condividi su Tumblr", "Salva su Pocket", "Condividi su
VK", "Condividi su LINE", "Invia via SMS", "Invia per email", "Copia link") and
delete the duplicated keys listed above, leaving the original occurrences
elsewhere in the file intact.
- SettingsController: dedup/canonicalize sharing slugs (array_unique + lowercase) - BookRepository: toPlainTextDescription() helper handles <p>, <br>, entities instead of raw strip_tags() - DigitalLibraryPlugin: atomic hook registration with INSERT...ON DUPLICATE KEY UPDATE, replacing race-prone SELECT+INSERT - schema.sql + migrate_0.5.0.sql: add UNIQUE KEY uk_plugin_hook_callback on plugin_hooks (plugin_id, hook_name, callback_class, callback_method) - frontend-buttons.php: add rel="noopener noreferrer" on target="_blank" link - frontend-pdf-viewer.php + DigitalLibraryPlugin + frontend-buttons.php: parse_url() before pathinfo() to handle query strings in URLs - locale/en_US.json + de_DE.json: remove duplicate keys (Anteprima, Condividi, Condividi su Facebook, Condividi su WhatsApp, Link copiato!) - README.md: update Digital Library plugin version 1.2.0 → 1.3.0 - settings/index.php: deep-linkable tabs via ?tab= and #hash with popstate - Add social-sharing.spec.js: 9 E2E tests covering settings, frontend buttons, OG meta tags, provider toggle, and share URL correctness
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/database/data_de_DE.sql (1)
277-282:⚠️ Potential issue | 🟡 MinorRe-add
cms.events_page_enabledto the German seed set.This block adds the new sharing setting, but
installer/database/data_de_DE.sqlno longer seedscms.events_page_enabledeven thoughapp/Views/frontend/layout.phpandEventsControllerstill read it, andinstaller/database/data_en_US.sqlstill provides that row. Freshde_DEinstalls will silently fall back to'1'instead of having the setting explicitly persisted like the other locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_de_DE.sql` around lines 277 - 282, The German seed no longer includes the cms.events_page_enabled row while app/Views/frontend/layout.php and EventsController still read it; restore the missing seed by adding an INSERT ... ON DUPLICATE KEY UPDATE row for namespace 'cms', key 'events_page_enabled' (value '1') with an appropriate German description and timestamps, matching the pattern used for other settings (same SQL structure as the existing 'sharing' seed) so de_DE installs persist the setting just like en_US.
🧹 Nitpick comments (2)
app/Views/settings/index.php (1)
595-613: Extract the sharing provider catalog to a shared source.This list now has to stay in sync with
SettingsController::updateSharingSettings()'s whitelist. If they drift, the UI can render options that the backend silently drops on save.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/settings/index.php` around lines 595 - 613, The hard-coded $allProviders catalog in the settings view is duplicated logic that must be centralized so it cannot drift from SettingsController::updateSharingSettings(); extract the provider catalog into a single shared source (e.g., a config entry or a static provider list method like App\Support\SharingProviders::all() or a config key 'sharing.providers'), update the view to read the list from that shared source instead of the local $allProviders variable, and change SettingsController::updateSharingSettings() to validate/whitelist against the same shared source rather than a separate hard-coded list; ensure the view still derives $enabledProviders from ConfigStore but uses the centralized catalog for rendering.tests/social-sharing.spec.js (1)
164-166: Consider adding explicit navigation for test isolation.This test relies on being on the book detail page from the previous test. While the serial execution makes this work, if test 4 fails, this test will fail with a confusing error. Adding a brief navigation step would make the test more robust:
♻️ Optional: Add explicit navigation
test('5. Frontend: share links have correct URLs', async () => { - // We should still be on a book detail page from previous test + // Ensure we're on a book detail page + if (!page.url().includes('/libro/')) { + await page.goto(`${BASE}/catalogo`); + await page.locator('.book-card a, .card a').filter({ hasText: /.+/ }).first().click(); + await page.waitForLoadState('networkidle'); + } const shareCard = page.locator('#book-share-card');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/social-sharing.spec.js` around lines 164 - 166, The test "5. Frontend: share links have correct URLs" relies on prior state; add an explicit navigation step at the start of that test to land on the book detail page (e.g., use page.goto to the known book detail URL or locate and click the book list item) before querying '#book-share-card' so the test is isolated and won't fail if the previous test errors; update the test body in tests/social-sharing.spec.js accordingly around the test declaration "test('5. Frontend: share links have correct URLs'..." to perform this navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Views/settings/index.php`:
- Around line 730-735: The popstate handler currently no-ops when no tab is
present, leaving the previous panel visible; update the handler to restore the
default tab by computing a fallback tab and calling activateTab for it: after
computing tab (const tab = ... || ''), if tab is truthy and the selector exists
call activateTab(tab) as before, otherwise look for a designated default tab
element (e.g., document.querySelector('[data-settings-tab][data-default]') ) and
if not found fall back to the first
document.querySelector('[data-settings-tab]') and call
activateTab(defaultTabName) using its data-settings-tab value so the UI matches
the URL on back navigation.
In `@installer/database/migrations/migrate_0.5.0.sql`:
- Around line 41-50: Migration tries to add unique key uk_plugin_hook_callback
on table plugin_hooks but doesn't remove existing duplicate rows first; update
the migration so that before the ALTER TABLE (and before preparing `@sql`) you
deduplicate plugin_hooks by deleting rows that share the same (plugin_id,
hook_name, callback_class, callback_method) while keeping a single authoritative
row (e.g., the row with the smallest id), then proceed to check `@idx_exists` and
PREPARE/EXECUTE the ALTER TABLE to add uk_plugin_hook_callback. Ensure the
dedupe targets exactly those four columns and is safe to run in the migration
context (wrap in transaction or safe delete using subquery that selects the
MIN(id) per group).
---
Outside diff comments:
In `@installer/database/data_de_DE.sql`:
- Around line 277-282: The German seed no longer includes the
cms.events_page_enabled row while app/Views/frontend/layout.php and
EventsController still read it; restore the missing seed by adding an INSERT ...
ON DUPLICATE KEY UPDATE row for namespace 'cms', key 'events_page_enabled'
(value '1') with an appropriate German description and timestamps, matching the
pattern used for other settings (same SQL structure as the existing 'sharing'
seed) so de_DE installs persist the setting just like en_US.
---
Nitpick comments:
In `@app/Views/settings/index.php`:
- Around line 595-613: The hard-coded $allProviders catalog in the settings view
is duplicated logic that must be centralized so it cannot drift from
SettingsController::updateSharingSettings(); extract the provider catalog into a
single shared source (e.g., a config entry or a static provider list method like
App\Support\SharingProviders::all() or a config key 'sharing.providers'), update
the view to read the list from that shared source instead of the local
$allProviders variable, and change SettingsController::updateSharingSettings()
to validate/whitelist against the same shared source rather than a separate
hard-coded list; ensure the view still derives $enabledProviders from
ConfigStore but uses the centralized catalog for rendering.
In `@tests/social-sharing.spec.js`:
- Around line 164-166: The test "5. Frontend: share links have correct URLs"
relies on prior state; add an explicit navigation step at the start of that test
to land on the book detail page (e.g., use page.goto to the known book detail
URL or locate and click the book list item) before querying '#book-share-card'
so the test is isolated and won't fail if the previous test errors; update the
test body in tests/social-sharing.spec.js accordingly around the test
declaration "test('5. Frontend: share links have correct URLs'..." to perform
this navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab8e3d48-f67f-4f61-ac05-513dd6eb53d1
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
- Deduplicate plugin_hooks rows before adding unique key (migrate_0.5.0) - Popstate handler restores default tab on back navigation (settings) - Add missing cms.events_page_enabled to German seed (data_de_DE) - Smaller share buttons (30px) with centered layout - Test 5 isolation: navigate to book page if not already there
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/views/frontend-buttons.php (1)
42-49:⚠️ Potential issue | 🟡 MinorUse a generic eBook icon in the non-PDF branch.
This path explicitly handles non-PDF files, but it still renders
fa-file-pdf, so EPUB/other formats are mislabeled in the UI.🎨 Minimal fix
- <i class="fas fa-file-pdf me-2"></i> + <i class="fas fa-book-open me-2"></i>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/views/frontend-buttons.php` around lines 42 - 49, The non-PDF branch in frontend-buttons.php renders a PDF icon (class "fas fa-file-pdf") even for EPUB/other formats; update the icon class in that anchor (the <a> that has class "btn btn-outline-danger btn-lg" and title __("Scarica l'eBook in formato digitale")) to a generic eBook/file icon (e.g., "fas fa-file-alt" or "fas fa-book") so the UI correctly represents non-PDF files.
♻️ Duplicate comments (1)
app/Controllers/FrontendController.php (1)
955-955:⚠️ Potential issue | 🟠 MajorSubgenre nodes are still absent from the filter tree.
Adding
sghere fixes the downstream queries, butgetFilterOptions()still builds$options['generi']and its counts froml.genere_idonly. Books reached throughl.sottogenere_idnever add that node or its ancestors to the tree, sogetDisplayGenres()still falls back to level 0 after clicking a third-level link.Also applies to: 987-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 955, The filter-tree misses subgenre nodes because getFilterOptions() builds $options['generi'] using only l.genere_id and ignores l.sottogenere_id (and its ancestor chain), so update getFilterOptions() to include the newly joined sg (sottogenere) values: aggregate counts from both l.genere_id and l.sottogenere_id (and map/propagate counts up each ancestor retrieved by getDisplayGenres()/genre hierarchy functions) so that nodes for sottogenere and their parent genres are created and their counts incremented; ensure the SQL/aggregation uses the sg alias added in the JOIN and that the code that constructs $options['generi'] merges/accumulates counts from sg into the same tree structure used by getDisplayGenres().
🧹 Nitpick comments (3)
installer/database/schema.sql (1)
364-364: Consider keeping the search indexes in sync withdescrizione_plain.Fresh installs now get the plain-text column, but the table definition still only FULLTEXT-indexes
descrizione. If the read path is moving towarddescrizione_plain, large-catalog searches will likely lose their existing index help until the schema/migration updates the supporting index too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` at line 364, The schema adds descrizione_plain but the table still only FULLTEXT-indexes descrizione; add a corresponding FULLTEXT index on descrizione_plain (and update any existing FULLTEXT index definitions to include both columns if needed) and update migration logic to create/rollback that index; ensure any data-migration or application write path that populates descrizione_plain also triggers index rebuilds/optimizations (or use a migration that backfills descrizione_plain and then recreates the fulltext index) so searches against descrizione_plain get the same index support as descrizione.installer/database/data_de_DE.sql (1)
189-191: Updateupdated_atin this upsert too.This seed still leaves
updated_atstale on duplicate rows, unlike the new settings block below. If these locale data scripts are rerun during upgrades, this entry becomes the odd one out for audit/sync metadata.Suggested patch
ON DUPLICATE KEY UPDATE - setting_value = VALUES(setting_value), description = VALUES(description); + setting_value = VALUES(setting_value), + description = VALUES(description), + updated_at = NOW();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_de_DE.sql` around lines 189 - 191, The INSERT for system_settings with setting_key 'events_page_enabled' doesn't update updated_at on duplicates; update the statement for the row (table: system_settings, key: 'events_page_enabled') to include the updated_at column in the INSERT VALUES (e.g. CURRENT_TIMESTAMP) and add updated_at = VALUES(updated_at) to the ON DUPLICATE KEY UPDATE clause so the timestamp is refreshed on upsert.app/Views/frontend/partials/social-sharing.php (1)
169-178: Web Share API button still renders unconditionally.The Web Share button is rendered outside the
$sharingProvidersloop and becomes visible whenevernavigator.shareexists, regardless of admin configuration. If this is intentional progressive enhancement, consider adding a comment. If it should respect config, add awebshareprovider slug and gate rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 169 - 178, The Web Share API button (button.social-share-webapi) is currently rendered unconditionally outside the $sharingProviders loop; update the template to respect configuration by gating its output on the presence of a 'webshare' provider slug in $sharingProviders (e.g., check in_array('webshare', $sharingProviders) or equivalent) so it only renders when enabled, and ensure the provider slug used by any config/state matches 'webshare' (or alternatively add an inline comment explaining this is intentional progressive enhancement if you prefer to keep it unconditional).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 404-407: The current SQL joins (aliases g, gp, gpp, sg) and the
filtering rely on matching generi.nome values from buildWhereConditions(), which
is unstable across branches; instead resolve the taxonomy to stable generi.id
values (using your existing taxonomy lookup that accepts (name, parent_id)) and
filter by those ids. Update the query joins (aliases g, gp, gpp, sg) or the
WHERE predicates to compare l.genere_id / l.sottogenere_id / parent id columns
to the resolved generi.id values returned by the taxonomy lookup, and adjust
buildWhereConditions() to return or accept ids rather than nome; apply the same
change for the other occurrence around lines 488-491 so filters use generi.id
instead of generi.nome.
In `@app/Controllers/LibriController.php`:
- Around line 2798-2807: In LibriController (around the CSV export where
$descrizione is set), strip_tags alone leaves HTML entities; decode entities
using html_entity_decode with ENT_QUOTES|ENT_HTML5 and UTF-8 either before or
after strip_tags (e.g., cast to string, run strip_tags, then
html_entity_decode(..., ENT_QUOTES|ENT_HTML5, 'UTF-8')) and assign back to
$descrizione so the CSV contains plain, decoded text; update the code that
builds $row (the $descrizione variable) accordingly.
In `@app/Models/BookRepository.php`:
- Around line 96-108: The lazy backfill in getById() leaves descrizione_plain
NULL until a detail page is opened, breaking searches; change the code so
searches don't rely on descrizione_plain alone and/or perform an eager backfill:
either (A) update the search/indexing logic to fall back to calling
toPlainTextDescription((string)$row['descrizione']) when descrizione_plain is
null (reference to toPlainTextDescription and descrizione_plain), or (B) add a
one-time backfill routine (or do it on write/update) that sets descrizione_plain
for all existing rows (use hasColumn and the same conversion used in getById) so
future searches always see plain text. Ensure you update the code paths that
build the search index or execute description queries to use the fallback or run
the migration.
- Around line 1175-1185: The toPlainTextDescription method only converts </p>
and <br> to newlines, causing block-level tags (like div, li, headings,
blockquote, table rows/cells) to collapse; update toPlainTextDescription to
first replace relevant block tags with separators (e.g., convert
</(p|div|h[1-6]|blockquote|tr|th|td|li|ul|ol|br\s*\/?)> to "\n" and convert
opening <li[^>]*> to "- " or "\n• ") before calling strip_tags so list items and
other blocks remain separated and readable in CSV and search. Ensure
html_entity_decode, whitespace collapsing, and trimming remain as-is after these
additional replacements so existing cleanup is preserved.
In `@app/Views/frontend/layout.php`:
- Line 132: The hreflang alternate links are using short locale query params
which the app doesn't handle; update the logic that builds the alternate URLs
(variables like $hreflangIt, $hreflangEn or wherever alternates are created,
referencing $baseUrlClean and $currentUrl) to use the full locale route format
/language/{locale} (e.g., it_IT, en_US) and include the redirect querystring
(redirect=urlencode($currentUrl)) so clicking an alternate triggers the app's
/language/{locale} route; ensure the chosen full locale strings match those
returned by I18n::getLocale() and your locale database.
In `@README.md`:
- Around line 39-45: The README overstates the immediate effect of the
"HTML-free search" because COALESCE(descrizione_plain, descrizione) will still
return raw HTML for existing rows until descrizione_plain is populated; update
the README text to soften the claim by explicitly stating that descrizione_plain
is added by migrate_0.4.9.9.sql and will be NULL for existing rows until
backfilled (via BookRepository on create/update), so searches fall back to
descrizione until descrizione_plain is populated.
In `@tests/social-sharing.spec.js`:
- Around line 199-205: The test gathers externalLinks via externalLinks =
shareCard.locator('a[target="_blank"]') and currently only asserts that each
link's rel contains 'noopener'; update the assertion so it also enforces
'noreferrer' (for example by checking rel includes 'noreferrer' or that rel
equals or contains both 'noopener' and 'noreferrer') for every link in the loop
(using the same externalLinks.nth(i) and rel variable) to prevent regressions
that drop the noreferrer token.
- Around line 298-318: The cleanup currently hardcodes enabling
facebook,x,whatsapp,email inside the test '9. Cleanup: restore default
providers', which can overwrite non-default environments or be skipped on
earlier failures; instead capture the original sharing configuration at the
start of the suite (e.g., in a beforeAll by reading checked states from the
locator '[data-settings-panel="sharing"]' for inputs with values
'facebook','x','whatsapp','email' into a small object or array), then move the
restore logic into afterAll where you iterate that saved state to set each
input's checked state and submit (use the same locators/panel and the submit
button used in this test), ensuring the suite always restores the pre-test
configuration regardless of intermediate test outcomes.
---
Outside diff comments:
In `@storage/plugins/digital-library/views/frontend-buttons.php`:
- Around line 42-49: The non-PDF branch in frontend-buttons.php renders a PDF
icon (class "fas fa-file-pdf") even for EPUB/other formats; update the icon
class in that anchor (the <a> that has class "btn btn-outline-danger btn-lg" and
title __("Scarica l'eBook in formato digitale")) to a generic eBook/file icon
(e.g., "fas fa-file-alt" or "fas fa-book") so the UI correctly represents
non-PDF files.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: The filter-tree misses subgenre nodes because getFilterOptions()
builds $options['generi'] using only l.genere_id and ignores l.sottogenere_id
(and its ancestor chain), so update getFilterOptions() to include the newly
joined sg (sottogenere) values: aggregate counts from both l.genere_id and
l.sottogenere_id (and map/propagate counts up each ancestor retrieved by
getDisplayGenres()/genre hierarchy functions) so that nodes for sottogenere and
their parent genres are created and their counts incremented; ensure the
SQL/aggregation uses the sg alias added in the JOIN and that the code that
constructs $options['generi'] merges/accumulates counts from sg into the same
tree structure used by getDisplayGenres().
---
Nitpick comments:
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 169-178: The Web Share API button (button.social-share-webapi) is
currently rendered unconditionally outside the $sharingProviders loop; update
the template to respect configuration by gating its output on the presence of a
'webshare' provider slug in $sharingProviders (e.g., check in_array('webshare',
$sharingProviders) or equivalent) so it only renders when enabled, and ensure
the provider slug used by any config/state matches 'webshare' (or alternatively
add an inline comment explaining this is intentional progressive enhancement if
you prefer to keep it unconditional).
In `@installer/database/data_de_DE.sql`:
- Around line 189-191: The INSERT for system_settings with setting_key
'events_page_enabled' doesn't update updated_at on duplicates; update the
statement for the row (table: system_settings, key: 'events_page_enabled') to
include the updated_at column in the INSERT VALUES (e.g. CURRENT_TIMESTAMP) and
add updated_at = VALUES(updated_at) to the ON DUPLICATE KEY UPDATE clause so the
timestamp is refreshed on upsert.
In `@installer/database/schema.sql`:
- Line 364: The schema adds descrizione_plain but the table still only
FULLTEXT-indexes descrizione; add a corresponding FULLTEXT index on
descrizione_plain (and update any existing FULLTEXT index definitions to include
both columns if needed) and update migration logic to create/rollback that
index; ensure any data-migration or application write path that populates
descrizione_plain also triggers index rebuilds/optimizations (or use a migration
that backfills descrizione_plain and then recreates the fulltext index) so
searches against descrizione_plain get the same index support as descrizione.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 624bfb03-4289-44bc-a507-7e1450f3a821
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
| <meta property="og:url" content="<?= htmlspecialchars($ogUrl) ?>"> | ||
| <meta property="og:type" content="<?= htmlspecialchars($ogType) ?>"> | ||
| <meta property="og:site_name" content="<?= HtmlHelper::e($appName) ?>"> | ||
| <meta property="og:locale" content="<?= htmlspecialchars(I18n::getLocale() ?: 'it_IT') ?>"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== Query-parameter locale handling =="
rg -n -C2 --glob '!vendor/**' --glob '!node_modules/**' "\['lang'\]|getQueryParams\(\).*lang|queryParams\['lang'\]|setLocale|I18n::getLocale|I18n::setLocale" app public installer tests || true
echo
echo "== Locale code literals =="
rg -n -C1 --glob '!vendor/**' --glob '!node_modules/**' "it_IT|en_US|de_DE|['\"]it['\"]|['\"]en['\"]|['\"]de['\"]" app public installer tests || trueRepository: fabiodalez-dev/Pinakes
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking app/Views/frontend/layout.php for alternate language URLs ==="
wc -l app/Views/frontend/layout.php
echo
head -200 app/Views/frontend/layout.php | tail -100
echo
echo "=== Checking where 'lang=' parameter is used in frontend/layout.php ==="
grep -n "lang=" app/Views/frontend/layout.php || echo "Not found in simple pattern"
echo
echo "=== Check I18n class for normalization ==="
head -100 app/Support/I18n.phpRepository: fabiodalez-dev/Pinakes
Length of output: 9472
🏁 Script executed:
#!/bin/bash
set -e
echo "=== I18n.php normalizeLocaleCode and setLocale methods ==="
rg -A 20 "function normalizeLocaleCode|public static function setLocale" app/Support/I18n.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1363
🏁 Script executed:
#!/bin/bash
set -e
echo "=== LanguageController language switching logic ==="
grep -n "lang\|setLocale\|getLocale" app/Controllers/LanguageController.php | head -40Repository: fabiodalez-dev/Pinakes
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Full LanguageController content ==="
cat app/Controllers/LanguageController.phpRepository: fabiodalez-dev/Pinakes
Length of output: 2166
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Check public/index.php for lang query parameter handling ==="
grep -n "lang\|locale\|queryParams\|_GET" public/index.php | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Search for where lang query parameter is processed ==="
rg -n "lang.*=|_GET\[.*lang|queryParams.*lang" app public installer --glob '!vendor/**' | head -30
echo
echo "=== Check language-switcher partial to see how it builds URLs ==="
cat app/Views/partials/language-switcher.phpRepository: fabiodalez-dev/Pinakes
Length of output: 11553
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Check routes to see language switching endpoint ==="
fd -type f -name "*.php" public installer | xargs grep -l "language\|route" | head -5
echo
echo "=== Check if any router file exists ==="
find . -maxdepth 3 -name "*route*" -o -name "*Router*" | grep -v vendor | grep -v node_modules | head -10Repository: fabiodalez-dev/Pinakes
Length of output: 431
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Check locale/routes files to understand routing ==="
head -50 locale/routes_it_IT.jsonRepository: fabiodalez-dev/Pinakes
Length of output: 1055
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Search for any handling of 'lang' query parameter in bootstrap ==="
grep -rn "\$_GET\|queryParams.*lang\|lang.*=\|lang=" public/index.php app/Support/ --include="*.php" | grep -v node_modules | head -20
echo
echo "=== Check if frontend/layout.php alternates actually work or are broken ==="
echo "Looking at alternate hreflang links in frontend/layout.php..."
sed -n '94,104p' app/Views/frontend/layout.php
</parameter>Repository: fabiodalez-dev/Pinakes
Length of output: 2366
Fix broken hreflang alternates: they use short locale codes that the app doesn't process.
The og:locale meta tag correctly outputs full locale codes (e.g., it_IT, en_US), but the hreflang alternates on lines 99–100 build URLs with short codes (lang=it, lang=en). The app doesn't process the lang query parameter; it uses the /language/{locale} route with full codes. Clicking these alternates won't switch the language.
Update alternates to match the routing system:
$hreflangIt = $baseUrlClean . '/language/it_IT?redirect=' . urlencode($currentUrl);
$hreflangEn = $baseUrlClean . '/language/en_US?redirect=' . urlencode($currentUrl);(Adjust locale codes to match your database and support full locale format throughout.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/frontend/layout.php` at line 132, The hreflang alternate links are
using short locale query params which the app doesn't handle; update the logic
that builds the alternate URLs (variables like $hreflangIt, $hreflangEn or
wherever alternates are created, referencing $baseUrlClean and $currentUrl) to
use the full locale route format /language/{locale} (e.g., it_IT, en_US) and
include the redirect querystring (redirect=urlencode($currentUrl)) so clicking
an alternate triggers the app's /language/{locale} route; ensure the chosen full
locale strings match those returned by I18n::getLocale() and your locale
database.
- Non-PDF eBook icon: fa-file-pdf → fa-book-open (frontend-buttons) - CSV export: html_entity_decode after strip_tags for clean output - toPlainTextDescription: handle div/li/h1-h6/blockquote/td + nbsp - German seed: add updated_at to events_page_enabled upsert - README: soften HTML-free search claim (COALESCE fallback noted) - Web Share API: add comment explaining progressive enhancement - Tests: assert noreferrer + restore original config in afterAll - Share buttons: left-aligned with gap-2
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
app/Controllers/LibriController.php (1)
2798-2803:⚠️ Potential issue | 🟡 MinorPreserve block separators in exported
descrizione.This still flattens HTML with bare
strip_tags(), so markup like<p>,<li>, or table cells can collapse intoFooBarin the CSV. Mirror the same block-aware normalization already used fordescrizione_plainbefore stripping tags.Also applies to: 2811-2811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 2798 - 2803, The exported $descrizione is currently flattened by strip_tags; instead reuse the same block-aware normalization applied to descrizione_plain: first replace block-level tags (<p>, <div>, <li>, <tr>, <td>, <th>, <br>, etc.) with appropriate newline or separator characters, collapse consecutive whitespace/newlines, then run strip_tags/html_entity_decode and trim; update the logic around $descrizione in LibriController (the block that sets $descrizione and the similar spot later) to call or replicate the descrizione_plain normalization routine so block boundaries are preserved in the CSV.app/Controllers/FrontendController.php (2)
955-955:⚠️ Potential issue | 🟠 MajorThe generated genre tree still omits
sottogenere_idnodes.These extra
sgjoins fix the downstream queries, butgetFilterOptions()still builds$options['generi']and its counts froml.genere_idonly. Third-level clicks can filter the result set, yetgetDisplayGenres()still cannot find that subgenre in the sidebar tree and falls back to the higher level.Also applies to: 987-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 955, getFilterOptions() is only aggregating counts by l.genere_id so third-level nodes (sottogenere_id) are missing from $options['generi'] and getDisplayGenres() cannot find subgenres; update getFilterOptions() to also include l.sottogenere_id (and sg alias) in the query and aggregation, add distinct entries for sottogenere_id into $options['generi'] with proper parent linkage to genere_id so counts reflect subgenre clicks, and ensure getDisplayGenres() can look up nodes by sottogenere_id when building the sidebar tree.
404-407:⚠️ Potential issue | 🟠 MajorFilter by stable genre IDs, not
nome.The new
sgjoins remove the SQL error, but the filter still keys offg.nome/gp.nome/gpp.nome/sg.nome. Different branches can reuse the same label, so one click can still pull books from the wrong subtree. Resolve the selected node to ageneri.idand compare against the stored genre/subgenre ids instead.Also applies to: 488-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 404 - 407, The SQL currently filters on g.nome/gp.nome/gpp.nome/sg.nome which can match duplicate labels; instead resolve the selected tree node to its generi.id and compare against the stored foreign keys (e.g., l.genere_id, l.sottogenere_id) using the joined aliases (g, gp, gpp, sg) rather than comparing names. Update the query-building logic in FrontendController (the block with LEFT JOIN generi g/gp/gpp/sg) to accept the selected node's id and apply WHERE clauses like g.id = :selected_id OR gp.id = :selected_id OR gpp.id = :selected_id OR sg.id = :selected_id (and similarly in the other query block referenced at lines ~488-491), ensuring parameters use the numeric generi.id instead of nome.
🧹 Nitpick comments (1)
app/Views/settings/index.php (1)
594-613: Avoid hardcoding the sharing catalog in the template.This provider list now has to stay aligned with the whitelist in
SettingsController::updateSharingSettings; if one side changes first, the admin can offer providers that the backend silently drops. A shared catalog/helper would keep preview, validation, and rendering in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/settings/index.php` around lines 594 - 613, The template is hardcoding the sharing catalog ($allProviders) which can drift from the whitelist used in SettingsController::updateSharingSettings; extract the provider catalog into a single source (e.g., a new SharingProviders::getAll() static helper or a config entry in ConfigStore) and replace the inline $allProviders and any whitelist/validation in SettingsController::updateSharingSettings to reference that same source; update the view to call that helper (instead of defining $allProviders) and adjust the controller validation to use SharingProviders::getAll() keys so rendering and backend validation remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/SearchController.php`:
- Around line 131-134: The query references l.descrizione_plain directly which
will fail if the migration hasn't run; modify SearchController to detect the
column at runtime (e.g., run a single lightweight check like SHOW COLUMNS FROM
libri LIKE 'descrizione_plain' or query INFORMATION_SCHEMA once on startup) and
then build the SQL accordingly: if the column exists use
COALESCE(l.descrizione_plain, l.descrizione) in the WHERE clause, otherwise use
l.descrizione; update the SQL-building sites (the shown WHERE/ORDER block and
the other occurrences at the ranges you noted around lines 238-241 and 351-354)
and ensure the prepared statement creation/bind_param calls (the $stmt and
bind_param usage) use the same parameter count and placeholders as the
dynamically-built SQL.
In `@installer/database/migrations/README.md`:
- Around line 86-89: Update the 0.5.0 changelog entry to include the
plugin_hooks schema change: describe that the migration deduplicates existing
rows in the plugin_hooks table and adds a unique constraint/index named
uk_plugin_hook_callback (idempotent migration behavior and any
INFORMATION_SCHEMA or INSERT/UPDATE safety-net semantics if applicable) so
operators know the migration alters plugin_hooks and creates
uk_plugin_hook_callback in addition to the libri and sharing changes.
In `@updater.md`:
- Around line 429-430: The current shipped version.json still reports "0.4.9.8"
which prevents migrate_0.4.9.9.sql from running; update version.json to
"0.4.9.9" so migrations newer than the installed version will execute. Edit
version.json to set the version field to 0.4.9.9, verify updater.md and
README.md references remain consistent, and ensure no other code or tests rely
on the old 0.4.9.8 value before committing.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: getFilterOptions() is only aggregating counts by l.genere_id so
third-level nodes (sottogenere_id) are missing from $options['generi'] and
getDisplayGenres() cannot find subgenres; update getFilterOptions() to also
include l.sottogenere_id (and sg alias) in the query and aggregation, add
distinct entries for sottogenere_id into $options['generi'] with proper parent
linkage to genere_id so counts reflect subgenre clicks, and ensure
getDisplayGenres() can look up nodes by sottogenere_id when building the sidebar
tree.
- Around line 404-407: The SQL currently filters on
g.nome/gp.nome/gpp.nome/sg.nome which can match duplicate labels; instead
resolve the selected tree node to its generi.id and compare against the stored
foreign keys (e.g., l.genere_id, l.sottogenere_id) using the joined aliases (g,
gp, gpp, sg) rather than comparing names. Update the query-building logic in
FrontendController (the block with LEFT JOIN generi g/gp/gpp/sg) to accept the
selected node's id and apply WHERE clauses like g.id = :selected_id OR gp.id =
:selected_id OR gpp.id = :selected_id OR sg.id = :selected_id (and similarly in
the other query block referenced at lines ~488-491), ensuring parameters use the
numeric generi.id instead of nome.
In `@app/Controllers/LibriController.php`:
- Around line 2798-2803: The exported $descrizione is currently flattened by
strip_tags; instead reuse the same block-aware normalization applied to
descrizione_plain: first replace block-level tags (<p>, <div>, <li>, <tr>, <td>,
<th>, <br>, etc.) with appropriate newline or separator characters, collapse
consecutive whitespace/newlines, then run strip_tags/html_entity_decode and
trim; update the logic around $descrizione in LibriController (the block that
sets $descrizione and the similar spot later) to call or replicate the
descrizione_plain normalization routine so block boundaries are preserved in the
CSV.
---
Nitpick comments:
In `@app/Views/settings/index.php`:
- Around line 594-613: The template is hardcoding the sharing catalog
($allProviders) which can drift from the whitelist used in
SettingsController::updateSharingSettings; extract the provider catalog into a
single source (e.g., a new SharingProviders::getAll() static helper or a config
entry in ConfigStore) and replace the inline $allProviders and any
whitelist/validation in SettingsController::updateSharingSettings to reference
that same source; update the view to call that helper (instead of defining
$allProviders) and adjust the controller validation to use
SharingProviders::getAll() keys so rendering and backend validation remain in
sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81d15745-e557-4c76-8037-0473cd7d942f
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
Comment on lines
+131
to
+134
| WHERE l.deleted_at IS NULL AND (l.titolo LIKE ? OR l.sottotitolo LIKE ? OR l.isbn10 LIKE ? OR l.isbn13 LIKE ? OR l.ean LIKE ? OR COALESCE(l.descrizione_plain, l.descrizione) LIKE ?) | ||
| ORDER BY l.titolo | ||
| "); | ||
| $stmt->bind_param('sssss', $s, $s, $s, $s, $s); | ||
| $stmt->bind_param('ssssss', $s, $s, $s, $s, $s, $s); |
There was a problem hiding this comment.
Guard descrizione_plain during rollout.
These queries now hard-reference l.descrizione_plain, but the rest of the PR still treats that column as optional. If an app node serves this code before the migration has run, these endpoints will fail with Unknown column 'l.descrizione_plain'. Build the predicate behind a schema check, or fall back to l.descrizione until the migration is guaranteed complete everywhere.
Also applies to: 238-241, 351-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/SearchController.php` around lines 131 - 134, The query
references l.descrizione_plain directly which will fail if the migration hasn't
run; modify SearchController to detect the column at runtime (e.g., run a single
lightweight check like SHOW COLUMNS FROM libri LIKE 'descrizione_plain' or query
INFORMATION_SCHEMA once on startup) and then build the SQL accordingly: if the
column exists use COALESCE(l.descrizione_plain, l.descrizione) in the WHERE
clause, otherwise use l.descrizione; update the SQL-building sites (the shown
WHERE/ORDER block and the other occurrences at the ranges you noted around lines
238-241 and 351-354) and ensure the prepared statement creation/bind_param calls
(the $stmt and bind_param usage) use the same parameter count and placeholders
as the dynamically-built SQL.
- CSV export: block-aware HTML→plaintext (mirrors toPlainTextDescription) - Migrations README: document plugin_hooks dedup + unique key
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
app/Controllers/FrontendController.php (2)
955-955:⚠️ Potential issue | 🟠 MajorThird-level genres still never enter the generated sidebar tree.
Adding
sghere fixes the downstream queries, butgetFilterOptions()still builds the genre hierarchy froml.genere_idonly. Books whose active leaf lives inl.sottogenere_idstill won't contribute that node or its ancestor path, so the selected subgenre can disappear after navigation.Also applies to: 987-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 955, getFilterOptions() builds the genre tree only from l.genere_id so books whose active leaf is in l.sottogenere_id are omitted; update getFilterOptions() to also consider l.sottogenere_id (the joined alias sg) when collecting genre IDs and when marking selected/active nodes so that both the leaf id and its ancestor path are included in the generated sidebar tree. Locate getFilterOptions() in FrontendController.php and ensure any queries, arrays, or loops that currently use genere_id also include sottogenere_id (and merge/deduplicate IDs) and propagate those IDs when building the hierarchy and selection state.
404-407:⚠️ Potential issue | 🟠 MajorResolve hierarchy clicks to stable genre IDs, not display names.
These joins fix the missing
sgalias, but the filter logic still matchesg.nome/gp.nome/gpp.nome/sg.nome. If the same label exists in multiple branches, clicking one hierarchy node can still return books from another branch. Resolve the selected node togeneri.idand filter on ids instead.Also applies to: 488-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 404 - 407, The query joins added the missing alias (g, gp, gpp, sg) but the filter still matches on g.nome/gp.nome/gpp.nome/sg.nome; change the controller logic that reads the clicked hierarchy node (where you currently compare names) to first resolve that selection to the corresponding generi.id and then filter the query by generi.id (using the aliases g.id, gp.id, gpp.id, sg.id) instead of nome. Update both occurrences (the block with LEFT JOIN g/gp/gpp/sg and the similar block around lines 488-491) so the WHERE clauses compare ids, not display names, and ensure the code path that maps a clicked node to an id uses the same generi table lookup before building the SQL.app/Views/frontend/partials/social-sharing.php (1)
169-180:⚠️ Potential issue | 🟡 MinorKeep Web Share behind the configured provider list.
This button is rendered outside
$sharingProviders, so supported browsers expose an extra share action even when the admin did not enable it. Either add an explicitwebshareslug to the shared provider registry or only render this button when that slug is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 169 - 180, The Web Share button is always rendered causing an extra option even if admins didn't enable it; restrict rendering to when the 'webshare' provider is enabled by checking your provider registry (e.g., the $sharingProviders array) before outputting the button (or alternatively add a 'webshare' slug to the shared provider registry and check for that slug). Locate the social-share-webapi button in social-sharing.php and wrap its output in a conditional that only renders when in_array('webshare', $sharingProviders) (or equivalent provider-check function) so the button appears only when the admin-enabled provider list includes 'webshare'.app/Views/settings/index.php (1)
716-727:⚠️ Potential issue | 🟡 MinorResolve invalid tabs on first load too.
With a stale or invalid
?tab=bookmark, the server can render no active panel and this branch no-ops, so the settings page loads with every panel hidden until the user clicks a tab. Reuse the same fallback-tab resolution here that you already added forpopstate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/settings/index.php` around lines 716 - 727, The initial-tab branch should mirror the popstate fallback resolution: determine the candidate tab from hash/qTab (initialTab), check if an element exists for that candidate, and if not pick the fallback (the same fallback used in your popstate handler — e.g., the explicit defaultTab or the first element found via document.querySelector('[data-settings-tab]') and its data-settings-tab attribute); then call activateTab(resolvedTab) and sync the URL using the resolvedTab. Ensure you reference/replace uses of initialTab with resolvedTab and keep activateTab and the URL sync logic intact.installer/database/migrations/migrate_0.4.9.9.sql (1)
14-28:⚠️ Potential issue | 🟠 MajorBackfill
descrizione_plainduring upgrade.This migration only adds the column. Existing catalogs keep
descrizione_plain = NULL, so search still falls back to raw HTML for untouched rows until each book happens to be read or edited. That leaves the new description-search behavior inconsistent on upgraded installs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.4.9.9.sql` around lines 14 - 28, The migration only adds libri.descrizione_plain but leaves existing rows NULL; update the migration (migrate_0.4.9.9.sql) to perform a one-time backfill after adding the column by populating descrizione_plain from descrizione using a tag-stripping SQL expression (e.g., MySQL 8+ REGEXP_REPLACE to remove <...> patterns) and trimming results, ensuring you update all rows where descrizione_plain IS NULL; reference the libri table and the descrizione and descrizione_plain column names when implementing the UPDATE so upgraded installs get consistent plain-text descriptions.
🧹 Nitpick comments (2)
installer/database/data_de_DE.sql (1)
189-199: Minor inconsistency in ON DUPLICATE KEY UPDATE clauses.The new
cms.events_page_enabledentry includesupdated_at = NOW()in its ON DUPLICATE KEY UPDATE clause, while the adjacentcatalogue_modeandpickup_expiry_daysentries (lines 193-199) do not. Consider aligning all entries to consistently updateupdated_atfor better audit trail tracking.♻️ Proposed fix for consistency
INSERT INTO `system_settings` (`category`, `setting_key`, `setting_value`, `description`) VALUES ('system', 'catalogue_mode', '0', 'Nur-Katalog-Modus - deaktiviert Ausleihen, Vormerkungen und Wunschliste') -ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), description = VALUES(description); +ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), description = VALUES(description), updated_at = NOW(); INSERT INTO `system_settings` (`category`, `setting_key`, `setting_value`, `description`) VALUES ('loans', 'pickup_expiry_days', '3', 'Tage zum Abholen einer genehmigten Ausleihe bevor sie verfällt') -ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), description = VALUES(description); +ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), description = VALUES(description), updated_at = NOW();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/data_de_DE.sql` around lines 189 - 199, The ON DUPLICATE KEY UPDATE clause for the 'system'::'catalogue_mode' and 'loans'::'pickup_expiry_days' INSERTs is missing updated_at = NOW() causing inconsistent audit behavior; update the ON DUPLICATE KEY UPDATE for the INSERTs that set setting_key = 'catalogue_mode' and setting_key = 'pickup_expiry_days' so they also include updated_at = NOW() alongside setting_value = VALUES(setting_value) and description = VALUES(description) to match the 'cms'::'events_page_enabled' entry.app/Controllers/SettingsController.php (1)
903-910: Centralize the sharing provider registry.This whitelist now has to stay in sync with the checkbox list in
app/Views/settings/index.phpand the provider map inapp/Views/frontend/partials/social-sharing.php. The next provider change can easily leave save validation, admin preview, and frontend rendering disagreeing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SettingsController.php` around lines 903 - 910, The hardcoded $allowedSlugs whitelist causes duplication; extract it into a single shared source (e.g., a config entry or a static provider registry method like SharingProviders::list() or a config/sharing.php that returns the array) and replace the local $allowedSlugs usage in SettingsController (where $selected, $valid, $value are computed) with a call to that shared source; keep the existing normalization (strtolower/trim and array_unique/array_intersect) behavior so validation output remains the same, and update the admin view and frontend partial to read the same shared list instead of duplicating the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/LibriController.php`:
- Around line 2798-2813: The current preg_replace that converts block-level HTML
to newlines only matches bare tags and misses tags with attributes (so <p
class="..."> or <br data-...> get stripped without a separator); update the
regex used on $rawDesc (the first preg_replace call) to also match these tags
with optional attributes and word boundaries (e.g. match
p|div|li|ul|ol|h1-6|blockquote|tr|th|td with \b and optional [^>]*, and br with
optional attributes/self-close) and keep the replacement as "\n" so paragraphs
and breaks are preserved before strip_tags() runs.
In `@installer/database/migrations/README.md`:
- Around line 92-95: Update the changelog wording to clarify that the new
libri.descrizione_plain column is populated lazily rather than fully backfilled
during the upgrade: change the sentence about the "PHP-based strip_tags backfill
for existing rows" to state that PHP backfill runs lazily (rows will be
populated over time) and mention that queries use a COALESCE fallback until
descrizione_plain is populated; reference the `descrizione_plain` column name
and the `COALESCE` fallback so operators understand immediate search behavior
after upgrade.
In `@tests/social-sharing.spec.js`:
- Around line 165-166: The locator used (const bookLink =
page.locator('.book-card a, .card a').filter({ hasText: /.+/ }).first()) is too
generic and can click author/category/action links; replace it with a
book-detail-specific selector or stable data-hook (e.g., use a selector like
'.book-card a[data-test="detail-link"]' or '.card [data-test="book-detail"]')
and update every occurrence referenced (the bookLink declarations at the current
location and also the ones around lines noted in the comment). Ensure you
replace the existing selector in the variable named bookLink and in the
equivalent locators at the other occurrences so the test consistently clicks the
detail page link.
- Around line 27-29: The restore can run with an empty originalProviders if
beforeAll fails mid-capture; change the capture to use a local temp array (e.g.,
tempOriginalProviders) or make originalProviders nullable and set a completed
flag (e.g., originalProvidersCaptured = false) inside beforeAll, assign the
final array to originalProviders and set the flag to true only after all
provider state (the page checks/reads) is successfully captured in beforeAll;
then in afterAll (the cleanup/restore block) only perform unchecking/restoration
if originalProvidersCaptured is true (or originalProviders is non-null and was
initialized by the successful capture), leaving originalProviders untouched on
partial failures to avoid overwriting the real state.
- Around line 144-157: The test currently only checks the preview state; modify
"3. Settings: preview updates live" to actually toggle a provider and assert the
preview updates: within the same panel locator
('[data-settings-panel="sharing"]') find and click the Facebook provider
checkbox (the provider checkbox element for Facebook), then assert that the
preview area ('#sharing-preview') shows or hides the Facebook icon selector
('.fab.fa-facebook-f, .fa-facebook-f') accordingly (use await clicks and an
expect on fbIcon visibility after the toggle to verify live update).
In `@updater.md`:
- Around line 1038-1039: The version listed in the changelog ("Digital Library
plugin v1.3.0") and the bundled-plugin entry ("digital-library-v1.0.0.zip") are
inconsistent; update the bundled-plugin table entry to reference the correct
artifact name/version (e.g., change "digital-library-v1.0.0.zip" to
"digital-library-v1.3.0.zip") or, if the artifact is actually v1.0.0, revert the
changelog line to v1.0.0 so both places match; locate the strings "Digital
Library plugin v1.3.0" and "digital-library-v1.0.0.zip" in updater.md and make
them consistent.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 955: getFilterOptions() builds the genre tree only from l.genere_id so
books whose active leaf is in l.sottogenere_id are omitted; update
getFilterOptions() to also consider l.sottogenere_id (the joined alias sg) when
collecting genre IDs and when marking selected/active nodes so that both the
leaf id and its ancestor path are included in the generated sidebar tree. Locate
getFilterOptions() in FrontendController.php and ensure any queries, arrays, or
loops that currently use genere_id also include sottogenere_id (and
merge/deduplicate IDs) and propagate those IDs when building the hierarchy and
selection state.
- Around line 404-407: The query joins added the missing alias (g, gp, gpp, sg)
but the filter still matches on g.nome/gp.nome/gpp.nome/sg.nome; change the
controller logic that reads the clicked hierarchy node (where you currently
compare names) to first resolve that selection to the corresponding generi.id
and then filter the query by generi.id (using the aliases g.id, gp.id, gpp.id,
sg.id) instead of nome. Update both occurrences (the block with LEFT JOIN
g/gp/gpp/sg and the similar block around lines 488-491) so the WHERE clauses
compare ids, not display names, and ensure the code path that maps a clicked
node to an id uses the same generi table lookup before building the SQL.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 169-180: The Web Share button is always rendered causing an extra
option even if admins didn't enable it; restrict rendering to when the
'webshare' provider is enabled by checking your provider registry (e.g., the
$sharingProviders array) before outputting the button (or alternatively add a
'webshare' slug to the shared provider registry and check for that slug). Locate
the social-share-webapi button in social-sharing.php and wrap its output in a
conditional that only renders when in_array('webshare', $sharingProviders) (or
equivalent provider-check function) so the button appears only when the
admin-enabled provider list includes 'webshare'.
In `@app/Views/settings/index.php`:
- Around line 716-727: The initial-tab branch should mirror the popstate
fallback resolution: determine the candidate tab from hash/qTab (initialTab),
check if an element exists for that candidate, and if not pick the fallback (the
same fallback used in your popstate handler — e.g., the explicit defaultTab or
the first element found via document.querySelector('[data-settings-tab]') and
its data-settings-tab attribute); then call activateTab(resolvedTab) and sync
the URL using the resolvedTab. Ensure you reference/replace uses of initialTab
with resolvedTab and keep activateTab and the URL sync logic intact.
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 14-28: The migration only adds libri.descrizione_plain but leaves
existing rows NULL; update the migration (migrate_0.4.9.9.sql) to perform a
one-time backfill after adding the column by populating descrizione_plain from
descrizione using a tag-stripping SQL expression (e.g., MySQL 8+ REGEXP_REPLACE
to remove <...> patterns) and trimming results, ensuring you update all rows
where descrizione_plain IS NULL; reference the libri table and the descrizione
and descrizione_plain column names when implementing the UPDATE so upgraded
installs get consistent plain-text descriptions.
---
Nitpick comments:
In `@app/Controllers/SettingsController.php`:
- Around line 903-910: The hardcoded $allowedSlugs whitelist causes duplication;
extract it into a single shared source (e.g., a config entry or a static
provider registry method like SharingProviders::list() or a config/sharing.php
that returns the array) and replace the local $allowedSlugs usage in
SettingsController (where $selected, $valid, $value are computed) with a call to
that shared source; keep the existing normalization (strtolower/trim and
array_unique/array_intersect) behavior so validation output remains the same,
and update the admin view and frontend partial to read the same shared list
instead of duplicating the array.
In `@installer/database/data_de_DE.sql`:
- Around line 189-199: The ON DUPLICATE KEY UPDATE clause for the
'system'::'catalogue_mode' and 'loans'::'pickup_expiry_days' INSERTs is missing
updated_at = NOW() causing inconsistent audit behavior; update the ON DUPLICATE
KEY UPDATE for the INSERTs that set setting_key = 'catalogue_mode' and
setting_key = 'pickup_expiry_days' so they also include updated_at = NOW()
alongside setting_value = VALUES(setting_value) and description =
VALUES(description) to match the 'cms'::'events_page_enabled' entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9321615f-bc6c-4cfb-8e1c-82b00e82800c
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
Comment on lines
+165
to
+166
| const bookLink = page.locator('.book-card a, .card a').filter({ hasText: /.+/ }).first(); | ||
| await bookLink.click(); |
There was a problem hiding this comment.
Use a book-detail-specific locator here.
These clicks target the first anchor anywhere inside a card, so they can latch onto author/category/action links instead of the detail page. Please switch to a dedicated detail-link selector or a stable data-* hook to keep the suite deterministic.
Also applies to: 202-203, 281-282, 325-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/social-sharing.spec.js` around lines 165 - 166, The locator used (const
bookLink = page.locator('.book-card a, .card a').filter({ hasText: /.+/
}).first()) is too generic and can click author/category/action links; replace
it with a book-detail-specific selector or stable data-hook (e.g., use a
selector like '.book-card a[data-test="detail-link"]' or '.card
[data-test="book-detail"]') and update every occurrence referenced (the bookLink
declarations at the current location and also the ones around lines noted in the
comment). Ensure you replace the existing selector in the variable named
bookLink and in the equivalent locators at the other occurrences so the test
consistently clicks the detail page link.
The getFilterOptions() genre tree only considered l.genere_id, missing books classified via l.sottogenere_id. This caused third-level genre nodes to not appear in the frontend sidebar when books were assigned through the subgenre field.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
app/Models/BookRepository.php (1)
1180-1184:⚠️ Potential issue | 🟡 MinorMatch block tags with attributes before stripping HTML.
This regex only catches bare tags like
<p>and<li>. Imported/editor HTML often includes attributes (<p class="...">,<li data-...>), so those separators are still dropped anddescrizione_plaincan collapse adjacent words, which hurts CSV readability and description search.🛠️ Suggested patch
- $text = preg_replace( - '/<(?:\/?(?:p|div|li|ul|ol|h[1-6]|blockquote|tr|th|td)|br\s*\/?)>/i', - "\n", - $html - ); + $text = preg_replace( + '/<(?:\/?(?:p|div|li|ul|ol|h[1-6]|blockquote|tr|th|td)\b[^>]*|br\s*\/?)>/i', + "\n", + $html + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 1180 - 1184, The current preg_replace in BookRepository.php (the call that assigns $text) only matches bare tags like <p> or <li> and misses tags with attributes, so strip/replace the block and br tags even when they have attributes; update the regex used in preg_replace (the pattern passed where $html is the subject) to allow optional attributes before the closing '>' (e.g. permit \s...[^>]* or a non-greedy attribute group) and still handle self-closing <br/> and closing tags, then run tests to ensure descrizione_plain retains proper separators.app/Controllers/SearchController.php (1)
131-134:⚠️ Potential issue | 🟠 MajorGuard
descrizione_plainuntil the migration is guaranteed complete.These queries now hard-reference
l.descrizione_plain. On any mixed rollout where code lands before the schema change, all three endpoints will fail withUnknown column 'l.descrizione_plain'. Build the predicate from a cached schema check, or fall back tol.descrizioneuntil the new column is guaranteed present.🛡️ Safer pattern
$descriptionExpr = $this->hasDescrizionePlain($db) ? 'COALESCE(l.descrizione_plain, l.descrizione)' : 'l.descrizione';-WHERE l.deleted_at IS NULL AND ( ... OR COALESCE(l.descrizione_plain, l.descrizione) LIKE ?) +WHERE l.deleted_at IS NULL AND ( ... OR {$descriptionExpr} LIKE ?)Also applies to: 238-241, 351-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SearchController.php` around lines 131 - 134, The query references l.descrizione_plain directly and will break if the column doesn't exist during rollout; add a cached schema check method (e.g., hasDescrizionePlain) on SearchController that queries the DB once and caches the boolean, then build a $descriptionExpr variable used in the SQL (use "COALESCE(l.descrizione_plain, l.descrizione)" when hasDescrizionePlain() returns true, otherwise use "l.descrizione"); replace all occurrences of the hard-coded COALESCE(l.descrizione_plain, l.descrizione) in the SQL strings (including the query that binds params via $stmt->bind_param and the other occurrences noted) to use the $descriptionExpr so the code falls back safely until the migration is present.app/Controllers/LibriController.php (1)
2798-2805:⚠️ Potential issue | 🟡 MinorParagraph separators still disappear for tags with attributes.
Line 2800 only replaces bare block tags. If
descrizionecontains<p class="...">or<br data-...>,strip_tags()will still collapse those boundaries into a single line in the CSV.♻️ Minimal fix
-$rawDesc = preg_replace('/<(?:\/?(?:p|div|li|ul|ol|h[1-6]|blockquote|tr|th|td)|br\s*\/?)>/i', "\n", $rawDesc); +$rawDesc = preg_replace( + '/<(?:\/?(?:p|div|li|ul|ol|h[1-6]|blockquote|tr|th|td)\b[^>]*|br\b[^>]*\/?)>/i', + "\n", + $rawDesc +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 2798 - 2805, The current block-aware HTML→plain text logic in LibriController.php uses preg_replace on $rawDesc to replace only bare block tags, so tags with attributes (e.g. <p class="..."> or <br data-...>) are not matched and paragraph boundaries get lost; update the preg_replace that targets block tags (the line replacing tags with "\n") to use a regex that also matches those tags with optional attributes and self-closing variants (apply to the preg_replace call that operates on $rawDesc before strip_tags), preserving the replacement of those tag occurrences with "\n" so paragraph separators are kept.installer/database/migrations/README.md (1)
92-95:⚠️ Potential issue | 🟡 MinorClarify that the
descrizione_plainbackfill is lazy.Line 95 still reads like existing rows are backfilled during the upgrade, but the documented behavior is lazy population with a
COALESCE(descrizione_plain, descrizione)fallback until rows are touched later. Tightening that wording will set the right expectation for post-upgrade search behavior.📝 Suggested wording
### 0.4.9.9 (2026-03-01) **HTML-free search column** - Added `descrizione_plain` TEXT column to `libri` (after descrizione) -- PHP-based strip_tags backfill for existing rows (not SQL — avoids backslash issues with splitSqlStatements) +- Plaintext values are populated lazily in PHP on later reads/writes; until then searches fall back via `COALESCE(descrizione_plain, descrizione)`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/README.md` around lines 92 - 95, Update the release note wording for the descrizione_plain backfill to state that population is lazy (existing rows are not immediately rewritten) and that search will use COALESCE(descrizione_plain, descrizione) as a fallback until individual rows are updated/touched; specifically, replace the current phrasing that implies an immediate PHP backfill with a clear statement that a PHP-based strip_tags routine exists for eventual backfill but that the upgrade leaves existing rows unchanged and relies on the COALESCE fallback for correct search behavior until rows are processed later.updater.md (1)
1038-1039:⚠️ Potential issue | 🟡 MinorVersion history still disagrees with the bundled-plugin checklist.
This entry says the Digital Library plugin is v1.3.0, but the bundled-plugin table above still lists
digital-library-v1.0.0.zip. One of those is stale, so the upgrade docs point maintainers at the wrong artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater.md` around lines 1038 - 1039, The release notes in the 0.5.0/0.4.9.9 entries reference Digital Library plugin v1.3.0 while the bundled-plugin table still lists digital-library-v1.0.0.zip; make them consistent by either updating the bundled-plugin table entry to digital-library-v1.3.0.zip (preferred) or reverting the changelog text to v1.0.0 so both places match; update the version string in updater.md to the chosen canonical plugin filename and run a quick scan of the document for any other digital-library-v* references to keep all mentions consistent.app/Controllers/FrontendController.php (1)
896-919:⚠️ Potential issue | 🟡 MinorDeeper
sottogenerenodes still drop out of the sidebar tree.This query now emits
sgids and ancestors, but Line 1525’sgetDisplayGenres()lookup still stops at root/child/grandchild. If a book usesgpp > gp > g > sg, selecting that fourth level falls back to level 0 instead of keeping the active branch visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 896 - 919, getDisplayGenres() currently only maps/display up to three levels (root/gp/g) so fourth-level sottogenere (sg) selections drop out; update getDisplayGenres() to accept and resolve sg ids and traverse their parent chain (sg -> g -> gp -> gpp) when building the sidebar active branch. Specifically, modify the lookup logic in getDisplayGenres() to include sg id handling and loop or recurse up parents until no parent_id is found (use parent_id on generi rows), and ensure the SQL/array that feeds getDisplayGenres() includes the emitted sg ids (from the query that aliases sottogenere as sg) so the function can mark the full ancestor chain active.app/Views/frontend/book-detail.php (1)
1458-1458:⚠️ Potential issue | 🟠 MajorUse a stable genre identifier in these catalog links.
These anchors still round-trip through Line 809’s name-based predicate, so duplicate labels in different branches collapse together. Clicking a node like
Fiabacan therefore load books from unrelated branches with the same name; pass an id/slug instead and filter on that.Also applies to: 1635-1635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/book-detail.php` at line 1458, The catalog links currently pass the genre name via the "genere" query and use $genreHierarchy names ($genreName), which causes collisions when labels duplicate; update the loop that builds anchors (using $genreHierarchy and $catalogRoute) to emit a stable identifier (genre ID or slug) in the query (e.g., genre_id or genre_slug) instead of the name, and adjust the catalog endpoint/server-side predicate that currently filters by name (the name-based predicate) to accept and filter by that stable ID/slug so each link selects the exact genre node.app/Views/frontend/partials/social-sharing.php (1)
169-180:⚠️ Potential issue | 🟡 MinorKeep the Web Share button behind the same settings surface.
This button is rendered outside the
$sharingProvidersloop, so browsers withnavigator.shareexpose an extra action even when admins did not enable it. Either gate it behind an explicitwebshareslug end-to-end or hide it when that slug is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 169 - 180, The Web Share button (element with class social-share-webapi using data-share-title/data-share-url and $shareTitle/$shareUrl) is rendered unconditionally and must be gated by the same settings as other providers; wrap or conditionally render this button only when the sharing providers list includes the 'webshare' slug (e.g., check $sharingProviders for 'webshare' or similar provider.slug) so it is output only when admins enabled that provider, or alternatively hide it when 'webshare' is absent; update the template to perform that presence check around the button rendering.tests/social-sharing.spec.js (3)
154-156:⚠️ Potential issue | 🟡 MinorExercise an actual preview change here.
This only checks the current DOM state. A broken
changehandler would still pass because the test never flips a provider.Make the test cover the live update path
- // Preview should show icons for checked providers - const fbIcon = preview.locator('.fab.fa-facebook-f, .fa-facebook-f'); - await expect(fbIcon).toBeVisible(); + const facebook = panel.locator('input[value="facebook"]'); + const fbIcon = preview.locator('.fab.fa-facebook-f, .fa-facebook-f'); + const wasChecked = await facebook.isChecked(); + + await facebook.setChecked(!wasChecked); + if (wasChecked) { + await expect(fbIcon).toHaveCount(0); + } else { + await expect(fbIcon).toBeVisible(); + } + + await facebook.setChecked(wasChecked);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/social-sharing.spec.js` around lines 154 - 156, Test currently only inspects DOM state; update it to exercise the live update path by toggling the provider input and asserting preview updates: locate the provider checkbox/radio that controls Facebook (the same UI element the change handler listens to), simulate a user toggle (click/check and then uncheck or vice versa) to trigger the change handler, and assert that preview.locator('.fab.fa-facebook-f, .fa-facebook-f') becomes visible after checking and not visible after unchecking (or vice versa depending on initial state); use the existing preview and fbIcon identifiers to keep changes minimal.
27-29:⚠️ Potential issue | 🟠 MajorDon’t restore unless the original provider snapshot completed.
If
beforeAllthrows after Line 32 createspagebut before Lines 43-50 finish,originalProvidersstays empty or partial andafterAllwill save that bad state back to admin settings.Minimal hardening
- /** `@type` {string[]} Original checked provider slugs — captured for restore */ - let originalProviders = []; + /** `@type` {string[] | null} Original checked provider slugs — captured for restore */ + let originalProviders = null; @@ - const inputs = page.locator('[data-settings-panel="sharing"] input[type="checkbox"][name="sharing_providers[]"]'); - const count = await inputs.count(); - for (let i = 0; i < count; i++) { - if (await inputs.nth(i).isChecked()) { - const value = await inputs.nth(i).getAttribute('value'); - if (value) originalProviders.push(value); - } - } + const capturedProviders = []; + const inputs = page.locator('[data-settings-panel="sharing"] input[type="checkbox"][name="sharing_providers[]"]'); + const count = await inputs.count(); + for (let i = 0; i < count; i++) { + if (await inputs.nth(i).isChecked()) { + const value = await inputs.nth(i).getAttribute('value'); + if (value) capturedProviders.push(value); + } + } + originalProviders = capturedProviders; @@ - if (page) { + if (page && originalProviders !== null) {Also applies to: 40-50, 53-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/social-sharing.spec.js` around lines 27 - 29, beforeAll can fail after creating page and leave originalProviders empty/partial, so add a guard to avoid restoring a bad snapshot: capture originalProviders into the existing let originalProviders = [] only after the full snapshot logic completes (e.g., set a boolean flag originalProvidersCaptured = true at the end of the successful snapshot) and in afterAll check originalProvidersCaptured before writing settings back; update the beforeAll snapshot flow (where page is created and provider slugs are read) to set the flag only on success and ensure afterAll skips restore unless originalProvidersCaptured is true, referencing the beforeAll/afterAll hooks and the originalProviders variable.
165-166:⚠️ Potential issue | 🟠 MajorUse a detail-link-specific locator instead of the first anchor in the card.
These selectors can match author/category/action links, so the suite becomes nondeterministic and can miss the book detail page entirely. Switch them to a stable detail-link hook or a dedicated detail-page selector.
Also applies to: 202-203, 281-282, 325-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/social-sharing.spec.js` around lines 165 - 166, The locator for book links is too generic (page.locator('.book-card a, .card a').filter(...)) and can match non-detail anchors; replace it with a stable detail-link-specific selector/hook (e.g., a dedicated .detail-link or data-testid/detail-link attribute) wherever the variable bookLink (and the analogous locators at the other occurrences) is created so the test always clicks the card's detail link; update the three other instances you flagged (the same pattern at the other lines) to use the new detail-link selector and keep the subsequent .click() calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Views/frontend/layout.php`:
- Around line 133-136: The layout echoes Open Graph tags from $ogBookMeta
without validating entries; update the template loop that iterates $ogBookMeta
(the foreach over $bm) to skip malformed items by checking each $bm is an array
and that 'property' and 'content' keys exist and are non-empty (and cast to
string) before calling htmlspecialchars and rendering the <meta> tag;
effectively normalize/validate $bm in the loop and continue to next item when
validation fails to avoid PHP notices.
In `@app/Views/settings/index.php`:
- Around line 617-630: The provider card styling isn't updated when a checkbox
is toggled because the label's selected classes are only set on initial render;
add a small JS handler that on DOMContentLoaded and on "change" for inputs with
class "sharing-provider-checkbox" (or via event delegation) reads input.checked
(using the input's data-slug/data-* attributes as needed) and toggles the label
classes between the selected state ("border-gray-400 bg-white") and unselected
state ("border-gray-200 bg-white hover:border-gray-300"); place this logic in a
function (e.g., syncSharingProviderCardState) and call it both at load and from
the checkbox change listener so the visual card border/background always matches
the checkbox state.
In `@README.md`:
- Around line 47-50: Update the "CSV export fix" release note line to accurately
describe the actual change: replace the current "Export with selected IDs now
works correctly" text with a concise mention that the CSV export reorders the
descrizione field and strips HTML tags from its contents (e.g., "CSV export:
descrizione field reordered and HTML stripped"), ensuring the bullet references
"descrizione" and HTML stripping instead of selected-ID export so readers see
the actual PR behavior.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Around line 896-919: getDisplayGenres() currently only maps/display up to
three levels (root/gp/g) so fourth-level sottogenere (sg) selections drop out;
update getDisplayGenres() to accept and resolve sg ids and traverse their parent
chain (sg -> g -> gp -> gpp) when building the sidebar active branch.
Specifically, modify the lookup logic in getDisplayGenres() to include sg id
handling and loop or recurse up parents until no parent_id is found (use
parent_id on generi rows), and ensure the SQL/array that feeds
getDisplayGenres() includes the emitted sg ids (from the query that aliases
sottogenere as sg) so the function can mark the full ancestor chain active.
In `@app/Controllers/LibriController.php`:
- Around line 2798-2805: The current block-aware HTML→plain text logic in
LibriController.php uses preg_replace on $rawDesc to replace only bare block
tags, so tags with attributes (e.g. <p class="..."> or <br data-...>) are not
matched and paragraph boundaries get lost; update the preg_replace that targets
block tags (the line replacing tags with "\n") to use a regex that also matches
those tags with optional attributes and self-closing variants (apply to the
preg_replace call that operates on $rawDesc before strip_tags), preserving the
replacement of those tag occurrences with "\n" so paragraph separators are kept.
In `@app/Controllers/SearchController.php`:
- Around line 131-134: The query references l.descrizione_plain directly and
will break if the column doesn't exist during rollout; add a cached schema check
method (e.g., hasDescrizionePlain) on SearchController that queries the DB once
and caches the boolean, then build a $descriptionExpr variable used in the SQL
(use "COALESCE(l.descrizione_plain, l.descrizione)" when hasDescrizionePlain()
returns true, otherwise use "l.descrizione"); replace all occurrences of the
hard-coded COALESCE(l.descrizione_plain, l.descrizione) in the SQL strings
(including the query that binds params via $stmt->bind_param and the other
occurrences noted) to use the $descriptionExpr so the code falls back safely
until the migration is present.
In `@app/Models/BookRepository.php`:
- Around line 1180-1184: The current preg_replace in BookRepository.php (the
call that assigns $text) only matches bare tags like <p> or <li> and misses tags
with attributes, so strip/replace the block and br tags even when they have
attributes; update the regex used in preg_replace (the pattern passed where
$html is the subject) to allow optional attributes before the closing '>' (e.g.
permit \s...[^>]* or a non-greedy attribute group) and still handle self-closing
<br/> and closing tags, then run tests to ensure descrizione_plain retains
proper separators.
In `@app/Views/frontend/book-detail.php`:
- Line 1458: The catalog links currently pass the genre name via the "genere"
query and use $genreHierarchy names ($genreName), which causes collisions when
labels duplicate; update the loop that builds anchors (using $genreHierarchy and
$catalogRoute) to emit a stable identifier (genre ID or slug) in the query
(e.g., genre_id or genre_slug) instead of the name, and adjust the catalog
endpoint/server-side predicate that currently filters by name (the name-based
predicate) to accept and filter by that stable ID/slug so each link selects the
exact genre node.
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 169-180: The Web Share button (element with class
social-share-webapi using data-share-title/data-share-url and
$shareTitle/$shareUrl) is rendered unconditionally and must be gated by the same
settings as other providers; wrap or conditionally render this button only when
the sharing providers list includes the 'webshare' slug (e.g., check
$sharingProviders for 'webshare' or similar provider.slug) so it is output only
when admins enabled that provider, or alternatively hide it when 'webshare' is
absent; update the template to perform that presence check around the button
rendering.
In `@installer/database/migrations/README.md`:
- Around line 92-95: Update the release note wording for the descrizione_plain
backfill to state that population is lazy (existing rows are not immediately
rewritten) and that search will use COALESCE(descrizione_plain, descrizione) as
a fallback until individual rows are updated/touched; specifically, replace the
current phrasing that implies an immediate PHP backfill with a clear statement
that a PHP-based strip_tags routine exists for eventual backfill but that the
upgrade leaves existing rows unchanged and relies on the COALESCE fallback for
correct search behavior until rows are processed later.
In `@tests/social-sharing.spec.js`:
- Around line 154-156: Test currently only inspects DOM state; update it to
exercise the live update path by toggling the provider input and asserting
preview updates: locate the provider checkbox/radio that controls Facebook (the
same UI element the change handler listens to), simulate a user toggle
(click/check and then uncheck or vice versa) to trigger the change handler, and
assert that preview.locator('.fab.fa-facebook-f, .fa-facebook-f') becomes
visible after checking and not visible after unchecking (or vice versa depending
on initial state); use the existing preview and fbIcon identifiers to keep
changes minimal.
- Around line 27-29: beforeAll can fail after creating page and leave
originalProviders empty/partial, so add a guard to avoid restoring a bad
snapshot: capture originalProviders into the existing let originalProviders = []
only after the full snapshot logic completes (e.g., set a boolean flag
originalProvidersCaptured = true at the end of the successful snapshot) and in
afterAll check originalProvidersCaptured before writing settings back; update
the beforeAll snapshot flow (where page is created and provider slugs are read)
to set the flag only on success and ensure afterAll skips restore unless
originalProvidersCaptured is true, referencing the beforeAll/afterAll hooks and
the originalProviders variable.
- Around line 165-166: The locator for book links is too generic
(page.locator('.book-card a, .card a').filter(...)) and can match non-detail
anchors; replace it with a stable detail-link-specific selector/hook (e.g., a
dedicated .detail-link or data-testid/detail-link attribute) wherever the
variable bookLink (and the analogous locators at the other occurrences) is
created so the test always clicks the card's detail link; update the three other
instances you flagged (the same pattern at the other lines) to use the new
detail-link selector and keep the subsequent .click() calls unchanged.
In `@updater.md`:
- Around line 1038-1039: The release notes in the 0.5.0/0.4.9.9 entries
reference Digital Library plugin v1.3.0 while the bundled-plugin table still
lists digital-library-v1.0.0.zip; make them consistent by either updating the
bundled-plugin table entry to digital-library-v1.3.0.zip (preferred) or
reverting the changelog text to v1.0.0 so both places match; update the version
string in updater.md to the chosen canonical plugin filename and run a quick
scan of the document for any other digital-library-v* references to keep all
mentions consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 670a8692-7afc-48ea-a7c2-f2d51f6a902e
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
- Regex: match block tags with attributes (<p class="...">) in HTML→plaintext - OG meta: skip malformed entries in layout.php loop - Settings: fallback to first tab on stale/invalid ?tab= bookmark - Settings: sync card border styling on provider checkbox toggle - Tests: guard originalProviders restore with null sentinel - Tests: exercise actual live preview toggle in test 3 - README: accurate CSV export bullet description - updater.md: fix digital-library plugin version (v1.0.0 → v1.3.0) - data_de_DE.sql: add updated_at = NOW() to ON DUPLICATE KEY clauses - migrations/README: clarify lazy backfill wording for 0.4.9.9
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
app/Views/frontend/partials/social-sharing.php (1)
169-180:⚠️ Potential issue | 🟡 MinorWeb Share still bypasses the admin-enabled provider list.
This button is rendered outside the
$sharingProvidersloop, so supported browsers will surface an extra share action even when admins disabled it. Gate it behind an explicit provider slug or remove it from the configurable toolbar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/partials/social-sharing.php` around lines 169 - 180, The Web Share button is rendered unconditionally and bypasses the admin-controlled $sharingProviders list; update the template to only render the button when the Web Share provider is enabled (e.g., check in_array('webapi', $sharingProviders) or similar provider slug) or remove the standalone social-share-webapi button from the toolbar so share actions follow the configured $sharingProviders loop (reference the $sharingProviders variable and the social-share-webapi button/ CSS class to locate the code).installer/database/migrations/migrate_0.4.9.9.sql (1)
14-28:⚠️ Potential issue | 🟠 MajorBackfill is still deferred to first read, so upgraded rows remain searchable as raw HTML.
This migration only creates
libri.descrizione_plain. Existing rows keepdescrizione_plain = NULLuntilapp/Models/BookRepository.phptouches them on insert/update orgetById(), soapp/Controllers/SearchController.phpstill falls back tol.descrizionefor untouched books right after upgrade. That leaves description search noisy/incomplete for legacy catalog data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.4.9.9.sql` around lines 14 - 28, Migration only adds libri.descrizione_plain but doesn't backfill existing rows, so SearchController::getById fallback keeps raw HTML searchable; modify the migration (migrate_0.4.9.9.sql) to, after adding the column (the ALTER TABLE block that references descrizione_plain), run an UPDATE to backfill existing rows where descrizione_plain IS NULL by stripping HTML from descrizione (use MySQL REGEXP_REPLACE(descrizione,'<[^>]+>','') or an equivalent DB-safe expression) and set descrizione_plain = TRIM(...) so legacy rows are searchable immediately; keep the conditional column-exists logic and execute the UPDATE only when the column is newly created to avoid altering existing data.app/Controllers/FrontendController.php (1)
404-407:⚠️ Potential issue | 🟠 MajorThe new
sgjoins fix the 500, but genre filtering is still keyed by display name.
buildWhereConditions()still matchesg.nome / gp.nome / gpp.nome / sg.nome. If the same label exists in multiple branches, clicking one hierarchy link can still pull books from an unrelated branch. Please resolve the clicked node to a stablegeneri.idand filter by IDs/ancestor IDs instead of names.Also applies to: 488-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 404 - 407, buildWhereConditions() currently filters genres by matching g.nome/gp.nome/gpp.nome/sg.nome (display names), which is ambiguous; change it to resolve the clicked node to its canonical generi.id and ancestor IDs and filter by those IDs instead. Specifically: when a genre/hierarchy link is clicked, lookup the generi row to obtain its id and parent chain, then in the WHERE construction use g.id, gp.id, gpp.id, sg.id (or IN (ancestor_ids)) rather than comparing nome; update any code paths that accept a genre slug/name to first translate it to the generi.id, and modify the SQL/ORM filters built in buildWhereConditions() to use numeric ID comparisons (or an ancestors array) for both the main branch and the secondary join aliases (g, gp, gpp, sg).tests/social-sharing.spec.js (1)
172-173:⚠️ Potential issue | 🟡 MinorUse a book-detail-specific locator for these clicks.
This selector still matches any anchor inside a card, so the suite can land on author/category/action links instead of the detail page whenever card markup changes. Please switch to a stable detail-link hook or an href pattern that only matches book-detail URLs.
Also applies to: 209-210, 288-289, 332-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/social-sharing.spec.js` around lines 172 - 173, The current locator assigned to bookLink (page.locator('.book-card a, .card a').filter({ hasText: /.+/ }).first()) is too broad and can match non-detail anchors; change it to a stable detail-specific selector such as a data-test hook (e.g., '[data-test="book-detail-link"]') or an href regex that only matches book-detail URLs (e.g., use locator('a[href^="/books/"]').filter({ hasText: /.+/ }).first()), update the variable bookLink (and the equivalent occurrences at the other three spots) to use that new selector, and ensure subsequent await bookLink.click() targets only the book detail page link.
🧹 Nitpick comments (2)
app/Models/BookRepository.php (1)
1175-1190: Extract this HTML→text normalization into a shared utility.The same cleanup rules now live here and in
app/Controllers/LibriController.php:2798-2805. Keeping one implementation will prevent search/backfill and CSV export from drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 1175 - 1190, Extract the HTML→text normalization logic from BookRepository::toPlainTextDescription into a single shared utility (e.g., a new static method HtmlNormalizer::toPlainText or a global helper html_to_plain_text) that accepts ?string and returns ?string, preserving the exact normalization steps and null/empty handling; then replace BookRepository::toPlainTextDescription body to call that utility and update the duplicate usage in LibriController (the code at the previous lines ~2798–2805) to call the same utility so both locations share the same implementation and signature.app/Views/frontend/book-detail.php (1)
1458-1458: Extract the hierarchy-link renderer before these two blocks drift.The
?genere=URL composition, separator handling, and escaping are now duplicated in both the hero and details sections. A small partial/helper would keep future filter changes in one place.Also applies to: 1632-1635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/book-detail.php` at line 1458, Extract the duplicated genre-hierarchy rendering into a single helper/partial (e.g., renderGenreHierarchy or a partial template) and replace both inline blocks with a call to it; the helper should accept the catalogRoute and genreHierarchy, iterate preserving the separator logic (emit <span class="genre-separator">></span> only when index > 0), and produce each link with the same escaping pipeline used now: decode entities (html_entity_decode), urlencode the query value for the ?genere= parameter, and wrap displayed text in htmlspecialchars with ENT_QUOTES and UTF-8; ensure the helper returns safe HTML (or is included as a partial) and is used by both the hero and details sections so changes to URL param composition, separator handling, or escaping are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/BookRepository.php`:
- Around line 96-108: getById() currently performs a DB UPDATE to lazily
backfill descrizione_plain (calls toPlainTextDescription, uses
$this->db->prepare and $upd->execute()), which creates a write side-effect;
remove the UPDATE and avoid executing $upd->execute() inside getById(). Instead,
compute $plain = $this->toPlainTextDescription((string)$row['descrizione']) and
assign $row['descrizione_plain'] = $plain for the returned result only (no
bind_param/execute), or move the persistence into a separate explicit method
(e.g. backfillDescrizionePlain($id)) or a background migration job that can be
invoked where writes are allowed.
In `@app/Views/frontend/layout.php`:
- Line 4: The code currently sets $ogBookMeta only when null but can still be a
non-iterable scalar and cause a foreach at the later foreach ($ogBookMeta as
...) to warn; replace the current initialization with a defensive check such as
using is_iterable/is_array: set $ogBookMeta = is_iterable($ogBookMeta) ?
$ogBookMeta : []; so that the foreach and the per-entry validation (the loop at
the later foreach and the subsequent per-entry checks) always receive an
iterable.
In `@app/Views/settings/index.php`:
- Around line 716-729: The JS fallback currently chooses the first element
matching document.querySelector('[data-settings-tab]') when no hash or ?tab is
present, which overrides the server-selected tab ($activeTab); change the
resolvedTab fallback logic so it first looks for a server-marked/active tab in
the DOM (e.g. an element matching selectors like
'[data-settings-tab][data-active]', '[data-settings-tab].active', or
'[data-settings-tab][aria-selected="true"]') and only falls back to the first
tab if none of those exist; update the resolvedTab calculation (the variables
hash, qTab, candidate, resolvedTab and the activateTab call) to read the chosen
element's data-settings-tab attribute from that prioritized query instead of
unconditionally using document.querySelector('[data-settings-tab]').
In `@installer/database/schema.sql`:
- Line 548: The UNIQUE KEY uk_plugin_hook_callback currently includes
callback_class but the runtime (PluginManager.php) identifies and registers
hooks by hook_name, callback_method and priority and always registers callbacks
as [$pluginInstance, $callbackMethod], so two rows differing only by
callback_class can produce the same effective callback; fix by making schema and
runtime consistent: either remove callback_class from the UNIQUE KEY definition
in schema.sql so uniqueness matches hook_name+callback_method+priority, or
update PluginManager (the hook loading/registration logic that builds and
registers [$pluginInstance, $callbackMethod]) to factor in callback_class when
resolving/instantiating callbacks so the unique index correctly enforces
uniqueness of the effective callback identity.
In `@tests/social-sharing.spec.js`:
- Around line 249-270: The test '6. Frontend: OG meta tags present on book page'
relies on prior test navigation and must explicitly navigate to a book detail
page at its start; update the test to open the intended book URL or perform the
same navigation step used elsewhere (e.g., call the navigation helper or await
page.goto(bookDetailUrl) / await page.click(...) to open a specific book) and
wait for the page to load (e.g., await
page.waitForSelector('meta[property="og:title"]' or await page.waitForURL(...))
before asserting meta tags so the test is independent and does not inherit state
from previous tests.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Around line 404-407: buildWhereConditions() currently filters genres by
matching g.nome/gp.nome/gpp.nome/sg.nome (display names), which is ambiguous;
change it to resolve the clicked node to its canonical generi.id and ancestor
IDs and filter by those IDs instead. Specifically: when a genre/hierarchy link
is clicked, lookup the generi row to obtain its id and parent chain, then in the
WHERE construction use g.id, gp.id, gpp.id, sg.id (or IN (ancestor_ids)) rather
than comparing nome; update any code paths that accept a genre slug/name to
first translate it to the generi.id, and modify the SQL/ORM filters built in
buildWhereConditions() to use numeric ID comparisons (or an ancestors array) for
both the main branch and the secondary join aliases (g, gp, gpp, sg).
In `@app/Views/frontend/partials/social-sharing.php`:
- Around line 169-180: The Web Share button is rendered unconditionally and
bypasses the admin-controlled $sharingProviders list; update the template to
only render the button when the Web Share provider is enabled (e.g., check
in_array('webapi', $sharingProviders) or similar provider slug) or remove the
standalone social-share-webapi button from the toolbar so share actions follow
the configured $sharingProviders loop (reference the $sharingProviders variable
and the social-share-webapi button/ CSS class to locate the code).
In `@installer/database/migrations/migrate_0.4.9.9.sql`:
- Around line 14-28: Migration only adds libri.descrizione_plain but doesn't
backfill existing rows, so SearchController::getById fallback keeps raw HTML
searchable; modify the migration (migrate_0.4.9.9.sql) to, after adding the
column (the ALTER TABLE block that references descrizione_plain), run an UPDATE
to backfill existing rows where descrizione_plain IS NULL by stripping HTML from
descrizione (use MySQL REGEXP_REPLACE(descrizione,'<[^>]+>','') or an equivalent
DB-safe expression) and set descrizione_plain = TRIM(...) so legacy rows are
searchable immediately; keep the conditional column-exists logic and execute the
UPDATE only when the column is newly created to avoid altering existing data.
In `@tests/social-sharing.spec.js`:
- Around line 172-173: The current locator assigned to bookLink
(page.locator('.book-card a, .card a').filter({ hasText: /.+/ }).first()) is too
broad and can match non-detail anchors; change it to a stable detail-specific
selector such as a data-test hook (e.g., '[data-test="book-detail-link"]') or an
href regex that only matches book-detail URLs (e.g., use
locator('a[href^="/books/"]').filter({ hasText: /.+/ }).first()), update the
variable bookLink (and the equivalent occurrences at the other three spots) to
use that new selector, and ensure subsequent await bookLink.click() targets only
the book detail page link.
---
Nitpick comments:
In `@app/Models/BookRepository.php`:
- Around line 1175-1190: Extract the HTML→text normalization logic from
BookRepository::toPlainTextDescription into a single shared utility (e.g., a new
static method HtmlNormalizer::toPlainText or a global helper html_to_plain_text)
that accepts ?string and returns ?string, preserving the exact normalization
steps and null/empty handling; then replace
BookRepository::toPlainTextDescription body to call that utility and update the
duplicate usage in LibriController (the code at the previous lines ~2798–2805)
to call the same utility so both locations share the same implementation and
signature.
In `@app/Views/frontend/book-detail.php`:
- Line 1458: Extract the duplicated genre-hierarchy rendering into a single
helper/partial (e.g., renderGenreHierarchy or a partial template) and replace
both inline blocks with a call to it; the helper should accept the catalogRoute
and genreHierarchy, iterate preserving the separator logic (emit <span
class="genre-separator">></span> only when index > 0), and produce each link
with the same escaping pipeline used now: decode entities (html_entity_decode),
urlencode the query value for the ?genere= parameter, and wrap displayed text in
htmlspecialchars with ENT_QUOTES and UTF-8; ensure the helper returns safe HTML
(or is included as a partial) and is used by both the hero and details sections
so changes to URL param composition, separator handling, or escaping are made in
one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: efbde284-8dbf-4dae-a923-b2d068d9bac4
📒 Files selected for processing (30)
README.mdapp/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Controllers/SearchController.phpapp/Controllers/SettingsController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/layout.phpapp/Views/frontend/partials/social-sharing.phpapp/Views/settings/index.phpinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/README.mdinstaller/database/migrations/migrate_0.4.9.9.sqlinstaller/database/migrations/migrate_0.5.0.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/assets/css/digital-library.cssstorage/plugins/digital-library/plugin.jsonstorage/plugins/digital-library/views/frontend-buttons.phpstorage/plugins/digital-library/views/frontend-pdf-viewer.phptests/smoke-install.spec.jstests/social-sharing.spec.jsupdater.md
Comment on lines
+96
to
+108
| // Lazy backfill: populate descrizione_plain for pre-migration rows on first read | ||
| if ($this->hasColumn('descrizione_plain') | ||
| && $row['descrizione_plain'] === null | ||
| && !empty($row['descrizione']) | ||
| ) { | ||
| $plain = $this->toPlainTextDescription((string)$row['descrizione']); | ||
| $upd = $this->db->prepare("UPDATE libri SET descrizione_plain = ? WHERE id = ?"); | ||
| if ($upd) { | ||
| $upd->bind_param('si', $plain, $id); | ||
| $upd->execute(); | ||
| } | ||
| $row['descrizione_plain'] = $plain; | ||
| } |
There was a problem hiding this comment.
Keep getById() side-effect free.
This read path now issues an UPDATE. getById() is also used from unrelated read-before-write flows such as app/Controllers/LibriController.php:1070 and app/Support/MergeHelper.php:93, so this can unexpectedly require write access and introduce locking/transaction failures in places that only asked to read the book.
♻️ Minimal fix
- // Lazy backfill: populate descrizione_plain for pre-migration rows on first read
- if ($this->hasColumn('descrizione_plain')
- && $row['descrizione_plain'] === null
- && !empty($row['descrizione'])
- ) {
- $plain = $this->toPlainTextDescription((string)$row['descrizione']);
- $upd = $this->db->prepare("UPDATE libri SET descrizione_plain = ? WHERE id = ?");
- if ($upd) {
- $upd->bind_param('si', $plain, $id);
- $upd->execute();
- }
- $row['descrizione_plain'] = $plain;
- }
+ if ($this->hasColumn('descrizione_plain')
+ && $row['descrizione_plain'] === null
+ && !empty($row['descrizione'])
+ ) {
+ // Keep reads side-effect free; persist backfills in migrations or write paths.
+ $row['descrizione_plain'] = $this->toPlainTextDescription((string) $row['descrizione']);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/BookRepository.php` around lines 96 - 108, getById() currently
performs a DB UPDATE to lazily backfill descrizione_plain (calls
toPlainTextDescription, uses $this->db->prepare and $upd->execute()), which
creates a write side-effect; remove the UPDATE and avoid executing
$upd->execute() inside getById(). Instead, compute $plain =
$this->toPlainTextDescription((string)$row['descrizione']) and assign
$row['descrizione_plain'] = $plain for the returned result only (no
bind_param/execute), or move the persistence into a separate explicit method
(e.g. backfillDescrizionePlain($id)) or a background migration job that can be
invoked where writes are allowed.
| <?php $content = $content ?? ''; | ||
| // $footerDescription is always set from ConfigStore (line ~16). For custom OG description, use $ogDescription. | ||
| $ogTitle = $ogTitle ?? null; | ||
| $ogBookMeta = $ogBookMeta ?? []; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ogBookMeta producers / consumers ==="
rg -n -C2 --glob '!vendor/**' --glob '!node_modules/**' '\$ogBookMeta\b|ogBookMeta' app storage/plugins tests
echo
echo "=== Direct assignments to ogBookMeta ==="
rg -n --glob '!vendor/**' --glob '!node_modules/**' '\$ogBookMeta\s*=' app storage/plugins testsRepository: fabiodalez-dev/Pinakes
Length of output: 2108
Harden $ogBookMeta to guard against non-iterable values before the foreach.
Line 4 initializes $ogBookMeta only against null with a null coalescing operator. However, if a controller or plugin passes a non-null scalar (string, int, etc.), the foreach loop at line 134 will raise a PHP warning before the per-entry validation at line 135 can execute. The check on line 133 (!empty()) does not prevent iteration attempts on non-iterable values.
🛠️ Proposed fix
-$ogBookMeta = $ogBookMeta ?? [];
+$ogBookMeta = is_iterable($ogBookMeta ?? null) ? $ogBookMeta : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $ogBookMeta = $ogBookMeta ?? []; | |
| $ogBookMeta = is_iterable($ogBookMeta ?? null) ? $ogBookMeta : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/frontend/layout.php` at line 4, The code currently sets $ogBookMeta
only when null but can still be a non-iterable scalar and cause a foreach at the
later foreach ($ogBookMeta as ...) to warn; replace the current initialization
with a defensive check such as using is_iterable/is_array: set $ogBookMeta =
is_iterable($ogBookMeta) ? $ogBookMeta : []; so that the foreach and the
per-entry validation (the loop at the later foreach and the subsequent per-entry
checks) always receive an iterable.
Comment on lines
+716
to
729
| // Check URL on page load: hash takes priority, then ?tab= query param | ||
| const hash = window.location.hash.substring(1); | ||
| if (hash && document.querySelector(`[data-settings-tab="${hash}"]`)) { | ||
| activateTab(hash); | ||
| const qTab = new URL(window.location.href).searchParams.get('tab'); | ||
| const candidate = (hash || qTab || ''); | ||
| const resolvedTab = (candidate && document.querySelector(`[data-settings-tab="${candidate}"]`)) | ||
| ? candidate | ||
| : (document.querySelector('[data-settings-tab]')?.getAttribute('data-settings-tab') || ''); | ||
| if (resolvedTab) { | ||
| activateTab(resolvedTab); | ||
| const url = new URL(window.location.href); | ||
| url.searchParams.set('tab', resolvedTab); | ||
| url.hash = resolvedTab; | ||
| window.history.replaceState({}, '', url.toString()); | ||
| } |
There was a problem hiding this comment.
Don't override the server-selected tab with the first DOM tab.
The PHP render already trusts $activeTab, but the JS fallback ignores it and always picks the first tab when the URL has no valid ?tab=/#hash. Any render that sets $activeTab without also rewriting the URL will visibly jump back to general on hydration/back-forward navigation.
💡 Minimal fix
- const candidate = (hash || qTab || '');
+ const fallbackTab = <?= json_encode($activeTab, JSON_HEX_TAG | JSON_HEX_AMP) ?> || document.querySelector('[data-settings-tab]')?.getAttribute('data-settings-tab') || '';
+ const candidate = hash || qTab || fallbackTab;
const resolvedTab = (candidate && document.querySelector(`[data-settings-tab="${candidate}"]`))
? candidate
- : (document.querySelector('[data-settings-tab]')?.getAttribute('data-settings-tab') || '');
+ : fallbackTab;
@@
- const firstTab = document.querySelector('[data-settings-tab]');
- if (firstTab) activateTab(firstTab.getAttribute('data-settings-tab'));
+ if (fallbackTab) activateTab(fallbackTab);Also applies to: 732-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/settings/index.php` around lines 716 - 729, The JS fallback
currently chooses the first element matching
document.querySelector('[data-settings-tab]') when no hash or ?tab is present,
which overrides the server-selected tab ($activeTab); change the resolvedTab
fallback logic so it first looks for a server-marked/active tab in the DOM (e.g.
an element matching selectors like '[data-settings-tab][data-active]',
'[data-settings-tab].active', or '[data-settings-tab][aria-selected="true"]')
and only falls back to the first tab if none of those exist; update the
resolvedTab calculation (the variables hash, qTab, candidate, resolvedTab and
the activateTab call) to read the chosen element's data-settings-tab attribute
from that prioritized query instead of unconditionally using
document.querySelector('[data-settings-tab]').
| `is_active` tinyint(1) NOT NULL DEFAULT '1' COMMENT 'Whether hook is active', | ||
| `created_at` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| PRIMARY KEY (`id`), | ||
| UNIQUE KEY `uk_plugin_hook_callback` (`plugin_id`,`hook_name`,`callback_class`,`callback_method`), |
There was a problem hiding this comment.
Make the unique key match the callback identity the runtime actually uses.
app/Support/PluginManager.php:1234-1262 loads hooks by hook_name, callback_method, and priority, then always registers [$pluginInstance, $callbackMethod]. With the current key, two rows that differ only in callback_class still pass the constraint and will register the same effective callback twice. Either make runtime dispatch honor callback_class, or remove it from the uniqueness criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/database/schema.sql` at line 548, The UNIQUE KEY
uk_plugin_hook_callback currently includes callback_class but the runtime
(PluginManager.php) identifies and registers hooks by hook_name, callback_method
and priority and always registers callbacks as [$pluginInstance,
$callbackMethod], so two rows differing only by callback_class can produce the
same effective callback; fix by making schema and runtime consistent: either
remove callback_class from the UNIQUE KEY definition in schema.sql so uniqueness
matches hook_name+callback_method+priority, or update PluginManager (the hook
loading/registration logic that builds and registers [$pluginInstance,
$callbackMethod]) to factor in callback_class when resolving/instantiating
callbacks so the unique index correctly enforces uniqueness of the effective
callback identity.
Comment on lines
+249
to
+270
| test('6. Frontend: OG meta tags present on book page', async () => { | ||
| // Still on book detail page | ||
| const ogTitle = await page.locator('meta[property="og:title"]').getAttribute('content'); | ||
| expect(ogTitle).toBeTruthy(); | ||
| expect(ogTitle.length).toBeGreaterThan(0); | ||
|
|
||
| const ogUrl = await page.locator('meta[property="og:url"]').getAttribute('content'); | ||
| expect(ogUrl).toContain('http'); | ||
|
|
||
| const ogType = await page.locator('meta[property="og:type"]').getAttribute('content'); | ||
| expect(ogType).toBe('book'); | ||
|
|
||
| const ogImage = await page.locator('meta[property="og:image"]').getAttribute('content'); | ||
| expect(ogImage).toBeTruthy(); | ||
|
|
||
| // Twitter card | ||
| const twitterCard = await page.locator('meta[name="twitter:card"]').getAttribute('content'); | ||
| expect(twitterCard).toBe('summary_large_image'); | ||
|
|
||
| const twitterTitle = await page.locator('meta[name="twitter:title"]').getAttribute('content'); | ||
| expect(twitterTitle).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Don't let the OG test inherit page state from the previous case.
This test never reopens a book page; it assumes the previous test already left the browser on the right URL. If that navigation changes or the earlier case fails before reaching the detail page, this one reports meta-tag failures against the wrong page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/social-sharing.spec.js` around lines 249 - 270, The test '6. Frontend:
OG meta tags present on book page' relies on prior test navigation and must
explicitly navigate to a book detail page at its start; update the test to open
the intended book URL or perform the same navigation step used elsewhere (e.g.,
call the navigation helper or await page.goto(bookDetailUrl) / await
page.click(...) to open a specific book) and wait for the page to load (e.g.,
await page.waitForSelector('meta[property="og:title"]' or await
page.waitForURL(...)) before asserting meta tags so the test is independent and
does not inherit state from previous tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LEFT JOIN generi sg ON l.sottogenere_id = sg.idto 4 queries inFrontendController— thebuildWhereConditionsmethod referencessg.nomebut these queries were missing the JOIN, causing a 500 error when filtering by genrel.descrizioneto bothsearchBooksandsearchBooksWithDetailsso the header search bar matches book descriptionsdescrizionecolumn right aftersottotitolofor more logical orderingTest plan
descrizionecolumn is aftersottotitoloand contains plain text (no HTML)Fixes #90
Fixes #83
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Chores
Tests