feat: Discogs music scraper plugin (#87)#100
Merged
fabiodalez-dev merged 57 commits intomainfrom Apr 17, 2026
Merged
Conversation
New plugin that integrates with Discogs API for music metadata: - Search by barcode (EAN) or title+artist - Maps: Artist → Author, Label → Publisher, Tracklist → Description - Covers from Discogs images - Format detection: CD, Vinyl, Cassette - Optional API token for higher rate limits (60 vs 25 req/min) - Same hook pattern as OpenLibrary (scrape.sources/fetch/modify)
|
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 a Discogs-backed multi-source music metadata scraper and admin settings, introduces media-type support (libri.tipo_media) with schema/migration, MediaLabels helpers, bulk enrichment (service/controller/view/cron), plugin/updater refactors, catalog UI/filters/badges, many E2E tests, installer/packaging updates, and assorted locale and CSV/import changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as Admin/User
participant App as Application
participant Discogs as Discogs API
participant MB as MusicBrainz API
participant CAA as Cover Art Archive
participant Deezer as Deezer API
participant DB as Database
User->>App: Import or barcode scrape request
App->>Discogs: Search release by barcode
Discogs-->>App: Found / Not found
alt Discogs found
App->>Discogs: Fetch release details
Discogs-->>App: Release metadata + images
else Discogs not found
App->>MB: Search release by barcode
MB-->>App: Release metadata
App->>CAA: Fetch cover by MB release id
CAA-->>App: Cover image or 404
end
App->>App: Normalize/map payload (resolve tipo_media, tracks, cover, ids)
alt Missing cover or genres
App->>Deezer: Search album by artist+title
Deezer-->>App: high-res cover
end
App->>DB: Merge/persist merged data (including tipo_media)
DB-->>App: OK
App-->>User: Return populated form / saved record
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
storage/plugins/discogs/DiscogsPlugin.php (1)
85-89: Keep hook registration in one source of truth.
setPluginId()immediately callsensureHooksRegistered(), while the same hook names and priorities already live instorage/plugins/discogs/plugin.json. That leaves hook metadata duplicated in two places, and theCOUNT(*) > 0check is too weak to catch partial drift. Prefer manifest-driven registration, or validate the expected hook set explicitly.Also applies to: 94-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/DiscogsPlugin.php` around lines 85 - 89, The call to ensureHooksRegistered() from setPluginId() duplicates hook metadata that already lives in storage/plugins/discogs/plugin.json; remove the implicit registration here and instead drive hook registration from the manifest (plugin.json) or add an explicit validation that compares the manifest's expected hook names/priorities with what's persisted and only registers missing/incorrect entries. Concretely, stop calling ensureHooksRegistered() inside setPluginId(), implement a single manifest-driven routine (e.g., load and parse storage/plugins/discogs/plugin.json) to register hooks (or to diff and fix persisted hooks), and/or update ensureHooksRegistered()/the registration functions referenced in the block around lines 94-155 to accept the manifest as the source of truth and perform exact set comparison (not COUNT(*) > 0) before inserting/updating hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 225-259: Centralize and implement token-aware throttling inside
the DiscogsPlugin::apiRequest method so callers like the search/fetch sequence
in the try block (calls to apiRequest from within DiscogsPlugin where
getSetting('api_token') is used) no longer have to sleep manually; detect
whether a token is present (token truthy from getSetting or passed into
apiRequest) and enforce a minimum inter-request delay of ~1.0s for authenticated
requests and ~2.4s for anonymous requests, tracking the timestamp of the last
request per-plugin (or per-token/anonymous bucket) to sleep the remaining time
before performing the HTTP call, and remove/avoid hardcoded usleep calls (e.g.,
the usleep(1000000) calls around search and release fetch) so rate limiting is
handled centrally in apiRequest.
- Around line 232-245: The fallback currently does a generic q= search with only
$isbn and then trusts $searchResult['results'][0] which can return an unrelated
release; update the logic in the DiscogsPlugin method that calls
$this->apiRequest so that after the search you verify the candidate release
actually contains the barcode/isbn before accepting it (e.g., inspect the
returned release's identifiers/barcodes fields for a match to $isbn), and if no
exact barcode is found return $currentResult instead of using results[0];
alternatively, remove the generic q= fallback and immediately return
$currentResult when the barcode lookup fails.
---
Nitpick comments:
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 85-89: The call to ensureHooksRegistered() from setPluginId()
duplicates hook metadata that already lives in
storage/plugins/discogs/plugin.json; remove the implicit registration here and
instead drive hook registration from the manifest (plugin.json) or add an
explicit validation that compares the manifest's expected hook names/priorities
with what's persisted and only registers missing/incorrect entries. Concretely,
stop calling ensureHooksRegistered() inside setPluginId(), implement a single
manifest-driven routine (e.g., load and parse
storage/plugins/discogs/plugin.json) to register hooks (or to diff and fix
persisted hooks), and/or update ensureHooksRegistered()/the registration
functions referenced in the block around lines 94-155 to accept the manifest as
the source of truth and perform exact set comparison (not COUNT(*) > 0) before
inserting/updating hooks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b53824a1-80c6-433c-8656-8439478dee1d
📒 Files selected for processing (5)
.gitignorestorage/plugins/discogs/DiscogsPlugin.phpstorage/plugins/discogs/README.mdstorage/plugins/discogs/plugin.jsonstorage/plugins/discogs/wrapper.php
- MediaLabels helper: dynamic field labels based on format (Artista, Etichetta, Tracce for music media) - Frontend book-detail: labels adapt for CD/vinyl/cassette - Admin scheda_libro: labels adapt for music - Discogs plugin settings page with API token input - Rate limiting centralized in apiRequest() (1s auth, 2.5s anon) - Removed unreliable generic q= search fallback - Translations: IT/EN/DE for all music labels
- Add discogs to BUNDLED_PLUGINS in PluginManager and Updater - 7 E2E tests: plugin install/activate, settings page, MediaLabels for music vs regular books, barcode label on frontend, scraping
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)
app/Views/frontend/book-detail.php (1)
1699-1717:⚠️ Potential issue | 🟡 MinorMusic imports still fall through to the
EANlabel.
storage/plugins/discogs/DiscogsPlugin.phpstores the scanned code ineanand leavesisbn13empty. This change only renames theisbn13row, so real Discogs items still hit the unchangedEANblock below and never show the new "Barcode" label. Prefer theeanvalue/label when$isMusicis true.🤖 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 1699 - 1717, The music imports put the scanned code in $book['ean'] so adjust the ISBN/EAN rendering to prefer EAN for music: update the block that currently uses $book['isbn13'] (and the label logic using $isMusic) to display $book['ean'] when $isMusic and $book['ean'] is not empty (fall back to $book['isbn13'] otherwise), and change the EAN meta-item block to either skip for music or show the label "Barcode" when $isMusic (and use $book['ean'] as the value); target the conditional blocks using $isMusic, $book['isbn13'] and $book['ean'] in the book-detail.php snippet so Discogs-imported items display the Barcode label and value correctly.
🤖 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/Support/MediaLabels.php`:
- Around line 14-29: isMusic currently does substring matching against
self::$musicFormats (including 'audio'), so strings like
"audiolibro"/"audiobook" match; change isMusic to only accept exact normalized
tokens or word-boundary matches: in the isMusic(?string $formato) method
normalize (trim, strtolower), then either split on non-alphanumeric characters
and check each resulting token for exact membership in self::$musicFormats or
use a single preg_match that checks
\b(music|cd|vinyl|lp|cassette|cd_audio|vinile|cassetta|musik)\b; also remove or
avoid plain substring checks for 'audio' (make it a separate token if you keep
it) so audiobook strings no longer match.
In `@app/Views/frontend/book-detail.php`:
- Around line 2178-2180: The fallback label uses the parent page's $isMusic;
change the ternary to use the related item's format flag so each related card
decides its own label—e.g. when rendering <?=
htmlspecialchars($related['autori'] ?? __($isMusic ? 'Artista sconosciuto' :
'Autore sconosciuto')...) ?> replace $isMusic with ($related['isMusic'] ??
$isMusic) (or the related item's format key if named differently) so the
translation call uses the related item's isMusic flag while keeping
htmlspecialchars, ENT_QUOTES and UTF-8 intact.
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 220-255: The early-return guard that skips Discogs when any
non-Discogs source set a title prevents Discogs-only fields from ever being
merged; locate the conditional starting with is_array($currentResult) &&
!empty($currentResult['title']) and either remove that short-circuit or change
it to check a real completeness signal (e.g. a boolean flag like
$currentResult['complete'] === true or a specific required-field check) so that
mergeBookData($currentResult, $discogsData) and BookDataMerger::merge() still
run for partial upstream results; ensure mapReleaseToPinakes($release,
$firstResult, $isbn) output is merged rather than skipped.
- Around line 604-648: getSetting()/saveSettings() in DiscogsPlugin currently
read/write plaintext from plugin_settings while
PluginController::updateSettings() uses PluginManager::setSetting() (which
stores encrypted "ENC:..." values), causing inconsistent handling of api_token;
update DiscogsPlugin to always use PluginManager's API for settings so
encryption/decryption is consistent: replace direct DB reads/writes in
getSetting() and saveSettings() to call
PluginManager::getSetting()/PluginManager::setSetting() (or the project
equivalent), ensure the resolved pluginId path still works when fetching via
PluginManager, and make sure the token passed to the Discogs client is the
decrypted value returned by PluginManager::getSetting() (i.e., strip/handle
"ENC:" transparently via the manager).
In `@storage/plugins/discogs/views/settings.php`:
- Around line 96-100: The form currently round-trips the masked value from
DiscogsPlugin::getSettings() through the password input (id/name "api_token"),
which causes literal asterisks to overwrite the real token on save; change the
view in settings.php to render the password input value as empty when a token
exists and instead render a separate indicator/hidden field (e.g., "has_token")
to show presence of a saved token, and update the server-side save handler
(e.g., DiscogsPlugin::saveSettings() or equivalent) to only update the stored
token when a non-empty "api_token" is submitted (ignore empty values and/or the
"has_token" flag), so opening and saving without editing does not replace the
real token.
- Around line 133-137: Replace the hardcoded href that calls
url('/admin/plugins') with the named-route helper (e.g., route_path('plugins')
or RouteTranslator::route('plugins')) and pass that result through
htmlspecialchars(…, ENT_QUOTES, 'UTF-8') before output; update the anchor in
settings.php so it uses the named route helper instead of the literal
"/admin/plugins" to keep routing and localization consistent (refer to
route_path and RouteTranslator::route as the functions to use).
In `@tests/discogs-plugin.spec.js`:
- Around line 183-202: The test currently fetches the admin URL
(`/admin/libri/${bookId}`) so it never exercises the frontend view or the
EAN→Barcode label logic in app/Views/frontend/book-detail.php; update the test
'6. Frontend: music book shows Barcode instead of ISBN-13' to request the public
book page (use the public route the app serves for book details instead of
`/admin/libri/${bookId}`, e.g. the frontend path built from BASE and the book
identifier/slug), then assert that the returned HTML for that public page
contains "Barcode" (or the music-aware labels) and not the ISBN label; keep the
same test setup (inserting the record with TEST_BARCODE and selecting bookId)
but replace the admin fetch using page.request.get with a fetch to the frontend
URL so the EAN→Barcode behavior in app/Views/frontend/book-detail.php is
actually validated.
- Around line 77-137: The test uses broad page-wide selectors so operations may
act on other plugins and silent no-ops hide failures; scope all locators to the
Discogs card (discogsCard) before clicking install (installBtn), activate
(activateBtn) or opening settings (settingsLink), assert discogsCard.isVisible()
and throw/fail if missing, assert that dbQuery shows activation
(pluginActivated) after activateBtn.click(), and explicitly assert
tokenField.isVisible() (instead of silently continuing) so the test fails when
Discogs is not installed/activated or the API token field is absent.
---
Outside diff comments:
In `@app/Views/frontend/book-detail.php`:
- Around line 1699-1717: The music imports put the scanned code in $book['ean']
so adjust the ISBN/EAN rendering to prefer EAN for music: update the block that
currently uses $book['isbn13'] (and the label logic using $isMusic) to display
$book['ean'] when $isMusic and $book['ean'] is not empty (fall back to
$book['isbn13'] otherwise), and change the EAN meta-item block to either skip
for music or show the label "Barcode" when $isMusic (and use $book['ean'] as the
value); target the conditional blocks using $isMusic, $book['isbn13'] and
$book['ean'] in the book-detail.php snippet so Discogs-imported items display
the Barcode label and value correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1ff7bca-6875-4950-9817-c1b642b81b57
📒 Files selected for processing (12)
.gitignoreapp/Controllers/PluginController.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phplocale/de_DE.jsonlocale/en_US.jsonstorage/plugins/discogs/DiscogsPlugin.phpstorage/plugins/discogs/views/settings.phptests/discogs-plugin.spec.js
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- app/Support/Updater.php
- app/Support/PluginManager.php
- locale/de_DE.json
- locale/en_US.json
ScrapeController::normalizeIsbnFields() was auto-populating isbn13 from the search term. For Discogs results (source=discogs), the search term is a barcode/EAN, not an ISBN — skip ISBN auto-population.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/discogs-import.spec.js (2)
109-112: The artist check is currently a no-op.This block computes
authorCountbut never asserts it, so the suite does not actually verify theArtists → Autoremapping from this PR.Suggested fix
- const authorItems = page.locator('#autori-wrapper .choices__item--selectable, .choices__item.choices__item--selectable'); + const authorItems = page.locator('#autori-wrapper .choices__item.choices__item--selectable'); const authorCount = await authorItems.count().catch(() => 0); + if (authorCount === 0) { + throw new Error('No artist chips were populated by the Discogs import'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 109 - 112, The test computes authorCount from the locator authorItems but never asserts it; update the test (in tests/discogs-import.spec.js) to assert that authorCount is greater than zero (e.g., use your test framework's expect/assert on authorCount) so the "Artists → Autore" mapping is actually verified; refer to the authorItems locator and the authorCount variable when adding the assertion.
179-184: Keep the music-format guard aligned with production.
app/Support/MediaLabels.php:14-33treatslp,cassetta,cassette,audio,musik, andmusicas music too. This subset can skip the label assertion even when the saved item is a valid music import.Suggested fix
- if (formato && ['cd_audio', 'vinile', 'cd', 'vinyl'].some(f => formato.toLowerCase().includes(f))) { + if (formato && ['cd_audio', 'vinile', 'cassetta', 'vinyl', 'lp', 'cd', 'cassette', 'audio', 'musik', 'music'] + .some(f => formato.toLowerCase().includes(f))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 179 - 184, The music-format guard in the test only checks ['cd_audio','vinile','cd','vinyl'] but must match the production list used by MediaLabels (include tokens like 'lp', 'cassetta', 'cassette', 'audio', 'musik', 'music' as well); update the array used with formato.toLowerCase().includes(...) in the test to include those additional tokens so the conditional around hasMusicLabel (which checks content for 'Etichetta'/'Label'/'Anno di Uscita'/'Release Year') mirrors the production MediaLabels behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/discogs-import.spec.js`:
- Around line 77-79: The test currently treats a missing import button or empty
titolo as a skip (using importBtn.isVisible(...).catch(() => false) then
test.skip), but after plugin activation those cases indicate a real product
failure because fetchFromDiscogs() can return previous results on errors; update
the test to fail fast instead: replace the test.skip branches at the import
button check (importBtn.isVisible) and the titolo empty check with explicit
assertions that throw or use test.fail/assert to fail the test when the import
button is not visible or titolo is empty, and reference the fetchFromDiscogs()
behavior in the failure message to aid debugging.
- Around line 50-53: The test teardown in test.afterAll currently only removes
rows by EAN via dbExec("DELETE FROM libri WHERE ean = '0720642442524'...") but
the import logic also persists ISBN-13 (see save paths around lines saving
isbn13), so modify the cleanup to delete rows where either ean = '0720642442524'
OR isbn13 = '0720642442524' (or run two deletes) using the same dbExec call(s);
update the test.afterAll block to remove both persistence paths so stale rows
don't survive between test runs.
- Around line 151-160: The test currently uses test.skip when dbQuery returns no
row, which hides a failed persistence; update the block that checks book and
bookByIsbn so that a missing DB row fails the test instead of skipping: replace
the test.skip(bookByIsbn === '', ...) call with an assertion or explicit failure
(e.g., expect(bookByIsbn).not.toBe('') or throw new Error(...)) and keep the
subsequent expect(bookByIsbn.toLowerCase()).toContain('nevermind') path; locate
the logic around the book variable, the dbQuery(...) call and the test.skip
invocation to make this change.
---
Nitpick comments:
In `@tests/discogs-import.spec.js`:
- Around line 109-112: The test computes authorCount from the locator
authorItems but never asserts it; update the test (in
tests/discogs-import.spec.js) to assert that authorCount is greater than zero
(e.g., use your test framework's expect/assert on authorCount) so the "Artists →
Autore" mapping is actually verified; refer to the authorItems locator and the
authorCount variable when adding the assertion.
- Around line 179-184: The music-format guard in the test only checks
['cd_audio','vinile','cd','vinyl'] but must match the production list used by
MediaLabels (include tokens like 'lp', 'cassetta', 'cassette', 'audio', 'musik',
'music' as well); update the array used with formato.toLowerCase().includes(...)
in the test to include those additional tokens so the conditional around
hasMusicLabel (which checks content for 'Etichetta'/'Label'/'Anno di
Uscita'/'Release Year') mirrors the production MediaLabels behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9563aa2a-a47d-44d2-8396-94e770ceb55c
📒 Files selected for processing (1)
tests/discogs-import.spec.js
The book form submit shows a SweetAlert 'Are you sure?' dialog. The test now clicks the confirm button before waiting for navigation. All 6 import tests pass including save + DB verify + label check.
- MediaLabels::formatDisplayName() maps cd_audio→"CD Audio", vinile→"Vinile" etc. - All format names translatable via __() (IT/EN/DE) - Frontend book-detail: format shows display name, not raw key - Admin scheda_libro: same format display name - Discogs plugin: tracklist now generates <ol> HTML, not plain text - Frontend: music description section shows formatted tracklist with <ol> - MediaLabels::formatTracklist() for legacy plain-text tracklists
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/Support/MediaLabels.php (1)
14-29:⚠️ Potential issue | 🟠 MajorStop using substring matches for music detection.
Line 28 still matches short fragments like
audio,cd, andlp, soaudiolibro/audiobookand other unrelated formats can flip the UI to music labels. Normalize to a canonical token and do exact matches or word-boundary checks instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/MediaLabels.php` around lines 14 - 29, The isMusic method in MediaLabels currently uses substring checks against self::$musicFormats which causes false positives (e.g., "audiolibro"); update it to normalize the input (trim, lowercase) then split/tokenize on non-word boundaries into canonical tokens and check for exact token membership instead of str_contains; also normalize/replace entries in the $musicFormats list to canonical tokens (e.g., "cd", "vinyl", "cassette", "lp", "music", "audio") and perform either an in_array lookup or a word-boundary regex match against those tokens in isMusic to avoid matching substrings like "audiobook".
🧹 Nitpick comments (6)
app/Views/libri/scheda_libro.php (2)
8-9: Unused variable$isMusic.The variable is declared but not referenced in the changed code. The label switching is done inline by passing
$libro['formato']directly toMediaLabels::label()calls. Consider removing if not used elsewhere in this file.#!/bin/bash # Verify if $isMusic is used anywhere in the file rg '\$isMusic' app/Views/libri/scheda_libro.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` around lines 8 - 9, Remove the unused local variable $isMusic which is assigned via MediaLabels::isMusic($libro['formato'] ?? null) but never referenced; instead, delete the assignment line and rely on the existing inline MediaLabels::label(...) calls (or if you intended to use $isMusic, replace the inline calls to reference $isMusic where appropriate). Ensure you remove only the unused $isMusic declaration and do not alter MediaLabels::label or other logic.
147-147: Consider escapingMediaLabels::label()output for consistency.Line 379 properly escapes the format display name with
HtmlHelper::e(), but lines 147, 337, and 366 output labels directly. While these are likely safe translation strings, escaping ensures consistent defense-in-depth.🔧 Suggested fix for consistent escaping
- <span class="font-medium"><?= \App\Support\MediaLabels::label('editore', $libro['formato'] ?? null) ?>:</span> + <span class="font-medium"><?= App\Support\HtmlHelper::e(\App\Support\MediaLabels::label('editore', $libro['formato'] ?? null)) ?>:</span>Apply similar changes to lines 337 and 366.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` at line 147, Escape the output of MediaLabels::label() for defense-in-depth: replace direct echoing of \App\Support\MediaLabels::label(...) with HtmlHelper::e(\App\Support\MediaLabels::label(...)) wherever it is used (e.g., the occurrences that currently render labels), and apply the same change to the other two label usages so all label outputs are consistently wrapped with HtmlHelper::e().tests/discogs-import.spec.js (4)
63-63: Prefer condition-based waits over fixed timeouts.
waitForTimeout(2000)is brittle and can cause flaky tests or unnecessary slowdowns. Consider waiting for a specific DOM condition or network idle state instead.Suggested fix
- await page.waitForTimeout(2000); + // Wait for plugin registration to complete + await page.waitForFunction(() => { + return document.querySelector('[data-plugin="discogs"]') !== null || + document.readyState === 'complete'; + }, { timeout: 5000 }).catch(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` at line 63, Replace the brittle fixed delay at page.waitForTimeout(2000) with a condition-based wait: identify a stable DOM element or network condition that signals the app is ready (e.g., a specific selector or an API response) and use page.waitForSelector('...') or page.waitForResponse(...) / page.waitForLoadState('networkidle') instead of page.waitForTimeout; update the test in tests/discogs-import.spec.js where page.waitForTimeout(2000) is called so it waits for the specific DOM selector or response relevant to the import flow.
110-112: Unnecessary.catch(() => 0)oncount().Playwright's
count()returns aPromise<number>that resolves to 0 for no matches—it doesn't reject. The catch masks unexpected errors.Suggested fix
- const authorCount = await authorItems.count().catch(() => 0); + const authorCount = await authorItems.count();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 110 - 112, The call to authorItems.count() is wrapped with an unnecessary .catch(() => 0) which masks real errors; remove the .catch and simply await authorItems.count() (locator: authorItems from page.locator('#autori-wrapper .choices__item--selectable, .choices__item.choices__item--selectable')) so you receive the actual numeric count (Playwright resolves to 0 when no matches) and let unexpected errors surface normally.
117-122: Complex OR assertion is hard to debug on failure.When this fails, the error message won't indicate which field was checked or what values were found.
Suggested fix for clearer failure messages
// Check EAN field has the barcode const eanValue = await page.locator('input[name="ean"]').inputValue(); // The barcode might be in isbn13 or ean depending on the scraper const isbn13Value = await page.locator('input[name="isbn13"]').inputValue(); - expect(eanValue === TEST_BARCODE || isbn13Value === TEST_BARCODE || - eanValue.includes('720642442524') || isbn13Value.includes('720642442524')).toBe(true); + const barcodeFound = eanValue === TEST_BARCODE || isbn13Value === TEST_BARCODE || + eanValue.includes('720642442524') || isbn13Value.includes('720642442524'); + expect(barcodeFound, `Expected barcode in ean="${eanValue}" or isbn13="${isbn13Value}"`).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 117 - 122, The complex OR assertion using eanValue, isbn13Value, and TEST_BARCODE is hard to debug; replace it with clearer checks: first compute eanValue and isbn13Value as already done, then assert each possibility in separate expectations (or a single expectation that includes a diagnostic message) so test failure shows which field/value failed; reference the eanValue and isbn13Value variables and the TEST_BARCODE constant in tests/discogs-import.spec.js (replace the combined expect(...) with individual expects that check eanValue === TEST_BARCODE, isbn13Value === TEST_BARCODE, eanValue.includes('720642442524'), and isbn13Value.includes('720642442524') or a single expect with a descriptive failure message containing the actual values).
86-88: Replace fixed 8-second timeout with a condition-based wait.Fixed timeouts are brittle—too short causes flakiness, too long wastes CI time. Wait for the scrape result indicator instead.
Suggested fix
// Wait for scraping response (Discogs needs time + rate limits) - // The scraping service tries multiple sources — wait up to 20s - await page.waitForTimeout(8000); + // Wait for title field to be populated or loading indicator to disappear + await page.waitForFunction( + () => { + const title = document.querySelector('input[name="titolo"]'); + const loading = document.querySelector('.scrape-loading, .spinner'); + return (title && title.value !== '') || !loading; + }, + { timeout: 20000 } + ).catch(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 86 - 88, Replace the brittle fixed delay (page.waitForTimeout) with a condition-based wait that polls for the scraping result indicator: remove the await page.waitForTimeout(8000) call and instead use page.waitForSelector(...) or page.waitForFunction(...) to wait up to the same maximum (e.g., 20s) for the element/text that signals the scrape completed (the DOM selector or text shown by the scraping service), and keep the existing test flow after that; this change touches the page.waitForTimeout usage in the test and uses page.waitForSelector/page.waitForFunction to await the scrape result.
🤖 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/Support/MediaLabels.php`:
- Around line 90-91: The helper returns raw external tracklist text ($text) when
($tracks === false || count($tracks) < 2) and in the similar branch at lines
104-105; instead of returning $text raw, HTML-escape/encode it (e.g., via the
framework's escape helper or htmlspecialchars) before returning so the helper
consistently returns safe HTML fragments; update the branches that return $text
to return the escaped version and keep any intentional HTML whitelisting as a
separate, explicit path.
In `@locale/de_DE.json`:
- Around line 3517-3527: The German locale adds overly specific and a duplicate
media label: change the value for the "Cartaceo" key from "Taschenbuch" to a
more generic term like "Druckausgabe" (or "Print") so
MediaLabels::formatDisplayName() doesn't force a paperback meaning, and remove
or consolidate the duplicate "Altro" key (the second "Altro": "Sonstiges" entry)
so there is only one "Altro" translation in the file.
In `@tests/discogs-import.spec.js`:
- Line 28: The test suite runs files in parallel despite test.describe.serial in
tests/discogs-import.spec.js; to avoid cross-file DB race conditions set
Playwright workers to 1: update tests/playwright.config.js to add or set
workers: 1 in the exported config object, or alter the test NPM script in
package.json to append the flag --workers=1 so all test files run with a single
worker; ensure you apply the change globally so tests like
discogs-import.spec.js, discogs-plugin.spec.js, admin-features.spec.js, and
email-notifications.spec.js do not execute concurrently.
---
Duplicate comments:
In `@app/Support/MediaLabels.php`:
- Around line 14-29: The isMusic method in MediaLabels currently uses substring
checks against self::$musicFormats which causes false positives (e.g.,
"audiolibro"); update it to normalize the input (trim, lowercase) then
split/tokenize on non-word boundaries into canonical tokens and check for exact
token membership instead of str_contains; also normalize/replace entries in the
$musicFormats list to canonical tokens (e.g., "cd", "vinyl", "cassette", "lp",
"music", "audio") and perform either an in_array lookup or a word-boundary regex
match against those tokens in isMusic to avoid matching substrings like
"audiobook".
---
Nitpick comments:
In `@app/Views/libri/scheda_libro.php`:
- Around line 8-9: Remove the unused local variable $isMusic which is assigned
via MediaLabels::isMusic($libro['formato'] ?? null) but never referenced;
instead, delete the assignment line and rely on the existing inline
MediaLabels::label(...) calls (or if you intended to use $isMusic, replace the
inline calls to reference $isMusic where appropriate). Ensure you remove only
the unused $isMusic declaration and do not alter MediaLabels::label or other
logic.
- Line 147: Escape the output of MediaLabels::label() for defense-in-depth:
replace direct echoing of \App\Support\MediaLabels::label(...) with
HtmlHelper::e(\App\Support\MediaLabels::label(...)) wherever it is used (e.g.,
the occurrences that currently render labels), and apply the same change to the
other two label usages so all label outputs are consistently wrapped with
HtmlHelper::e().
In `@tests/discogs-import.spec.js`:
- Line 63: Replace the brittle fixed delay at page.waitForTimeout(2000) with a
condition-based wait: identify a stable DOM element or network condition that
signals the app is ready (e.g., a specific selector or an API response) and use
page.waitForSelector('...') or page.waitForResponse(...) /
page.waitForLoadState('networkidle') instead of page.waitForTimeout; update the
test in tests/discogs-import.spec.js where page.waitForTimeout(2000) is called
so it waits for the specific DOM selector or response relevant to the import
flow.
- Around line 110-112: The call to authorItems.count() is wrapped with an
unnecessary .catch(() => 0) which masks real errors; remove the .catch and
simply await authorItems.count() (locator: authorItems from
page.locator('#autori-wrapper .choices__item--selectable,
.choices__item.choices__item--selectable')) so you receive the actual numeric
count (Playwright resolves to 0 when no matches) and let unexpected errors
surface normally.
- Around line 117-122: The complex OR assertion using eanValue, isbn13Value, and
TEST_BARCODE is hard to debug; replace it with clearer checks: first compute
eanValue and isbn13Value as already done, then assert each possibility in
separate expectations (or a single expectation that includes a diagnostic
message) so test failure shows which field/value failed; reference the eanValue
and isbn13Value variables and the TEST_BARCODE constant in
tests/discogs-import.spec.js (replace the combined expect(...) with individual
expects that check eanValue === TEST_BARCODE, isbn13Value === TEST_BARCODE,
eanValue.includes('720642442524'), and isbn13Value.includes('720642442524') or a
single expect with a descriptive failure message containing the actual values).
- Around line 86-88: Replace the brittle fixed delay (page.waitForTimeout) with
a condition-based wait that polls for the scraping result indicator: remove the
await page.waitForTimeout(8000) call and instead use page.waitForSelector(...)
or page.waitForFunction(...) to wait up to the same maximum (e.g., 20s) for the
element/text that signals the scrape completed (the DOM selector or text shown
by the scraping service), and keep the existing test flow after that; this
change touches the page.waitForTimeout usage in the test and uses
page.waitForSelector/page.waitForFunction to await the scrape result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52c3815c-fa32-4a11-81f2-e258f66516db
📒 Files selected for processing (8)
app/Controllers/ScrapeController.phpapp/Support/MediaLabels.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phplocale/de_DE.jsonlocale/en_US.jsonstorage/plugins/discogs/DiscogsPlugin.phptests/discogs-import.spec.js
✅ Files skipped from review due to trivial changes (2)
- app/Controllers/ScrapeController.php
- locale/en_US.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/frontend/book-detail.php
Database: - New ENUM column tipo_media (libro/disco/audiolibro/dvd/altro) - Migration auto-populates from existing formato values - Index idx_libri_tipo_media for efficient filtering Backend: - MediaLabels: allTypes(), icon(), schemaOrgType(), inferTipoMedia() - All 7 controllers updated: filter, import/export, scraping - BookRepository: hasColumn guard for backward compat - Discogs plugin auto-sets tipo_media='disco' Admin UI: - Book form: tipo_media dropdown selector - Book list: icon column + filter dropdown - Book detail: media type badge Frontend: - Catalog: tipo_media filter in sidebar - Book detail: Schema.org dynamic type (Book/MusicAlbum/Audiobook/Movie) - Catalog grid: media type badge on non-book cards - Format display names: cd_audio → "CD Audio" (translated) Translations: IT/EN/DE for all media types and labels
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Controllers/ScrapeController.php (1)
76-99:⚠️ Potential issue | 🟠 MajorDiscogs plugin cannot process non-ISBN barcodes due to validation gate.
The
byIsbn()method validates input withisValidIsbn()(line 89), which accepts only ISBN-10 (10 chars) or ISBN-13 (13 digits) formats with strict checksums. Non-ISBN EAN/UPC codes (e.g., 12-digit EAN) are rejected beforescrape.fetch.customis invoked. Discogs plugin registers onlyscrape.sources,scrape.fetch.custom, andscrape.data.modifyhooks—noscrape.isbn.validatebypass to widen acceptance. No alternative barcode entrypoint exists.The guard at
normalizeIsbnFields()(lines 776–780) preservessource === 'discogs'data, but the plugin never receives non-ISBN barcode inputs due to the validation wall at line 94. Barcode-first imports will fail with a 400 error before reaching the plugin.To enable EAN/UPC support as documented, either: (1) relax
isValidIsbn()to accept EAN formats, (2) add ascrape.isbn.validatehook in Discogs plugin to bypass validation, or (3) create a separate barcode endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ScrapeController.php` around lines 76 - 99, byIsbn() currently rejects non-ISBN barcodes so Discogs never sees EAN/UPC inputs; update the validation path: change isValidIsbn() to also accept common barcode formats (EAN-13, UPC-A 12, EAN-8) and ensure the cleaned value ($cleanIsbn) is normalized (strip non-digits, uppercase) before validation, keeping the existing hook call (\App\Support\Hooks::apply('scrape.isbn.validate', ...)); this lets barcode values pass the validation gate and reach scrape.fetch.custom and normalizeIsbnFields() where source === 'discogs' is preserved.app/Controllers/CsvImportController.php (1)
1229-1254:⚠️ Potential issue | 🟠 MajorGuard the
UPDATE libriwith the soft-delete predicate.
findExistingBook()filters deleted rows, but this standalone update can still hit a record that gets soft-deleted between lookup and write. Add the predicate here so the write path matches the repo contract.As per coding guidelines, `Every query on libri table MUST include soft-delete condition: AND deleted_at IS NULL`.🛡️ Minimal fix
- WHERE id = ? + WHERE id = ? AND deleted_at IS NULL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CsvImportController.php` around lines 1229 - 1254, The UPDATE statement in CsvImportController (the standalone "UPDATE libri SET ... WHERE id = ?") must include the soft-delete predicate to match findExistingBook() and avoid updating a row that was soft-deleted between lookup and write; modify the SQL in the CsvImportController update prepared statement to add "AND deleted_at IS NULL" (so the WHERE clause becomes "WHERE id = ? AND deleted_at IS NULL") so every write to the libri table respects the soft-delete contract.
♻️ Duplicate comments (2)
app/Support/MediaLabels.php (2)
93-95:⚠️ Potential issue | 🔴 CriticalEscape the fallback text before returning HTML.
These branches hand raw external text back from a helper that otherwise emits HTML fragments, so callers can end up echoing untrusted markup whenever the parser does not split the tracklist.
Safer fallback
- if ($tracks === false || count($tracks) < 2) { - return $text; // Not a tracklist, return as-is - } + if ($tracks === false || count($tracks) < 2) { + return nl2br(htmlspecialchars($text, ENT_QUOTES, 'UTF-8'), false); + } @@ - if (empty($items)) { - return $text; - } + if ($items === []) { + return nl2br(htmlspecialchars($text, ENT_QUOTES, 'UTF-8'), false); + }Also applies to: 107-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/MediaLabels.php` around lines 93 - 95, The helper returns raw external $text when parsing fails (if $tracks === false || count($tracks) < 2 and similarly at lines 107-108), which can emit untrusted markup; change those fallback returns to return an escaped string instead (e.g., replace return $text with return e($text) in Laravel or return htmlspecialchars($text, ENT_QUOTES, 'UTF-8') where e() isn't available) so callers always get safely escaped HTML when $tracks or $text are returned.
14-35:⚠️ Potential issue | 🟠 MajorTighten the music matcher so audiobooks stay out of it.
str_contains($lower, 'audio')makes values likeaudiolibro/audiobookreturntrue, which flips labels and tracklist rendering to music-specific UI for audiobook records.Suggested tightening
- /** `@var` array<int, string> */ - private static array $musicFormats = [ - 'cd_audio', 'vinile', 'cassetta', 'vinyl', 'lp', 'cd', 'cassette', 'audio', 'musik', 'music' - ]; + /** `@var` array<int, string> */ + private const MUSIC_FORMATS = [ + 'cd_audio', + 'vinile', + 'audiocassetta', + 'cassetta', + 'vinyl', + 'lp', + 'cd', + 'cassette', + ]; @@ - foreach (self::$musicFormats as $musicFormat) { - if (str_contains($lower, $musicFormat)) { - return true; - } - } - return false; + if (in_array($lower, self::MUSIC_FORMATS, true)) { + return true; + } + + return preg_match('/\b(?:music|musik)\b/i', $lower) === 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/MediaLabels.php` around lines 14 - 35, The isMusic method currently treats any occurrence of tokens like "audio" as music (via str_contains), causing "audiolibro"/"audiobook" to be misclassified; update MediaLabels::isMusic to match whole words or token boundaries instead of substring matches: replace the str_contains loop with a regex word-boundary check (e.g., use preg_match with \b and preg_quote for each entry) or remove overly-generic tokens like "audio" from the $musicFormats list and only keep explicit music identifiers; reference the static property MediaLabels::$musicFormats and the method MediaLabels::isMusic when making the change. Ensure null/empty handling remains unchanged and that matching is case-insensitive.
🤖 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/CsvImportController.php`:
- Line 773: When processing CSV columns for tipo_media, don't blindly synthesize
or overwrite on updates: in updateBook() only set tipo_media when the CSV
actually contains a non-null value (e.g., check array_key_exists and that the
value is not null/empty) and normalize that provided value via your media helper
instead of inferring; do not call MediaLabels::inferTipoMedia() in updateBook().
Leave inference with MediaLabels::inferTipoMedia() only in insertBook() when
creating a new record. Apply the same pattern to the other CSV upsert paths
referenced (the other places using 'tipo_media' around the file) so updates
preserve existing tipo_media when the CSV omits the column and inserts still get
a sensible default.
In `@app/Controllers/LibriController.php`:
- Line 652: Remove the hardcoded default that sets 'tipo_media' => 'libro' in
the payload so that scraped_tipo_media can be used by updateOptionals($id,
array_merge($data, $fields)); specifically, stop seeding 'tipo_media' in the
code block that currently inserts 'tipo_media' => 'libro' (and the duplicate
instance around the other occurrence noted) so the merged payload does not
always override scraped_tipo_media — leave tipo_media absent or null unless an
explicit, authoritative tipo_media is provided.
In `@app/Models/BookRepository.php`:
- Around line 646-649: The update path currently normalizes a missing tipo_media
to 'libro', overwriting existing values; change the logic in BookRepository so
tipo_media is only persisted when the caller explicitly provided it (check
array_key_exists('tipo_media', $data) or isset($data['tipo_media']) before
calling $addSet). In other words, keep the existing DB value unless tipo_media
is present in $data, and when present continue using normalizeEnumValue($val,
'tipo_media', 'libro') and $addSet('tipo_media', 's', ...).
In `@app/Support/MediaLabels.php`:
- Around line 41-53: The formatDisplayName mapping and lookup are missing common
token aliases (e.g., 'cd', 'vinyl', 'cassette', 'blu_ray' and other
spelling/underscore/hyphen variants), so update the private static array
$formatDisplayNames to include these alias keys pointing to the same display
values (e.g., add 'cd' => 'CD', 'vinyl' => 'Vinile'/'Vinyl' as appropriate,
'cassette' => 'Cassetta', 'blu_ray' => 'Blu-ray', plus any
space/underscore/hyphen variants), and also ensure formatDisplayName()
normalizes the incoming token (lowercase and normalize
underscores/hyphens/spaces) before lookup; apply the same alias
additions/normalization to the other equivalent mappings referenced in the
comment (the arrays/methods around the other ranges) so all code paths accept
the same tokens.
In `@app/Views/frontend/catalog-grid.php`:
- Around line 41-43: The badge outputs unescaped helper values into HTML
attributes (span title and i class) using
MediaLabels::tipoMediaDisplayName($book['tipo_media']) and
MediaLabels::icon($book['tipo_media']); wrap both outputs with
htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before echoing so translated labels
or icon names with quotes won’t break markup, e.g. escape the title value and
the class string for the <i> element while preserving the existing conditional
on $book['tipo_media'].
In `@app/Views/frontend/catalog.php`:
- Around line 1302-1326: The new tipo_media filter only updates the server but
the client never reapplies active states or label mapping; add a client-side
sync path analogous to syncAvailabilityActiveState() that listens for tipo_media
changes and updates the sidebar links' active class and the summary via
updateActiveFiltersDisplay(), and expose a JS label map built from
MediaLabels::allTypes() (emit the value=>label pairs from PHP into the page) so
updateActiveFiltersDisplay() shows human-friendly labels instead of raw enum
keys; ensure updateFilter('tipo_media', ...) triggers the same AJAX/DOM-refresh
flow as availability so the active state persists after AJAX updates.
- Around line 1318-1321: The inline JS argument currently passes raw PHP
interpolation for '<?= $tmValue ?>' into the onclick that calls
updateFilter('tipo_media', ...); fix by serializing the PHP value with
json_encode using JSON_HEX_TAG so the JS receives a safely escaped string;
update the onclick to call updateFilter('tipo_media', <serialized tmValue>)
where <serialized tmValue> is produced by json_encode($tmValue, JSON_HEX_TAG) to
prevent breaking quotes/HTML and follow the project's escaping guideline.
In `@app/Views/libri/index.php`:
- Around line 688-698: The render currently shows the enum key for tipo_media;
load the localized label map from MediaLabels::allTypes() into JS (e.g., a
mediaLabels object) and use it in the tipo_media column render: replace title
and displayed tooltip text with mediaLabels[data] || data (falling back to the
raw value). Do the same replacement where the active-filter chip is built (the
component that renders the selected tipo_media filter) so both the tooltip and
the chip show the localized label instead of the enum key.
- Around line 314-315: The DataTables order and CSS column selectors must be
updated to account for the new tipo_media column: change the JS order config
currently using order: [[3, 'asc']] so it sorts the Info/title column instead of
Cover (update it to order: [[4, 'asc']]); update the CSS selector that uses
nth-child(4) for text wrapping to nth-child(5) so it targets Info/title; and
update the mobile-hide rules that currently use nth-child(4)/hide the Media icon
so they instead target the Cover/Media positions as intended (use nth-child(4)
for hiding the Media icon per the comment). Locate and change the exact
occurrences of the strings order: [[3, 'asc']], nth-child(4) (text-wrap) and the
two nth-child rules around the mobile hide comments to the corrected indices.
In `@app/Views/libri/partials/book_form.php`:
- Around line 424-433: The rendered select must only use keys from
MediaLabels::allTypes(), so ensure any incoming scraper value (e.g.
data.tipo_media assigned to $book['tipo_media']) is validated/mapped before
rendering or saving; replace direct use of $book['tipo_media'] with a normalized
value derived by checking array_key_exists($book['tipo_media'],
MediaLabels::allTypes()) (or intersecting keys) and falling back to a default
like 'libro' (or null) — update the view logic around the select and the code
path that assigns $book['tipo_media'] from scraper input to perform this
whitelist check so saved and displayed values always match
MediaLabels::allTypes() keys.
- Around line 427-428: The option value is rendered raw in the loop over
\App\Support\MediaLabels::allTypes() (variables $value and $meta) — update the
option tag to escape the attribute by wrapping $value with
htmlspecialchars($value, ENT_QUOTES, 'UTF-8') when outputting the value
attribute (keep the comparison ($book['tipo_media'] ?? 'libro') === $value using
the original unescaped $value). Ensure ENT_QUOTES and UTF-8 are used to follow
the repo view-escaping guideline.
---
Outside diff comments:
In `@app/Controllers/CsvImportController.php`:
- Around line 1229-1254: The UPDATE statement in CsvImportController (the
standalone "UPDATE libri SET ... WHERE id = ?") must include the soft-delete
predicate to match findExistingBook() and avoid updating a row that was
soft-deleted between lookup and write; modify the SQL in the CsvImportController
update prepared statement to add "AND deleted_at IS NULL" (so the WHERE clause
becomes "WHERE id = ? AND deleted_at IS NULL") so every write to the libri table
respects the soft-delete contract.
In `@app/Controllers/ScrapeController.php`:
- Around line 76-99: byIsbn() currently rejects non-ISBN barcodes so Discogs
never sees EAN/UPC inputs; update the validation path: change isValidIsbn() to
also accept common barcode formats (EAN-13, UPC-A 12, EAN-8) and ensure the
cleaned value ($cleanIsbn) is normalized (strip non-digits, uppercase) before
validation, keeping the existing hook call
(\App\Support\Hooks::apply('scrape.isbn.validate', ...)); this lets barcode
values pass the validation gate and reach scrape.fetch.custom and
normalizeIsbnFields() where source === 'discogs' is preserved.
---
Duplicate comments:
In `@app/Support/MediaLabels.php`:
- Around line 93-95: The helper returns raw external $text when parsing fails
(if $tracks === false || count($tracks) < 2 and similarly at lines 107-108),
which can emit untrusted markup; change those fallback returns to return an
escaped string instead (e.g., replace return $text with return e($text) in
Laravel or return htmlspecialchars($text, ENT_QUOTES, 'UTF-8') where e() isn't
available) so callers always get safely escaped HTML when $tracks or $text are
returned.
- Around line 14-35: The isMusic method currently treats any occurrence of
tokens like "audio" as music (via str_contains), causing
"audiolibro"/"audiobook" to be misclassified; update MediaLabels::isMusic to
match whole words or token boundaries instead of substring matches: replace the
str_contains loop with a regex word-boundary check (e.g., use preg_match with \b
and preg_quote for each entry) or remove overly-generic tokens like "audio" from
the $musicFormats list and only keep explicit music identifiers; reference the
static property MediaLabels::$musicFormats and the method MediaLabels::isMusic
when making the change. Ensure null/empty handling remains unchanged and that
matching is case-insensitive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bb4cce7-9e90-491a-aac6-f9542e2d3b3c
📒 Files selected for processing (22)
app/Controllers/CsvImportController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Controllers/ScrapeController.phpapp/Models/BookRepository.phpapp/Support/MediaLabels.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog-grid.phpapp/Views/frontend/catalog.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.4.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/discogs/DiscogsPlugin.phptest-results/.last-run.json
✅ Files skipped from review due to trivial changes (4)
- test-results/.last-run.json
- installer/database/schema.sql
- locale/it_IT.json
- app/Views/libri/scheda_libro.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/frontend/book-detail.php
Comment on lines
+1302
to
+1326
| <!-- Media Type --> | ||
| <div class="filter-section"> | ||
| <div class="filter-title"> | ||
| <i class="fas fa-compact-disc"></i> | ||
| <?= __("Tipo Media") ?> | ||
| </div> | ||
| <div class="filter-options"> | ||
| <?php | ||
| $currentTipo = $filters['tipo_media'] ?? ''; | ||
| $tipoFilters = ['' => ['icon' => 'fa-th-large', 'label' => __('Tutti i media')]]; | ||
| foreach (\App\Support\MediaLabels::allTypes() as $tmValue => $tmMeta) { | ||
| $tipoFilters[$tmValue] = ['icon' => $tmMeta['icon'], 'label' => __($tmMeta['label'])]; | ||
| } | ||
| foreach ($tipoFilters as $tmValue => $tmInfo): | ||
| $isActive = $currentTipo === (string)$tmValue; | ||
| ?> | ||
| <a href="#" | ||
| class="filter-option <?= $isActive ? 'active' : '' ?>" | ||
| onclick="updateFilter('tipo_media', '<?= $tmValue ?>'); return false;"> | ||
| <i class="fas <?= $tmInfo['icon'] ?> me-1"></i> | ||
| <?= $tmInfo['label'] ?> | ||
| </a> | ||
| <?php endforeach; ?> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
tipo_media is only half-wired on the client.
After updateFilter('tipo_media', ...), the sidebar list never reapplies its .active state, and updateActiveFiltersDisplay() falls back to raw enum keys (disco, dvd, …) because this filter has no value-label mapping. Mirroring syncAvailabilityActiveState() with a media-type sync path and emitting a label map from MediaLabels::allTypes() would keep the new filter consistent after AJAX updates.
Also applies to: 1539-1556, 1614-1667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/frontend/catalog.php` around lines 1302 - 1326, The new tipo_media
filter only updates the server but the client never reapplies active states or
label mapping; add a client-side sync path analogous to
syncAvailabilityActiveState() that listens for tipo_media changes and updates
the sidebar links' active class and the summary via
updateActiveFiltersDisplay(), and expose a JS label map built from
MediaLabels::allTypes() (emit the value=>label pairs from PHP into the page) so
updateActiveFiltersDisplay() shows human-friendly labels instead of raw enum
keys; ensure updateFilter('tipo_media', ...) triggers the same AJAX/DOM-refresh
flow as availability so the active state persists after AJAX updates.
- Add extractCredits() for Discogs extra artists (producers, engineers) - Map all Discogs fields: styles→keywords, notes, weight, price, track count, catalog number, physical description, credits, URL - Fix isMusic() false positive on "audiolibro" (exact match, not substring) - Escape formatTracklist() fallback text (XSS prevention) - updateBasic() only persists tipo_media when explicitly provided - Add format display name aliases (cd, vinyl, cassette, blu_ray)
- EAN→Barcode label for music in frontend book-detail - HTML escaping in catalog-grid badge, catalog filter, book_form option - Remove tipo_media default from LibriController (let scraped values flow) - CSV import: null tipo_media on update preserves existing value - DataTable column index shift after tipo_media column - German "Cartaceo" → "Druckausgabe" - Remove Discogs early-return guard (let merge fill missing fields)
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Support/PluginManager.php (1)
45-53:⚠️ Potential issue | 🟠 MajorBundling Discogs here makes the “optional” plugin auto-activate.
Anything in
BUNDLED_PLUGINSis auto-registered withis_active = 1byautoRegisterBundledPlugins(). Addingdiscogshere means the next deploy/update turns it on for everyone, which bypasses the documented install/activate flow for an optional integration. Either keep Discogs out of the bundled allowlists, or add a bundled-but-disabled registration path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 45 - 53, The BUNDLED_PLUGINS constant currently includes 'discogs', which causes autoRegisterBundledPlugins() to auto-activate that integration; remove 'discogs' from the BUNDLED_PLUGINS array in the PluginManager class so it is not auto-registered, or alternatively implement a bundled-but-disabled path inside autoRegisterBundledPlugins() that sets is_active = 0 for specific plugin keys (e.g., special-case 'discogs' in autoRegisterBundledPlugins() to create the record but leave is_active false). Ensure references to BUNDLED_PLUGINS and autoRegisterBundledPlugins() are updated consistently.
♻️ Duplicate comments (13)
app/Views/frontend/catalog.php (1)
1539-1553:⚠️ Potential issue | 🟡 Minor
tipo_mediafilter still lacks active-state sync and human-readable value labels.After updates, the active tag can still display raw enum values (
disco,dvd, …), and only availability has a dedicated active-state sync path.Also applies to: 1614-1653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/catalog.php` around lines 1539 - 1553, The updateFilter function currently only triggers syncAvailabilityActiveState for 'disponibilita' but not for 'tipo_media', and active tags show raw enum values; add a dedicated sync for tipo_media (e.g., call a new or existing syncTipoMediaActiveState function from updateFilter when key === 'tipo_media') and ensure updateActiveFiltersDisplay (or the function that builds active tag labels) maps tipo_media enum keys ('disco','dvd', etc.) to human-readable labels before rendering; update any helper that reads currentFilters to use this mapping so active-state styles and display text are consistent.app/Views/libri/partials/book_form.php (1)
3615-3624:⚠️ Potential issue | 🟡 MinorValidate
data.tipo_mediaagainst allowed option keys before assignment.The JavaScript handler sets
tipoMediaSelect.valuedirectly fromdata.tipo_mediawithout validating that the value exists in the rendered<select>options. WhileScrapeControllerusesMediaLabels::inferTipoMedia()which returns valid keys, plugins like the Discogs scraper could return different values (e.g.,'cd','vinyl'), causing the select to show no selection or an invalid state.🛡️ Proposed fix to validate against allowed options
// Auto-set tipo_media from scraped data try { if (data.tipo_media) { const tipoMediaSelect = document.getElementById('tipo_media'); - if (tipoMediaSelect) { - tipoMediaSelect.value = data.tipo_media; + if (tipoMediaSelect && tipoMediaSelect.querySelector(`option[value="${CSS.escape(data.tipo_media)}"]`)) { + tipoMediaSelect.value = data.tipo_media; } } } catch (err) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 3615 - 3624, The handler sets tipo_mediaSelect.value directly from data.tipo_media without verifying the value exists in the rendered select options; update the logic around the tipo_media assignment (where data.tipo_media, tipoMediaSelect are referenced) to first check whether an option with that value exists (e.g., via tipoMediaSelect.querySelector or iterating tipoMediaSelect.options) and only set tipoMediaSelect.value if a matching option is present; if not present, skip the assignment or set a safe fallback, and keep references to ScrapeController/MediaLabels::inferTipoMedia in mind for expected keys.app/Views/libri/index.php (1)
688-697:⚠️ Potential issue | 🟡 MinorRender the localized media label here too.
The filter dropdown is localized, but the table tooltip and active-filter chip still show the raw enum key (
disco,audiolibro, ...). That leaves the UI partially untranslated and inconsistent with the filter options.Also applies to: 885-886
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 688 - 697, The media column render currently sets the icon title to the raw enum key (in the render function for the column with data: 'tipo_media'), causing untranslated tooltips; update that render to look up the localized label (use the same localization map/function used by the filter dropdown) and include it in the title/tooltip alongside the icon (e.g., title = localizedLabel || data). Also update the active-filter chip rendering (the code that builds the filter chips around lines 885-886) to display the localized label instead of the raw enum key, using the same localization lookup so both tooltip and chip match the dropdown translations.storage/plugins/discogs/views/settings.php (2)
14-18:⚠️ Potential issue | 🟠 MajorDon’t save the masked placeholder back as the token.
DiscogsPlugin::getSettings()returns********when a token exists, and this view posts that value back throughsaveSettings(). Opening the page and saving without edits will overwrite the real token with literal asterisks. Keep the field empty, show token presence separately, and only overwrite the stored value on an explicit replace/clear action.Also applies to: 96-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/views/settings.php` around lines 14 - 18, The form is currently writing the literal mask back into storage because the view posts the masked value; update the POST handling in the settings view so you only overwrite the stored token when the user explicitly provides a new value or requests a clear: when handling the save (the block using \App\Support\Csrf::validate and building $settings with 'api_token'), treat empty string or the mask value (DiscogsPlugin::getSettings() mask like "********") as “no change” (i.e., do not include api_token in $settings), and add/handle an explicit clear action (e.g., a separate checkbox or button whose POST field like clear_discogs_token signals to set api_token to empty) so saveSettings()/DiscogsPlugin::saveSettings() only updates the token on an explicit replace or clear.
133-137:⚠️ Potential issue | 🟡 MinorUse the named plugins route here.
Hardcoding the admin path in a view makes this link drift-prone if routes are renamed or localized. Build the
hreffrom the route helper and escape that result in the attribute.As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login" and "Usehtmlspecialchars()with ENT_QUOTES and UTF-8 encoding forroute_path()calls in HTML attributes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/views/settings.php` around lines 133 - 137, The view hardcodes "/admin/plugins" in url('/admin/plugins'); replace that with the named route helper and escape it: call route_path('plugins.index') (or the correct named key for the admin plugins route) and wrap the result in htmlspecialchars(..., ENT_QUOTES, 'UTF-8') for the href; alternatively use RouteTranslator::route('plugins.index') if your app uses that helper. Update the anchor to use htmlspecialchars(route_path('YOUR_ROUTE_KEY'), ENT_QUOTES, 'UTF-8') instead of url('/admin/plugins').app/Views/frontend/book-detail.php (1)
2187-2189:⚠️ Potential issue | 🟡 MinorUse the related item’s own media type for this fallback.
This still keys off the parent page’s
$isMusic, so a mixed related-items section can show "Artista sconosciuto" or "Autore sconosciuto" based on the current book instead of the related card.🤖 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 2187 - 2189, The fallback text for author vs artist currently uses the parent page's $isMusic; change it to use the related item's media flag so each related card decides its own label. Update the expression in the related card (where $related['autori'] is used) to compute a per-card flag from the related data, e.g. $relatedIsMusic = $related['isMusic'] ?? $related['media_type'] === 'music' ?? $isMusic, and then use that ($relatedIsMusic) instead of $isMusic when choosing __('Artista sconosciuto' : 'Autore sconosciuto'); ensure you reference the $related array keys that exist in your dataset (replace 'isMusic'/'media_type' with the actual key if different).tests/discogs-plugin.spec.js (2)
91-118:⚠️ Potential issue | 🟠 MajorScope install/activate/settings to the Discogs card and assert they exist.
Line 94 clicks the first
Installabutton on the page if the generic Discogs locator misses, so this suite can mutate a different plugin and then silently continue. Root the install, activate, and settings locators in the Discogs card and fail if that card or its controls are missing.Also applies to: 129-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 91 - 118, The test currently clicks a generic first "Installa" / "Attiva" button which can target another plugin; scope all install/activate/settings locators to the Discogs card (use the existing discogsCard locator and call discogsCard.locator(...) or similar) and assert the card and each control are present/visible before interacting, failing the test if the discogsCard or its install/activate/settings controls are missing; update usage of installBtn and activateBtn to be rooted off discogsCard and replace silent .catch(() => false) flows with explicit assertions so the test cannot mutate a different plugin.
193-202:⚠️ Potential issue | 🟠 MajorThis "frontend" assertion still exercises the admin page.
Line 194 requests
/admin/libri/${bookId}, so it never validates the public detail template or the music-specificBarcodelabel there. Resolve the real public book URL and assert against that response instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 193 - 202, The test is requesting the admin URL (`page.request.get(`${BASE}/admin/libri/${bookId}`)`) but intends to assert the public book detail template; change the request to the public path (`page.request.get(`${BASE}/libri/${bookId}`)`), keep the same HTML checks for 'Barcode' or music-specific labels, and update the comment to indicate it's validating the public detail page rather than the admin page; reference the variables BASE and bookId and the request call to locate the change.tests/discogs-import.spec.js (4)
155-164:⚠️ Potential issue | 🟠 MajorTreat a missing persisted row as a failure, not a skip.
Line 160 skips when neither lookup finds the saved record. That hides a broken save path and lets the remaining checks run on invalid state.
Suggested fix
- if (book === '') { - // Try isbn13 - const bookByIsbn = dbQuery( - `SELECT titolo, isbn13, formato FROM libri WHERE isbn13 LIKE '%720642442524%' AND deleted_at IS NULL LIMIT 1` - ); - test.skip(bookByIsbn === '', 'CD not found in database'); - if (bookByIsbn) { - expect(bookByIsbn.toLowerCase()).toContain('nevermind'); - } - return; - } - - expect(book.toLowerCase()).toContain('nevermind'); + let persisted = book; + if (persisted === '') { + persisted = dbQuery( + `SELECT titolo, isbn13, formato FROM libri WHERE isbn13 LIKE '%720642442524%' AND deleted_at IS NULL LIMIT 1` + ); + } + + expect(persisted).not.toBe(''); + expect(persisted.toLowerCase()).toContain('nevermind');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 155 - 164, The test currently treats a missing persisted row as a skip (see the book variable check and test.skip call). Replace the test.skip call with a failing assertion so that when dbQuery (the bookByIsbn lookup) returns no result the test fails immediately; e.g., assert or test.fail with a clear message that the expected DB row was not saved, and only run the expect(bookByIsbn.toLowerCase()).toContain(...) when bookByIsbn is truthy. Ensure you update the block around the book, dbQuery, test.skip, and expect references to reflect this change.
77-79:⚠️ Potential issue | 🟠 MajorFail this flow when import UI or scrape output is missing.
Lines 77-79 and 94-97 currently turn real regressions into skips. After plugin activation, a missing import button or an empty
titolomeans the Discogs path is broken.Suggested fix
- if (!await importBtn.isVisible({ timeout: 5000 }).catch(() => false)) { - test.skip(true, 'Import button not visible — no scraping plugin active'); - return; - } + await expect(importBtn).toBeVisible({ timeout: 5000 }); @@ - if (titleValue === '') { - // Scraping may have failed (rate limit, network). Check if any source populated data - test.skip(true, 'Scraping did not return data (possibly rate limited)'); - return; - } + expect( + titleValue, + `Discogs scrape returned no data for barcode ${TEST_BARCODE}` + ).not.toBe('');Also applies to: 94-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 77 - 79, The current checks silently skip on missing UI/scrape results; instead fail the test when the import button or the scraped titolo is missing by replacing the skip logic with a failing assertion or explicit throw—locate the importBtn visibility check (importBtn.isVisible(...)) and change the branch to call test.fail(...) or throw new Error('Import button not visible — no scraping plugin active'), and likewise find the titolo emptiness check (the variable titolo and its test.skip branch) and replace it with test.fail(...) or throw new Error('Scrape produced empty titolo — Discogs path broken') so missing UI or empty scrape output causes a test failure.
28-28:⚠️ Potential issue | 🟠 Major
serialhere does not prevent cross-file DB races.Line 28 only serializes tests inside this file. Playwright can still run the other DB-backed specs in parallel unless the runner is pinned to one worker via
/tmp/run-e2e.shor Playwright config.Expected result: either
workers: 1appears in Playwright config or the E2E runner passes--workers=1, and/tmp/run-e2e.shexports the requiredE2E_*variables.#!/bin/bash set -e echo "=== Playwright config ===" fd -i 'playwright.config.*' -x sh -c ' echo "--- $1 ---" sed -n "1,200p" "$1" | rg -n "workers|fullyParallel|testDir" ' sh {} echo echo "=== package.json scripts ===" fd -i 'package.json' -x sh -c ' echo "--- $1 ---" rg -n "\"(e2e|playwright|test)\"" "$1" -A2 -B1 ' sh {} echo echo "=== /tmp/run-e2e.sh ===" if [ -f /tmp/run-e2e.sh ]; then sed -n '1,200p' /tmp/run-e2e.sh | rg -n 'playwright|workers|E2E_' else echo "/tmp/run-e2e.sh not available in sandbox" fiBased on learnings, "E2E tests with Playwright require
/tmp/run-e2e.shto set DB/admin credentials as env vars. Always use--workers=1for serial execution".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` at line 28, The test uses test.describe.serial('Discogs Import: full scraping flow') which only serializes tests inside this file and does not prevent cross-file DB races; update the E2E runner so Playwright runs with a single worker (either set workers: 1 in the Playwright config or ensure the test runner is invoked with --workers=1) and verify /tmp/run-e2e.sh exports the required E2E_* environment variables (DB/admin creds) before launching Playwright; ensure any CI or package.json e2e script that calls Playwright includes the --workers=1 flag or the config change so DB-backed specs cannot run in parallel.
50-52:⚠️ Potential issue | 🟠 MajorClean up both barcode persistence paths.
Line 52 only deletes by
ean, but Lines 118-122 and 157-159 treatisbn13as a valid save target too. A previous run can leave theisbn13row behind and make the DB assertions pass against stale data.Suggested fix
test.afterAll(async () => { // Cleanup test data - try { dbExec("DELETE FROM libri WHERE ean = '0720642442524' AND deleted_at IS NULL"); } catch {} + try { + const normalizedBarcode = TEST_BARCODE.slice(1); + dbExec( + `DELETE FROM libri + WHERE (ean = '${TEST_BARCODE}' OR isbn13 LIKE '%${normalizedBarcode}%') + AND deleted_at IS NULL` + ); + } catch {} await context?.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 50 - 52, The test.afterAll cleanup only deletes rows by ean and misses rows saved under isbn13; update the cleanup in the test.afterAll block that calls dbExec so it removes both persistence paths for the barcode used in tests (targeting the libri table), e.g. run a single DELETE that includes "WHERE (ean = '0720642442524' OR isbn13 = '0720642442524') AND deleted_at IS NULL" or execute two dbExec deletes (one for ean and one for isbn13) to ensure no stale rows remain for the test barcode.storage/plugins/discogs/DiscogsPlugin.php (1)
696-752:⚠️ Potential issue | 🟠 MajorDon't round-trip
api_tokenthrough raw SQL.Lines 725-739 and 796-813 read and write the Discogs personal access token verbatim from
plugin_settings. That stores a secret in plaintext and creates a second settings path that can drift from the app's centralized plugin-settings handling.Also applies to: 761-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/DiscogsPlugin.php` around lines 696 - 752, The getSetting()/getSettings() code directly queries plugin_settings and returns the api_token plaintext; remove this direct SQL access and stop round-tripping the secret through plugin_settings. Replace the manual SQL in getSetting (and any code that writes api_token) with calls to the app's central plugin-settings API/manager (use the existing centralized accessor used elsewhere in the project) so the Discogs plugin reads/writes the token through the unified settings layer and preserves secret handling (no plaintext return in getSettings — mask or omit the token). Update any references to $this->pluginId/$this->db in getSetting to use that centralized API instead of raw queries.
🧹 Nitpick comments (1)
app/Models/BookRepository.php (1)
1000-1000:tipo_mediais not normalized inupdateOptionals.Unlike
createBasicandupdateBasic, this path storestipo_media(andscraped_tipo_media) without passing throughnormalizeEnumValue(). If a scraper returns an unexpected value, it could violate the DB enum constraint.Consider normalizing the value:
Suggested improvement
} else { - $cols[$c] = $data[$c]; + $cols[$c] = $c === 'tipo_media' + ? $this->normalizeEnumValue((string) $data[$c], 'tipo_media', 'libro') + : $data[$c]; }And similarly for
scraped_tipo_media:if ($this->hasColumn('tipo_media') && !array_key_exists('tipo_media', $cols) && !empty($data['scraped_tipo_media'])) { - $cols['tipo_media'] = (string) $data['scraped_tipo_media']; + $cols['tipo_media'] = $this->normalizeEnumValue((string) $data['scraped_tipo_media'], 'tipo_media', 'libro'); }Also applies to: 1055-1057
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` at line 1000, In updateOptionals, the loop that assigns optional fields (foreach over ['numero_pagine', 'ean', ... 'tipo_media']) currently writes tipo_media and scraped_tipo_media without normalization; modify updateOptionals to pass values for 'tipo_media' and 'scraped_tipo_media' through the same normalizeEnumValue() used by createBasic/updateBasic before persisting to ensure DB enum constraints aren't violated (identify the assignment site inside updateOptionals where tipo_media and scraped_tipo_media are set and replace direct assignment with normalizeEnumValue($value) for those keys).
🤖 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/PluginController.php`:
- Around line 398-406: The code currently ignores the boolean result of
$this->pluginManager->setSetting($pluginId, 'api_token', $apiToken, true) so the
endpoint can return success even if persistence failed; update the handler in
PluginController to capture the return value (e.g. $saved =
$this->pluginManager->setSetting(...)), and if $saved is false return an error
JSON response and appropriate non-200 status (and do not claim has_token true),
otherwise continue to return the success payload; adjust the response body and
status accordingly so the API accurately reflects whether setSetting succeeded.
In `@app/Support/MediaLabels.php`:
- Around line 21-38: The function isMusic should let the provided $tipoMedia be
authoritative: change isMusic so the first check is if ($tipoMedia !== null)
return $tipoMedia === 'disco'; i.e. if tipo_media is set, return true only for
the explicit music type(s) you consider music (e.g. 'disco') and otherwise
return false, instead of falling back to evaluating $formato; update the
early-return logic in isMusic to use $tipoMedia as the decisive value.
- Around line 165-182: The inferTipoMedia function misclassifies cassette
formats like "audiocassetta" as libro; update the matching list inside
inferTipoMedia to include the cassette variants used elsewhere (e.g., add
patterns such as 'audiocassetta', 'audio_cassetta', 'audio-cassetta', and any
spaced forms like 'audio cassetta') so those inputs hit the cassette branch and
return 'disco' (modify the array in the foreach that currently contains
['cd_audio', 'vinile', 'lp', 'cd', 'vinyl', 'cassetta', 'cassette'] to include
the additional cassette variants).
In `@app/Views/frontend/book-detail.php`:
- Around line 27-29: The media-type detection is duplicated: $isMusic is derived
from both 'tipo_media' and fallback 'formato' using
\App\Support\MediaLabels::isMusic, but the hero badge and schemaOrgType() still
read 'tipo_media' directly; amend the template to resolve the media type once
(e.g., compute $isMusic and/or a normalized $resolvedMediaType using
MediaLabels::isMusic and the 'formato' fallback) and then pass or use that
resolved value when rendering the hero badge, the tracklist/music labels, and
when calling schemaOrgType() so all places (hero badge, JSON‑LD, and labels)
consistently treat records inferred as music from formato as music.
In `@app/Views/frontend/catalog.php`:
- Around line 1321-1322: Escape the dynamic icon class and label when rendering
the media filter option: replace unescaped uses of $tmInfo['icon'] and
$tmInfo['label'] in the view with htmlspecialchars(..., ENT_QUOTES, 'UTF-8')
(e.g., wrap $tmInfo['icon'] passed into the <i class="fas ..."> and wrap
$tmInfo['label'] output) so both the HTML attribute and visible text are
properly escaped; update the occurrences in frontend/catalog.php where
$tmInfo['icon'] and $tmInfo['label'] are rendered.
In `@app/Views/libri/scheda_libro.php`:
- Around line 8-10: The header badge is still reading only libro['tipo_media']
while $isMusic uses a fallback to formato; fix by resolving a single media type
variable (e.g., $mediaType = $libro['tipo_media'] ?? $libro['formato'] ?? '')
and use that for both the music detection (via
\App\Support\MediaLabels::isMusic($libro['formato'] ?? null,
$libro['tipo_media'] ?? null) or better
\App\Support\MediaLabels::isMusic($mediaType)) and the header badge logic
(replace direct uses of libro['tipo_media'] in the badge rendering and the block
around lines 72-75 with the resolved $mediaType) so the badge and $isMusic
detection use the same fallback source.
In `@installer/database/migrations/migrate_0.5.4.sql`:
- Around line 1-38: The migration file migrate_0.5.4.sql is named for a higher
version than the packaged release (pinakes-v0.5.3.zip.sha256) so
Updater::runMigrations() (which only runs migrations with version <= $toVersion)
will skip creating libri.tipo_media; fix by either renaming migrate_0.5.4.sql to
migrate_0.5.3.sql so its version ≤ the packaged release, or increment the
packaged release/version to 0.5.4 (and update pinakes-v0.5.3.zip.sha256
accordingly) so the migration is applied—ensure the migration filename version
matches the release version used by Updater::runMigrations().
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 203-206: When Discogs barcode lookup returns no hits within
fetchFromDiscogs(), don't return immediately; implement the promised
title/artist fallback by using the provided $currentResult (access
$currentResult->title and $currentResult->author) to perform a query-based
search before giving up. Update the barcode-null handling path (and the similar
block around the earlier return) to call the existing query/search routine with
those title/author values as a targeted fallback, and only return after this
fallback has been attempted and exhausted.
In `@storage/plugins/discogs/README.md`:
- Around line 49-70: The README and current implementation use a fixed 1s pause
between Discogs requests which allows ~60/min and will cause 429s for
unauthenticated usage; update the plugin's rate-limiting logic to be token-aware
by checking the `api_token` setting and applying a different throttle: when
`api_token` is present allow 1s between requests (≈60/min), otherwise set the
delay to at least 2.4s per request (≈25/min) or implement a tokenless budget of
25 requests/min; also add 429 handling with exponential backoff/retry and log
details so the path that reads `api_token` and the rate limiter (the
module/function that enforces the 1-second pause) uses the new token-aware
delays.
- Around line 15-17: Update the fallback search example in the README: replace
the incorrect barcode placeholder in the second step (`GET
/database/search?q={ean}&type=release`) with a title-based query such as `GET
/database/search?q={title}&type=release` or `GET
/database/search?q={title}+{artist}&type=release` to match the described
second-pass title/title+artist search; keep the first and third steps (`GET
/database/search?barcode={ean}&type=release` and `GET /releases/{id}`)
unchanged.
In `@test-results/.last-run.json`:
- Around line 1-4: Remove the ephemeral test runner state file (.last-run.json)
from version control and stop committing it: delete the tracked file from the
repo (git rm --cached .last-run.json or remove then commit), add an entry for
.last-run.json to your .gitignore (or the appropriate test-results/ pattern),
and commit the change so future Playwright runs won't create PR churn; verify no
other ephemeral test artifacts are still tracked in the repository.
In `@tests/discogs-import.spec.js`:
- Around line 181-188: The test's guard uses the `formato` query which can be
wrong and skips the label assertion; change the DB query to fetch `tipo_media`
instead (use dbQuery to SELECT tipo_media FROM libri WHERE id = ${bookId}) and
replace the `if (formato && ['cd_audio' ...].some(...))` check with a check that
runs the label assertion when `tipo_media === 'disco'`; keep the `hasMusicLabel`
check (looking at `content.includes('Etichetta')` / `Label` / `Anno di Uscita` /
`Release Year'`) and `expect(hasMusicLabel).toBe(true)` so the assertion doesn't
become a no-op when `formato` mapping is wrong.
---
Outside diff comments:
In `@app/Support/PluginManager.php`:
- Around line 45-53: The BUNDLED_PLUGINS constant currently includes 'discogs',
which causes autoRegisterBundledPlugins() to auto-activate that integration;
remove 'discogs' from the BUNDLED_PLUGINS array in the PluginManager class so it
is not auto-registered, or alternatively implement a bundled-but-disabled path
inside autoRegisterBundledPlugins() that sets is_active = 0 for specific plugin
keys (e.g., special-case 'discogs' in autoRegisterBundledPlugins() to create the
record but leave is_active false). Ensure references to BUNDLED_PLUGINS and
autoRegisterBundledPlugins() are updated consistently.
---
Duplicate comments:
In `@app/Views/frontend/book-detail.php`:
- Around line 2187-2189: The fallback text for author vs artist currently uses
the parent page's $isMusic; change it to use the related item's media flag so
each related card decides its own label. Update the expression in the related
card (where $related['autori'] is used) to compute a per-card flag from the
related data, e.g. $relatedIsMusic = $related['isMusic'] ??
$related['media_type'] === 'music' ?? $isMusic, and then use that
($relatedIsMusic) instead of $isMusic when choosing __('Artista sconosciuto' :
'Autore sconosciuto'); ensure you reference the $related array keys that exist
in your dataset (replace 'isMusic'/'media_type' with the actual key if
different).
In `@app/Views/frontend/catalog.php`:
- Around line 1539-1553: The updateFilter function currently only triggers
syncAvailabilityActiveState for 'disponibilita' but not for 'tipo_media', and
active tags show raw enum values; add a dedicated sync for tipo_media (e.g.,
call a new or existing syncTipoMediaActiveState function from updateFilter when
key === 'tipo_media') and ensure updateActiveFiltersDisplay (or the function
that builds active tag labels) maps tipo_media enum keys ('disco','dvd', etc.)
to human-readable labels before rendering; update any helper that reads
currentFilters to use this mapping so active-state styles and display text are
consistent.
In `@app/Views/libri/index.php`:
- Around line 688-697: The media column render currently sets the icon title to
the raw enum key (in the render function for the column with data:
'tipo_media'), causing untranslated tooltips; update that render to look up the
localized label (use the same localization map/function used by the filter
dropdown) and include it in the title/tooltip alongside the icon (e.g., title =
localizedLabel || data). Also update the active-filter chip rendering (the code
that builds the filter chips around lines 885-886) to display the localized
label instead of the raw enum key, using the same localization lookup so both
tooltip and chip match the dropdown translations.
In `@app/Views/libri/partials/book_form.php`:
- Around line 3615-3624: The handler sets tipo_mediaSelect.value directly from
data.tipo_media without verifying the value exists in the rendered select
options; update the logic around the tipo_media assignment (where
data.tipo_media, tipoMediaSelect are referenced) to first check whether an
option with that value exists (e.g., via tipoMediaSelect.querySelector or
iterating tipoMediaSelect.options) and only set tipoMediaSelect.value if a
matching option is present; if not present, skip the assignment or set a safe
fallback, and keep references to ScrapeController/MediaLabels::inferTipoMedia in
mind for expected keys.
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 696-752: The getSetting()/getSettings() code directly queries
plugin_settings and returns the api_token plaintext; remove this direct SQL
access and stop round-tripping the secret through plugin_settings. Replace the
manual SQL in getSetting (and any code that writes api_token) with calls to the
app's central plugin-settings API/manager (use the existing centralized accessor
used elsewhere in the project) so the Discogs plugin reads/writes the token
through the unified settings layer and preserves secret handling (no plaintext
return in getSettings — mask or omit the token). Update any references to
$this->pluginId/$this->db in getSetting to use that centralized API instead of
raw queries.
In `@storage/plugins/discogs/views/settings.php`:
- Around line 14-18: The form is currently writing the literal mask back into
storage because the view posts the masked value; update the POST handling in the
settings view so you only overwrite the stored token when the user explicitly
provides a new value or requests a clear: when handling the save (the block
using \App\Support\Csrf::validate and building $settings with 'api_token'),
treat empty string or the mask value (DiscogsPlugin::getSettings() mask like
"********") as “no change” (i.e., do not include api_token in $settings), and
add/handle an explicit clear action (e.g., a separate checkbox or button whose
POST field like clear_discogs_token signals to set api_token to empty) so
saveSettings()/DiscogsPlugin::saveSettings() only updates the token on an
explicit replace or clear.
- Around line 133-137: The view hardcodes "/admin/plugins" in
url('/admin/plugins'); replace that with the named route helper and escape it:
call route_path('plugins.index') (or the correct named key for the admin plugins
route) and wrap the result in htmlspecialchars(..., ENT_QUOTES, 'UTF-8') for the
href; alternatively use RouteTranslator::route('plugins.index') if your app uses
that helper. Update the anchor to use
htmlspecialchars(route_path('YOUR_ROUTE_KEY'), ENT_QUOTES, 'UTF-8') instead of
url('/admin/plugins').
In `@tests/discogs-import.spec.js`:
- Around line 155-164: The test currently treats a missing persisted row as a
skip (see the book variable check and test.skip call). Replace the test.skip
call with a failing assertion so that when dbQuery (the bookByIsbn lookup)
returns no result the test fails immediately; e.g., assert or test.fail with a
clear message that the expected DB row was not saved, and only run the
expect(bookByIsbn.toLowerCase()).toContain(...) when bookByIsbn is truthy.
Ensure you update the block around the book, dbQuery, test.skip, and expect
references to reflect this change.
- Around line 77-79: The current checks silently skip on missing UI/scrape
results; instead fail the test when the import button or the scraped titolo is
missing by replacing the skip logic with a failing assertion or explicit
throw—locate the importBtn visibility check (importBtn.isVisible(...)) and
change the branch to call test.fail(...) or throw new Error('Import button not
visible — no scraping plugin active'), and likewise find the titolo emptiness
check (the variable titolo and its test.skip branch) and replace it with
test.fail(...) or throw new Error('Scrape produced empty titolo — Discogs path
broken') so missing UI or empty scrape output causes a test failure.
- Line 28: The test uses test.describe.serial('Discogs Import: full scraping
flow') which only serializes tests inside this file and does not prevent
cross-file DB races; update the E2E runner so Playwright runs with a single
worker (either set workers: 1 in the Playwright config or ensure the test runner
is invoked with --workers=1) and verify /tmp/run-e2e.sh exports the required
E2E_* environment variables (DB/admin creds) before launching Playwright; ensure
any CI or package.json e2e script that calls Playwright includes the --workers=1
flag or the config change so DB-backed specs cannot run in parallel.
- Around line 50-52: The test.afterAll cleanup only deletes rows by ean and
misses rows saved under isbn13; update the cleanup in the test.afterAll block
that calls dbExec so it removes both persistence paths for the barcode used in
tests (targeting the libri table), e.g. run a single DELETE that includes "WHERE
(ean = '0720642442524' OR isbn13 = '0720642442524') AND deleted_at IS NULL" or
execute two dbExec deletes (one for ean and one for isbn13) to ensure no stale
rows remain for the test barcode.
In `@tests/discogs-plugin.spec.js`:
- Around line 91-118: The test currently clicks a generic first "Installa" /
"Attiva" button which can target another plugin; scope all
install/activate/settings locators to the Discogs card (use the existing
discogsCard locator and call discogsCard.locator(...) or similar) and assert the
card and each control are present/visible before interacting, failing the test
if the discogsCard or its install/activate/settings controls are missing; update
usage of installBtn and activateBtn to be rooted off discogsCard and replace
silent .catch(() => false) flows with explicit assertions so the test cannot
mutate a different plugin.
- Around line 193-202: The test is requesting the admin URL
(`page.request.get(`${BASE}/admin/libri/${bookId}`)`) but intends to assert the
public book detail template; change the request to the public path
(`page.request.get(`${BASE}/libri/${bookId}`)`), keep the same HTML checks for
'Barcode' or music-specific labels, and update the comment to indicate it's
validating the public detail page rather than the admin page; reference the
variables BASE and bookId and the request call to locate the change.
---
Nitpick comments:
In `@app/Models/BookRepository.php`:
- Line 1000: In updateOptionals, the loop that assigns optional fields (foreach
over ['numero_pagine', 'ean', ... 'tipo_media']) currently writes tipo_media and
scraped_tipo_media without normalization; modify updateOptionals to pass values
for 'tipo_media' and 'scraped_tipo_media' through the same normalizeEnumValue()
used by createBasic/updateBasic before persisting to ensure DB enum constraints
aren't violated (identify the assignment site inside updateOptionals where
tipo_media and scraped_tipo_media are set and replace direct assignment with
normalizeEnumValue($value) for those keys).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 739bc1b7-a6b4-4f3f-8906-47428c6773b5
📒 Files selected for processing (33)
.gitignoreapp/Controllers/CsvImportController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/PublicApiController.phpapp/Controllers/ScrapeController.phpapp/Models/BookRepository.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog-grid.phpapp/Views/frontend/catalog.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.4.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonpinakes-v0.5.3.zip.sha256storage/plugins/discogs/DiscogsPlugin.phpstorage/plugins/discogs/README.mdstorage/plugins/discogs/plugin.jsonstorage/plugins/discogs/views/settings.phpstorage/plugins/discogs/wrapper.phptest-results/.last-run.jsontests/discogs-import.spec.jstests/discogs-plugin.spec.js
Comment on lines
+15
to
+17
| 1. Ricerca per barcode (EAN/UPC) -- `GET /database/search?barcode={ean}&type=release` | ||
| 2. Se nessun risultato, ricerca per query -- `GET /database/search?q={ean}&type=release` | ||
| 3. Recupero dettagli completi della release -- `GET /releases/{id}` |
There was a problem hiding this comment.
Fix the fallback search example.
Line 16 still shows the barcode placeholder in the fallback query. The PR objective says the second pass is title search, so this doc should describe the actual title/title+artist query instead of q={ean}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/plugins/discogs/README.md` around lines 15 - 17, Update the fallback
search example in the README: replace the incorrect barcode placeholder in the
second step (`GET /database/search?q={ean}&type=release`) with a title-based
query such as `GET /database/search?q={title}&type=release` or `GET
/database/search?q={title}+{artist}&type=release` to match the described
second-pass title/title+artist search; keep the first and third steps (`GET
/database/search?barcode={ean}&type=release` and `GET /releases/{id}`)
unchanged.
- isMusic(): tipoMedia is now authoritative (no formato fallback when set) - inferTipoMedia(): add audiocassetta to disco formats - Settings: empty password field, skip save on empty, "Token configurato" indicator - PluginController: check setSetting result, return 500 on failure - Resolve tipoMedia once for badge + Schema.org in book-detail and scheda_libro - README: remove fallback search, document token-aware rate limiting - Escape catalog filter icon/label with htmlspecialchars - normalizeEnumValue in updateOptionals for tipo_media - Remove test-results/.last-run.json from tracking - Add "Token configurato" translations (IT/EN/DE)
1. Admin list tipo_media=disco filter via DataTable API 2. CSV export includes tipo_media column with correct value 3. Schema.org tipo_media stored as 'disco' vs 'libro' in DB 4. Admin detail: music labels + tracklist vs book labels + prose 5. Edit persistence: tipo_media='disco' survives form save
Was showing raw 'Tracklist: 1. Come Together (4:19) 2. Something...' as plain text. Now calls MediaLabels::formatTracklist() which converts to <ol class='tracklist'> with individual <li> per track. Also shows music icon + 'Tracklist' label instead of 'Descrizione'.
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
Single plugin, three sources in cascade: 1. Discogs (barcode search, primary) 2. MusicBrainz + Cover Art Archive (barcode fallback, open data) 3. Deezer (cover HD enrichment by title+artist) - MusicBrainz: barcode search, full release with artists/labels/recordings - Cover Art Archive: front cover in high resolution - Deezer: cover_xl (1000x1000) when other sources lack images - Each API has independent rate limiting (1s Discogs, 1.1s MB, 1s Deezer) - Plugin display name: "Music Scraper (Discogs, MusicBrainz, Deezer)"
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Support/Updater.php (1)
39-47:⚠️ Potential issue | 🟠 MajorRollback does not cover bundled plugin files.
Adding
discogshere meansinstallUpdate()will updatestorage/plugins/discogsviaupdateBundledPlugins(), butbackupAppFiles()/restoreAppFiles()never snapshotstorage/plugins. If install or migrations fail after that copy, rollback restores the core and leaves Discogs on the newer version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/Updater.php` around lines 39 - 47, The rollback path doesn't snapshot storage/plugins so updating bundled plugins (BUNDLED_PLUGINS used by updateBundledPlugins() called from installUpdate()) can leave plugins upgraded when core is restored; modify backupAppFiles() and restoreAppFiles() to include the storage/plugins directory (or alternatively make updateBundledPlugins() write plugin updates to a temporary location and atomically move them only after install/migrations succeed), ensuring any plugin paths touched by BUNDLED_PLUGINS are backed up and restored on failure.app/Support/PluginManager.php (1)
45-53:⚠️ Potential issue | 🟠 MajorThis makes Discogs auto-active, not optional.
autoRegisterBundledPlugins()inserts bundled plugins withis_active = 1and immediately runsonActivate(). Once the Discogs folder ships in the app package, startup/update will silently enable it for every instance instead of waiting for an admin opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 45 - 53, The bundled "discogs" entry is being auto-enabled because autoRegisterBundledPlugins() inserts entries with is_active = 1 and immediately calls onActivate(); change the behavior so bundled optional plugins are registered but not auto-activated: either remove 'discogs' from the BUNDLED_PLUGINS constant and add it to a new BUNDLED_OPTIONAL_PLUGINS list, or modify autoRegisterBundledPlugins() to insert with is_active = 0 and skip calling onActivate() for items flagged optional (check for a new optionalPlugins array or a lookup), keeping onActivate() calls only for truly required plugins; update references to BUNDLED_PLUGINS, autoRegisterBundledPlugins(), is_active and onActivate() accordingly.app/Controllers/CsvImportController.php (1)
1231-1256:⚠️ Potential issue | 🟡 MinorAdd the soft-delete guard to this
libriupdate.
findExistingBook()already filters out soft-deleted rows, but this write still updates byidalone. A concurrent soft-delete between lookup and update will mutate archived data, and the query also violates the repository rule forlibri.🩹 Minimal fix
- WHERE id = ? + WHERE id = ? AND deleted_at IS NULLAs per coding guidelines, "Every query on
libritable MUST include soft-delete condition:AND deleted_at IS NULL".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CsvImportController.php` around lines 1231 - 1256, The UPDATE statement prepared in CsvImportController (the $stmt prepared for table libri) must include the soft-delete guard so we only update non-deleted rows; modify the WHERE clause from "WHERE id = ?" to "WHERE id = ? AND deleted_at IS NULL" (or equivalent parameterized condition) so a concurrent soft-delete won’t allow updates to archived rows; ensure the change is applied to the prepared SQL used by the update path that complements findExistingBook().
♻️ Duplicate comments (14)
app/Views/libri/partials/book_form.php (1)
3615-3624:⚠️ Potential issue | 🟡 MinorWhitelist and normalize scraped
tipo_mediabefore assigning it.At Line 3620, assigning raw
data.tipo_mediacan silently miss valid select keys (e.g., provider sendsCD/vinyl) and leave the form in an inconsistent state. Restrict assignment to known keys fromMediaLabels::allTypes().🔧 Suggested fix
// Auto-set tipo_media from scraped data try { if (data.tipo_media) { const tipoMediaSelect = document.getElementById('tipo_media'); - if (tipoMediaSelect) { - tipoMediaSelect.value = data.tipo_media; + const allowedTipoMedia = new Set(<?= json_encode(array_keys(\App\Support\MediaLabels::allTypes()), JSON_HEX_TAG) ?>); + const normalizedTipoMedia = String(data.tipo_media).trim().toLowerCase(); + if (tipoMediaSelect && allowedTipoMedia.has(normalizedTipoMedia)) { + tipoMediaSelect.value = normalizedTipoMedia; } } } catch (err) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 3615 - 3624, The code sets the select `#tipo_media` directly from data.tipo_media which may contain variants (e.g., "CD", "vinyl") and leaves the select unset; instead whitelist and normalize the incoming value against the canonical keys from MediaLabels::allTypes(): create a mapping of allowed keys/aliases (server-side output of MediaLabels::allTypes() into a JS object or array), normalize data.tipo_media (trim, lower/upper-case as needed), look up the normalized value in that whitelist (or map aliases to canonical keys) and only assign tipoMediaSelect.value when a match exists; keep the try/catch but log or silently skip when no valid match is found to avoid inconsistent form state.app/Views/frontend/catalog.php (1)
1302-1326:⚠️ Potential issue | 🟡 Minor
tipo_mediafilter is still only partially wired on the client.Line 1302-1326 adds the filter UI, but there is still no client-side active-state sync path (like availability), and active-filter tags still show raw enum keys rather than user-facing labels.
Suggested patch
@@ - <a href="#" - class="filter-option <?= $isActive ? 'active' : '' ?>" + <a href="#" + class="filter-option <?= $isActive ? 'active' : '' ?>" + data-media-filter="<?= htmlspecialchars((string) $tmValue, ENT_QUOTES, 'UTF-8') ?>" onclick="updateFilter('tipo_media', <?= htmlspecialchars(json_encode((string) $tmValue, JSON_HEX_TAG | JSON_HEX_APOS), ENT_QUOTES, 'UTF-8') ?>); return false;"> @@ $i18nTranslations = [ @@ 'errore_caricamento' => __('Errore nel caricamento. Riprova.') ]; $i18nJson = json_encode($i18nTranslations, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_HEX_TAG); +$mediaTypeLabels = ['' => __('Tutti i media')]; +foreach (\App\Support\MediaLabels::allTypes() as $tmValue => $tmMeta) { + $mediaTypeLabels[(string) $tmValue] = __($tmMeta['label']); +} +$mediaTypeLabelsJson = json_encode($mediaTypeLabels, JSON_UNESCAPED_UNICODE | JSON_HEX_TAG); @@ const i18n = {$i18nJson}; +const MEDIA_TYPE_LABELS = {$mediaTypeLabelsJson}; @@ syncAvailabilityActiveState(); + syncMediaTypeActiveState(); }); @@ if (key === 'disponibilita') { syncAvailabilityActiveState(); + } else if (key === 'tipo_media') { + syncMediaTypeActiveState(); } @@ +function syncMediaTypeActiveState() { + const currentValue = currentFilters.tipo_media || ''; + document.querySelectorAll('[data-media-filter]').forEach((option) => { + option.classList.toggle('active', (option.dataset.mediaFilter || '') === currentValue); + }); +} + function updateActiveFiltersDisplay() { @@ - } else if (filterKey === 'genere_id') { + } else if (filterKey === 'genere_id') { displayValue = currentGenreName || value; + } else if (filterKey === 'tipo_media') { + displayValue = MEDIA_TYPE_LABELS[value] || value; }Also applies to: 1460-1460, 1622-1622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/catalog.php` around lines 1302 - 1326, The tipo_media UI is added server-side but not fully wired to the client: ensure the client-side active-state sync mirrors how availability works by having updateFilter('tipo_media', ...) trigger the same JS path that sets/unsets the .filter-option.active class and updates the active-filter tags; use the user-facing labels from MediaLabels::allTypes() (the $tipoFilters entries / $tmInfo['label']) when rendering active-filter tags instead of raw enum keys so displayed tags show localized labels, and ensure any JS that reads currentTipo (filters['tipo_media']) uses the stringified values you json_encode in the onclick to match server/state comparison.app/Controllers/CsvImportController.php (1)
773-775:⚠️ Potential issue | 🟠 MajorNormalize
tipo_mediabefore binding it to the enum column.
parseCsvRow()now forwards the raw CSV cell unchanged, and both new SQL branches persist that value directly. Inputs likeCD,Vinyl,Audiobook, or localized labels won't match thelibri.tipo_mediaenum, so the import can fail or store an unintended value. Normalize/validate the CSV value to the canonical enum (libro,disco,audiolibro,dvd,altro) before it reaches the query.Also applies to: 1269-1292, 1360-1390
app/Views/frontend/book-detail.php (1)
2192-2193:⚠️ Potential issue | 🟡 MinorUse each related card's media flag for the fallback author label.
This still keys off the parent page's
$isMusic, so a mixed related-items section can showArtista sconosciutoon a book card orAutore sconosciutoon a music card.🤖 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 2192 - 2193, The fallback author label currently uses the parent $isMusic flag; change it to use the related item’s own media flag by switching the ternary to check $related's isMusic flag first (e.g. $related['isMusic'] ?? $isMusic) so the fallback text for <?= 'autori' ?> is chosen per related card rather than the parent page.storage/plugins/discogs/views/settings.php (1)
138-142:⚠️ Potential issue | 🟡 MinorUse the named plugins route for this backlink.
This link is still bound to
/admin/plugins, so it will drift if the admin route is renamed or localized. Use the existing named route helper and escape that result in thehref.As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/views/settings.php` around lines 138 - 142, Replace the hardcoded url('/admin/plugins') used in the anchor href with the named-route helper and escape its result; specifically, in the anchor where htmlspecialchars(url('/admin/plugins')...) is used, call htmlspecialchars(route_path('key'), ENT_QUOTES, 'UTF-8') (or RouteTranslator::route('key') if your app uses that), keeping the same ENT_QUOTES and UTF-8 arguments so the backlink uses the named route instead of a hardcoded path.app/Views/libri/index.php (1)
688-697:⚠️ Potential issue | 🟡 MinorUse the localized media label here, not the enum key.
The dropdown is already powered by
MediaLabels::allTypes(), but the icon tooltip and the active-filter chip still show raw values likediscoandaudiolibro, so this UI stays partially untranslated. Reuse the same label map in both places.Also applies to: 885-886
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 688 - 697, The column render currently uses the raw enum key (data) for the icon tooltip; replace that with the localized label from the same mapping used for the dropdown (use MediaLabels::allTypes() or the label map it returns) so the tooltip shows the translated media name; update the render in the 'tipo_media' column (and the similar code at the other occurrence around the active-filter chip) to look up the label by key (fallback to a default like 'libro') and use that value in the title attribute instead of the raw key.tests/discogs-import.spec.js (4)
50-53:⚠️ Potential issue | 🟠 MajorTeardown still misses the
isbn13persistence path.Tests 3, 5, and 6 explicitly accept records saved through
isbn13, butafterAll()only deletes byean. A previous run can therefore leave the imported row behind and make later runs pass against stale data.Suggested fix
- try { dbExec("DELETE FROM libri WHERE ean = '0720642442524' AND deleted_at IS NULL"); } catch {} + try { + dbExec( + "DELETE FROM libri WHERE (ean = '0720642442524' OR isbn13 LIKE '%720642442524%') AND deleted_at IS NULL" + ); + } catch {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 50 - 53, The teardown in test.afterAll only deletes by ean and misses records persisted via isbn13; update the cleanup to also remove rows where isbn13 = '0720642442524' (either add a second dbExec call deleting where isbn13 matches or expand the SQL to delete where ean = '0720642442524' OR isbn13 = '0720642442524'), keep the try/catch around dbExec, and still await context?.close(); reference test.afterAll, dbExec, isbn13, ean, and context.close when making the change.
56-67:⚠️ Potential issue | 🟠 MajorThe main import suite still turns product failures into skips.
After the env-var gate, an inactive plugin, missing import button, or empty
titolomeans the Discogs flow failed. Usingtest.skip()here lets the “full scraping flow” suite go green without actually exercising the import path.Suggested fix
- test.skip(parseInt(isActiveNow) === 0, 'Discogs plugin could not be activated'); + expect(parseInt(isActiveNow)).toBeGreaterThan(0); @@ - if (!await importBtn.isVisible({ timeout: 5000 }).catch(() => false)) { - test.skip(true, 'Import button not visible — no scraping plugin active'); - return; - } + await expect(importBtn).toBeVisible({ timeout: 5000 }); @@ - if (titleValue === '') { - // Scraping may have failed (rate limit, network). Check if any source populated data - test.skip(true, 'Scraping did not return data (possibly rate limited)'); - return; - } + expect(titleValue, `Discogs scrape returned no data for barcode ${TEST_BARCODE}`).not.toBe('');Also applies to: 77-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 56 - 67, The current test turns real failures into skips by calling test.skip(...) when the Discogs plugin isn't active or import prerequisites are missing; change this to cause a test failure instead: replace the test.skip(...) calls with an explicit failing assertion or throw (e.g., use expect(parseInt(isActiveNow)).toBeGreaterThan(0) or throw new Error('Discogs plugin could not be activated')) so the suite fails when the plugin/import button/titolo check fails; apply the same change for the similar block referenced around lines 77-97 and keep references to dbQuery, page.goto and the surrounding test('1. Verify Discogs plugin is active', ...) block to locate the code.
155-164:⚠️ Potential issue | 🟠 MajorMissing persisted row should fail this suite, not skip it.
If neither lookup finds the saved album, the save step regressed.
test.skip()hides that failure and lets the remaining checks continue with invalid state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 155 - 164, The test currently calls test.skip when dbQuery returns an empty result (bookByIsbn === '') which hides the failure; change this to actively fail the test instead—replace test.skip(bookByIsbn === '', 'CD not found in database') with an explicit failure (for example throw new Error('CD not found in database') or use your test framework's fail API) so that when dbQuery (dbQuery / bookByIsbn) yields no row the spec fails immediately and subsequent assertions (expect(...).toContain) do not run on invalid state.
180-188:⚠️ Potential issue | 🟠 MajorDon’t guard the only label assertion behind a
formatoheuristic.A broken format mapping just bypasses the check, so this test never proves the saved record is treated as music. Assert
tipo_media === 'disco'(or assert the persistedformatoseparately) and keep the label expectation mandatory for that branch.Suggested fix
- const formato = dbQuery(`SELECT formato FROM libri WHERE id = ${bookId}`); - - if (formato && ['cd_audio', 'vinile', 'cd', 'vinyl'].some(f => formato.toLowerCase().includes(f))) { - // Music labels should be active - const hasMusicLabel = content.includes('Etichetta') || content.includes('Label') || - content.includes('Anno di Uscita') || content.includes('Release Year'); - expect(hasMusicLabel).toBe(true); - } + const tipoMedia = dbQuery( + `SELECT tipo_media FROM libri WHERE id = ${bookId} AND deleted_at IS NULL LIMIT 1` + ); + expect(tipoMedia).toBe('disco'); + + const hasMusicLabel = content.includes('Etichetta') || content.includes('Label') || + content.includes('Anno di Uscita') || content.includes('Release Year'); + expect(hasMusicLabel).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 180 - 188, The test currently skips the music-label assertion based on a fragile formato heuristic; instead query the persisted tipo_media for the record (use dbQuery to SELECT tipo_media FROM libri WHERE id = ${bookId}) and assert expect(tipo_media).toBe('disco') (or separately assert the persisted formato equals a music format) and then always assert the label fields are present by checking content (hasMusicLabel) with expect(hasMusicLabel).toBe(true); keep references to the existing formato variable only if you add a separate assertion for the stored formato, and use the same bookId and content variables already in the test.tests/discogs-plugin.spec.js (2)
183-202:⚠️ Potential issue | 🟠 MajorThis “frontend” check still only exercises the admin template.
Requesting
${BASE}/admin/libri/${bookId}will not catch regressions in the public detail page or the Barcode-vs-ISBN label logic there. Resolve the public book URL and assert the frontend HTML directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 183 - 202, Test "6. Frontend: music book shows Barcode instead of ISBN-13" is hitting the admin template (`page.request.get(${BASE}/admin/libri/${bookId})`) so it doesn't validate the public/front-end labeling; change the request to the public book detail URL (use the public route pattern used elsewhere, e.g. `${BASE}/libri/${bookId}` or the app's public resolver) and re-run the same HTML checks against that response (inspect the returned HTML for 'Barcode' or music-specific labels) so the spec exercises the public detail page rather than the admin page.
91-109:⚠️ Potential issue | 🟠 MajorScope install/activate/settings actions to Discogs and fail when its card is missing.
button:has-text("Installa").first()can act on a different plugin, and Test 3 silently passes if the Discogs settings link never appears. Root those locators in the Discogs card and add explicit assertions for card visibility, activation, and token-field visibility before continuing.Also applies to: 121-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 91 - 109, The test currently uses global button locators that may target other plugins; update code to scope install/activate/settings actions to the Discogs card by deriving installBtn, activateBtn, and settings link from the discogsCard locator (discogsCard.locator(...)) and add explicit assertions that discogsCard.isVisible() throws/returns false -> fail the test, that activation actually occurred (assert activateBtn becomes hidden or card shows active state) and that the token field (e.g., the Discogs token input) is visible before proceeding; ensure any .isVisible({timeout}) checks use assertions (not silent .catch(() => false)) so the test fails fast if the Discogs card is missing or actions do not succeed.storage/plugins/discogs/DiscogsPlugin.php (2)
696-739:⚠️ Potential issue | 🟠 Major
api_tokenis still split across raw SQL and the encrypted settings API.
getSetting()readsplugin_settingsverbatim while the controller save path stores encryptedENC:values, so authenticated requests can end up sending ciphertext as the Discogs token.saveSettings()then writes plaintext back into the same table, which regresses secret handling in the opposite direction. Route both reads and writes through the samePluginManagersettings API.Also applies to: 761-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/DiscogsPlugin.php` around lines 696 - 739, getSetting() and saveSettings() are directly reading/writing plugin_settings (returning/storing raw or ENC: values) causing plaintext/ciphertext mismatch for api_token; switch both read and write paths to use the centralized PluginManager settings API instead of direct SQL. Locate getSetting() and saveSettings() in DiscogsPlugin and replace the direct DB SELECT/UPDATE logic that touches plugin_settings and pluginId with calls to the PluginManager methods that retrieve and store plugin settings (the encrypted-aware getters/setters provided by the PluginManager), ensuring you pass the plugin identifier (this->pluginId or resolved name) and the 'api_token' key so the API handles encryption/decryption consistently. Ensure you remove the direct SQL access (prepare/execute/bind_param) for those keys and rely solely on the PluginManager for both reads and writes.
223-231:⚠️ Potential issue | 🟠 MajorThe promised title fallback still never runs.
Line 230 returns immediately on a barcode miss, so releases without Discogs barcode metadata are unreachable even though this method is documented as “barcode, then query search.” If title/artist are required for the fallback, thread them into this hook or move the fallback to a stage that has them before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/DiscogsPlugin.php` around lines 223 - 231, The method currently returns $currentResult immediately when the barcode search yields no results (see $searchResult check), preventing the documented "barcode, then query search" fallback from ever running; instead, remove the early return and invoke the fallback query using title/artist (pull them from $currentResult if available) or call a dedicated fallback helper (e.g., a new or existing method like searchByTitleArtist) which builds a non-barcode $searchUrl and calls apiRequest with the same $token; if title/artist are not present in this scope, thread them into this hook (pass them into the method) or move the fallback invocation to the earlier stage that has those fields before returning.
🧹 Nitpick comments (1)
app/Controllers/ScrapeController.php (1)
776-780: Avoid baking a scraper name into core normalization.This branch is really about EAN-only music payloads, not Discogs specifically. Keying it off
source === 'discogs'means the next non-book scraper will need another core special-case instead of using a provider-agnostic signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ScrapeController.php` around lines 776 - 780, The normalization branch is keyed to ($data['source'] ?? '') === 'discogs', which bakes a provider name into core logic; instead detect EAN-only music payloads generically (e.g., check for presence of a barcode/EAN and absence of isbn13/isbn10 or a provider-agnostic flag). Update the ScrapeController.php branch that currently returns $data for Discogs to something like: if (isset($data['barcode']) && empty($data['isbn13']) && empty($data['isbn10'])) return $data; or use a new payload field like is_ean_only set by scrapers, and reference the existing variables $data['source'], $data['barcode'], $data['isbn13'], and $data['isbn10'] when making the change.
🤖 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 900-904: Guard the filter addition so we only add the
"l.tipo_media" condition when the column actually exists: before pushing to
conditions[], params[] and appending to types, check schema availability for the
column (e.g. use your DB schema helper like hasColumn('lotes','tipo_media') or
query information_schema) and only run the existing block when that check passes
and filters['tipo_media'] is not empty; update the code paths around
filters['tipo_media'], conditions[], params[] and types to be conditional on the
schema check to avoid "Unknown column 'l.tipo_media'" errors.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 914-915: parseLibraryThingRow() currently sets
$result['tipo_media'] via \App\Support\MediaLabels::inferTipoMedia(...)
unconditionally, causing updates to overwrite an existing tipo_media (e.g.,
disco/dvd) with a default like libro; change the logic so parseLibraryThingRow()
leaves $result['tipo_media'] null unless inferTipoMedia(...) returns a
recognized/valid value, and update the update branches (the code paths that
write back $result['tipo_media'] on matched imports) to skip writing tipo_media
when it is null; only apply the default inference (call inferTipoMedia and set
tipo_media if null) in the insert/new-record path so new imports get a default
but updates do not overwrite an existing tipo_media.
In `@app/Models/BookRepository.php`:
- Around line 303-306: The code currently sets tipo_media only from
$data['tipo_media'] (and similar for scraped_tipo_media), causing records that
only provide formato (as in the Discogs flow) to default to 'libro'; update the
logic in the blocks that call hasColumn('tipo_media') and normalizeEnumValue to
first check for a canonical tipo_media, then fall back to $data['formato'] (map
known formato values like CD/LP/cassette to their corresponding tipo_media)
before finally defaulting to 'libro'; apply the same fallback change to the
other occurrence that handles scraped_tipo_media so both paths use formato when
tipo_media is absent, using the existing normalizeEnumValue and addField
functions to validate and set the field.
In `@app/Views/frontend/book-detail.php`:
- Around line 164-166: The JSON-LD currently sets `@type` via
\App\Support\MediaLabels::schemaOrgType($resolvedTipoMedia) but always builds
$bookSchema with book-specific properties (bookEdition, isbn, numberOfPages);
change schema construction to branch on the resolved type (use the value
returned by MediaLabels::schemaOrgType($resolvedTipoMedia) or the
$resolvedTipoMedia variable) and create a media-specific schema object for
non-Book types (e.g., MusicRecording, Movie) that omits book-only fields and
adds appropriate properties for that media; update code around the $bookSchema
variable so you build and output the correct schema shape per type instead of
only switching `@type`.
In `@installer/database/migrations/migrate_0.5.4.sql`:
- Around line 27-38: The migration's UPDATEs use exact LOWER(formato) IN (...)
matching which misses values matched at runtime by
App\Support\MediaLabels::inferTipoMedia() (which uses substring checks); update
the three UPDATE statements in migrate_0.5.4.sql to use the same
substring/ILIKE/REGEXP rules as inferTipoMedia() (e.g., check LOWER(formato)
LIKE '%cd%' OR '%vinyl%' OR '%lp%' OR '%cassette%' for disco, patterns for
'audiolibro' like '%audio%' or '%audiobook%', and patterns for 'dvd' like
'%dvd%' OR '%blu%' OR '%blu-ray%'/'%blu_ray%') so existing rows get the same
tipo_media classification as the runtime MediaLabels::inferTipoMedia() logic.
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 277-291: The current early return in DiscogsPlugin (the check if
(($data['source'] ?? '') !== 'discogs') in DiscogsPlugin.php) prevents cover
enrichment when a discogs_id exists but source isn't 'discogs'; change the
gating logic to base enrichment on the presence of discogs_id instead of source:
remove or bypass the source check and allow the subsequent code that reads
$discogsId = $data['discogs_id'] ?? null and calls
$this->getSetting('api_token'), self::API_BASE . '/releases/' . (int)$discogsId
and $this->apiRequest(...) to run whenever discogs_id is present (null check
remains), so covers are backfilled even when BookDataMerger::merge() left source
unchanged.
In `@storage/plugins/discogs/plugin.json`:
- Around line 11-12: The manifest declares requires_app and max_app_version but
PluginManager only validates requires_php, so either add app-version validation
to PluginManager or remove those fields from the manifest; to fix, update
PluginManager to read the manifest's requires_app and max_app_version, parse
semantic versions, fetch the running application's version (use the existing
app-version source in your app, e.g. an Application.version or APP_VERSION
constant), then enforce that runningVersion >= requires_app and runningVersion
<= max_app_version and fail install/activation with a clear error, or if you
prefer the simpler change, remove the requires_app and max_app_version entries
from the plugin manifest to avoid promising unsupported compatibility.
In `@tests/discogs-advanced.spec.js`:
- Around line 116-139: The test "3. Schema.org JSON-LD type is MusicAlbum for
disco" currently only checks storage via apiResp/musicBook and DB queries
(musicBook, musicBookId, bookBookId); update it to actually fetch and assert the
frontend JSON-LD `@type`: navigate to the public detail page for the found
musicBook (or construct its public URL) and parse the <script
type="application/ld+json"> payload, then assert that the parsed JSON's "@type"
equals "MusicAlbum" for the disco record and "Book" for the libro record;
alternatively, if you intend to keep it as a storage-level test, rename the test
title to reflect that it only validates tipo_media in API/DB.
- Around line 52-60: The teardown is only deleting seeded rows by their original
titolo suffix so if a test (Test 5) renames a row the cleanup can miss it;
update the teardown (afterAll) to remove fixtures by stable identifiers instead
of mutable titles—e.g., call dbExec DELETE statements targeting the seeded ean
'1234567890123' and isbn13 '9781234567897' (and any other fixed identifiers used
in the inserts) or use a robust predicate like WHERE titolo LIKE 'E2E_ADV_%'
combined with OR ean=... OR isbn13=...; apply the same change for all teardown
spots related to the inserts created by RUN_ID (the dbExec insert sites and the
afterAll reset) so stale rows cannot survive a failed reset.
---
Outside diff comments:
In `@app/Controllers/CsvImportController.php`:
- Around line 1231-1256: The UPDATE statement prepared in CsvImportController
(the $stmt prepared for table libri) must include the soft-delete guard so we
only update non-deleted rows; modify the WHERE clause from "WHERE id = ?" to
"WHERE id = ? AND deleted_at IS NULL" (or equivalent parameterized condition) so
a concurrent soft-delete won’t allow updates to archived rows; ensure the change
is applied to the prepared SQL used by the update path that complements
findExistingBook().
In `@app/Support/PluginManager.php`:
- Around line 45-53: The bundled "discogs" entry is being auto-enabled because
autoRegisterBundledPlugins() inserts entries with is_active = 1 and immediately
calls onActivate(); change the behavior so bundled optional plugins are
registered but not auto-activated: either remove 'discogs' from the
BUNDLED_PLUGINS constant and add it to a new BUNDLED_OPTIONAL_PLUGINS list, or
modify autoRegisterBundledPlugins() to insert with is_active = 0 and skip
calling onActivate() for items flagged optional (check for a new optionalPlugins
array or a lookup), keeping onActivate() calls only for truly required plugins;
update references to BUNDLED_PLUGINS, autoRegisterBundledPlugins(), is_active
and onActivate() accordingly.
In `@app/Support/Updater.php`:
- Around line 39-47: The rollback path doesn't snapshot storage/plugins so
updating bundled plugins (BUNDLED_PLUGINS used by updateBundledPlugins() called
from installUpdate()) can leave plugins upgraded when core is restored; modify
backupAppFiles() and restoreAppFiles() to include the storage/plugins directory
(or alternatively make updateBundledPlugins() write plugin updates to a
temporary location and atomically move them only after install/migrations
succeed), ensuring any plugin paths touched by BUNDLED_PLUGINS are backed up and
restored on failure.
---
Duplicate comments:
In `@app/Views/frontend/book-detail.php`:
- Around line 2192-2193: The fallback author label currently uses the parent
$isMusic flag; change it to use the related item’s own media flag by switching
the ternary to check $related's isMusic flag first (e.g. $related['isMusic'] ??
$isMusic) so the fallback text for <?= 'autori' ?> is chosen per related card
rather than the parent page.
In `@app/Views/frontend/catalog.php`:
- Around line 1302-1326: The tipo_media UI is added server-side but not fully
wired to the client: ensure the client-side active-state sync mirrors how
availability works by having updateFilter('tipo_media', ...) trigger the same JS
path that sets/unsets the .filter-option.active class and updates the
active-filter tags; use the user-facing labels from MediaLabels::allTypes() (the
$tipoFilters entries / $tmInfo['label']) when rendering active-filter tags
instead of raw enum keys so displayed tags show localized labels, and ensure any
JS that reads currentTipo (filters['tipo_media']) uses the stringified values
you json_encode in the onclick to match server/state comparison.
In `@app/Views/libri/index.php`:
- Around line 688-697: The column render currently uses the raw enum key (data)
for the icon tooltip; replace that with the localized label from the same
mapping used for the dropdown (use MediaLabels::allTypes() or the label map it
returns) so the tooltip shows the translated media name; update the render in
the 'tipo_media' column (and the similar code at the other occurrence around the
active-filter chip) to look up the label by key (fallback to a default like
'libro') and use that value in the title attribute instead of the raw key.
In `@app/Views/libri/partials/book_form.php`:
- Around line 3615-3624: The code sets the select `#tipo_media` directly from
data.tipo_media which may contain variants (e.g., "CD", "vinyl") and leaves the
select unset; instead whitelist and normalize the incoming value against the
canonical keys from MediaLabels::allTypes(): create a mapping of allowed
keys/aliases (server-side output of MediaLabels::allTypes() into a JS object or
array), normalize data.tipo_media (trim, lower/upper-case as needed), look up
the normalized value in that whitelist (or map aliases to canonical keys) and
only assign tipoMediaSelect.value when a match exists; keep the try/catch but
log or silently skip when no valid match is found to avoid inconsistent form
state.
In `@storage/plugins/discogs/DiscogsPlugin.php`:
- Around line 696-739: getSetting() and saveSettings() are directly
reading/writing plugin_settings (returning/storing raw or ENC: values) causing
plaintext/ciphertext mismatch for api_token; switch both read and write paths to
use the centralized PluginManager settings API instead of direct SQL. Locate
getSetting() and saveSettings() in DiscogsPlugin and replace the direct DB
SELECT/UPDATE logic that touches plugin_settings and pluginId with calls to the
PluginManager methods that retrieve and store plugin settings (the
encrypted-aware getters/setters provided by the PluginManager), ensuring you
pass the plugin identifier (this->pluginId or resolved name) and the 'api_token'
key so the API handles encryption/decryption consistently. Ensure you remove the
direct SQL access (prepare/execute/bind_param) for those keys and rely solely on
the PluginManager for both reads and writes.
- Around line 223-231: The method currently returns $currentResult immediately
when the barcode search yields no results (see $searchResult check), preventing
the documented "barcode, then query search" fallback from ever running; instead,
remove the early return and invoke the fallback query using title/artist (pull
them from $currentResult if available) or call a dedicated fallback helper
(e.g., a new or existing method like searchByTitleArtist) which builds a
non-barcode $searchUrl and calls apiRequest with the same $token; if
title/artist are not present in this scope, thread them into this hook (pass
them into the method) or move the fallback invocation to the earlier stage that
has those fields before returning.
In `@storage/plugins/discogs/views/settings.php`:
- Around line 138-142: Replace the hardcoded url('/admin/plugins') used in the
anchor href with the named-route helper and escape its result; specifically, in
the anchor where htmlspecialchars(url('/admin/plugins')...) is used, call
htmlspecialchars(route_path('key'), ENT_QUOTES, 'UTF-8') (or
RouteTranslator::route('key') if your app uses that), keeping the same
ENT_QUOTES and UTF-8 arguments so the backlink uses the named route instead of a
hardcoded path.
In `@tests/discogs-import.spec.js`:
- Around line 50-53: The teardown in test.afterAll only deletes by ean and
misses records persisted via isbn13; update the cleanup to also remove rows
where isbn13 = '0720642442524' (either add a second dbExec call deleting where
isbn13 matches or expand the SQL to delete where ean = '0720642442524' OR isbn13
= '0720642442524'), keep the try/catch around dbExec, and still await
context?.close(); reference test.afterAll, dbExec, isbn13, ean, and
context.close when making the change.
- Around line 56-67: The current test turns real failures into skips by calling
test.skip(...) when the Discogs plugin isn't active or import prerequisites are
missing; change this to cause a test failure instead: replace the test.skip(...)
calls with an explicit failing assertion or throw (e.g., use
expect(parseInt(isActiveNow)).toBeGreaterThan(0) or throw new Error('Discogs
plugin could not be activated')) so the suite fails when the plugin/import
button/titolo check fails; apply the same change for the similar block
referenced around lines 77-97 and keep references to dbQuery, page.goto and the
surrounding test('1. Verify Discogs plugin is active', ...) block to locate the
code.
- Around line 155-164: The test currently calls test.skip when dbQuery returns
an empty result (bookByIsbn === '') which hides the failure; change this to
actively fail the test instead—replace test.skip(bookByIsbn === '', 'CD not
found in database') with an explicit failure (for example throw new Error('CD
not found in database') or use your test framework's fail API) so that when
dbQuery (dbQuery / bookByIsbn) yields no row the spec fails immediately and
subsequent assertions (expect(...).toContain) do not run on invalid state.
- Around line 180-188: The test currently skips the music-label assertion based
on a fragile formato heuristic; instead query the persisted tipo_media for the
record (use dbQuery to SELECT tipo_media FROM libri WHERE id = ${bookId}) and
assert expect(tipo_media).toBe('disco') (or separately assert the persisted
formato equals a music format) and then always assert the label fields are
present by checking content (hasMusicLabel) with
expect(hasMusicLabel).toBe(true); keep references to the existing formato
variable only if you add a separate assertion for the stored formato, and use
the same bookId and content variables already in the test.
In `@tests/discogs-plugin.spec.js`:
- Around line 183-202: Test "6. Frontend: music book shows Barcode instead of
ISBN-13" is hitting the admin template
(`page.request.get(${BASE}/admin/libri/${bookId})`) so it doesn't validate the
public/front-end labeling; change the request to the public book detail URL (use
the public route pattern used elsewhere, e.g. `${BASE}/libri/${bookId}` or the
app's public resolver) and re-run the same HTML checks against that response
(inspect the returned HTML for 'Barcode' or music-specific labels) so the spec
exercises the public detail page rather than the admin page.
- Around line 91-109: The test currently uses global button locators that may
target other plugins; update code to scope install/activate/settings actions to
the Discogs card by deriving installBtn, activateBtn, and settings link from the
discogsCard locator (discogsCard.locator(...)) and add explicit assertions that
discogsCard.isVisible() throws/returns false -> fail the test, that activation
actually occurred (assert activateBtn becomes hidden or card shows active state)
and that the token field (e.g., the Discogs token input) is visible before
proceeding; ensure any .isVisible({timeout}) checks use assertions (not silent
.catch(() => false)) so the test fails fast if the Discogs card is missing or
actions do not succeed.
---
Nitpick comments:
In `@app/Controllers/ScrapeController.php`:
- Around line 776-780: The normalization branch is keyed to ($data['source'] ??
'') === 'discogs', which bakes a provider name into core logic; instead detect
EAN-only music payloads generically (e.g., check for presence of a barcode/EAN
and absence of isbn13/isbn10 or a provider-agnostic flag). Update the
ScrapeController.php branch that currently returns $data for Discogs to
something like: if (isset($data['barcode']) && empty($data['isbn13']) &&
empty($data['isbn10'])) return $data; or use a new payload field like
is_ean_only set by scrapers, and reference the existing variables
$data['source'], $data['barcode'], $data['isbn13'], and $data['isbn10'] when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2a2b3b5-5827-45bc-a23d-e5cedd7a20fa
📒 Files selected for processing (33)
.gitignoreapp/Controllers/CsvImportController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/PublicApiController.phpapp/Controllers/ScrapeController.phpapp/Models/BookRepository.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog-grid.phpapp/Views/frontend/catalog.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.4.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonpinakes-v0.5.3.zip.sha256storage/plugins/discogs/DiscogsPlugin.phpstorage/plugins/discogs/README.mdstorage/plugins/discogs/plugin.jsonstorage/plugins/discogs/views/settings.phpstorage/plugins/discogs/wrapper.phptests/discogs-advanced.spec.jstests/discogs-import.spec.jstests/discogs-plugin.spec.js
Comment on lines
+11
to
+12
| "requires_app": "0.5.0", | ||
| "max_app_version": "1.0.0", |
There was a problem hiding this comment.
Don't rely on requires_app / max_app_version as a compatibility guard yet.
PluginManager currently validates only requires_php and just persists requires_app, so these manifest entries won't actually block incompatible Pinakes versions from installing or activating this plugin. Either add the app-version checks in PluginManager or drop the unsupported compatibility promise from the manifest for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/plugins/discogs/plugin.json` around lines 11 - 12, The manifest
declares requires_app and max_app_version but PluginManager only validates
requires_php, so either add app-version validation to PluginManager or remove
those fields from the manifest; to fix, update PluginManager to read the
manifest's requires_app and max_app_version, parse semantic versions, fetch the
running application's version (use the existing app-version source in your app,
e.g. an Application.version or APP_VERSION constant), then enforce that
runningVersion >= requires_app and runningVersion <= max_app_version and fail
install/activation with a clear error, or if you prefer the simpler change,
remove the requires_app and max_app_version entries from the plugin manifest to
avoid promising unsupported compatibility.
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
Backend: - CsvImportController: soft-delete guard on all UPDATE libri - CSV parseCsvRow: normalize tipo_media through inferTipoMedia - FrontendController: hasColumn guard for tipo_media filter - LT import: tipo_media null on update (preserve existing), infer only on insert - BookRepository createBasic: fallback from formato when tipo_media absent - Migration: LIKE patterns instead of exact IN() for broader matching - Discogs enrichment: gate on discogs_id, not source Schema.org: - MusicAlbum: byArtist, recordLabel, numTracks, EAN identifier - Movie: director, productionCompany, EAN identifier - Audiobook: author, publisher, readBy (narrator), isbn - Book: full properties (author, translator, illustrator, editor, isbn, issn, pages, edition) Tests: - Cleanup by ean/isbn13 (stable identifiers, not mutable titles)
fabiodalez-dev
force-pushed
the
feature/87-discogs-plugin
branch
from
a383add to
71af10c
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Controllers/LibriApiController.php (1)
153-162:⚠️ Potential issue | 🟡 MinorDescending position sort only reverses the last key.
Line 157 expands to
ORDER BY s.codice, m.numero_livello, COALESCE(...) DESC, so a descending sort still keeps shelf and level in ascending order. Users sorting by position descending will get a partially reversed result set.🐛 Proposed fix
- $orderByMap = [ - 4 => 'l.titolo', // Info column - sort by title - 5 => 'g.nome', // Genre column - 6 => 's.codice, m.numero_livello, COALESCE(l.posizione_progressiva, p.ordine)', // Position - 7 => 'l.anno_pubblicazione', // Year column - ]; - - $orderByClause = $orderByMap[$orderColumn] ?? 'l.titolo'; - $orderBy = "ORDER BY {$orderByClause} {$orderDir}"; + if ($orderColumn === 6) { + $orderBy = "ORDER BY s.codice {$orderDir}, m.numero_livello {$orderDir}, COALESCE(l.posizione_progressiva, p.ordine) {$orderDir}"; + } else { + $orderByMap = [ + 4 => 'l.titolo', // Info column - sort by title + 5 => 'g.nome', // Genre column + 7 => 'l.anno_pubblicazione', // Year column + ]; + + $orderByClause = $orderByMap[$orderColumn] ?? 'l.titolo'; + $orderBy = "ORDER BY {$orderByClause} {$orderDir}"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 153 - 162, The position sort uses a multi-column expression in $orderByMap[6] but the current code only appends {$orderDir} once, causing only the last key to be reversed; change the logic so when $orderColumn == 6 you build the ORDER BY clause with the direction applied to each component (e.g. s.codice, m.numero_livello, COALESCE(...) all suffixed with DESC when $orderDir === 'DESC'), by replacing the static $orderByMap entry for key 6 with conditional code that constructs $orderByClause for position using $orderDir applied to each field before forming $orderBy.app/Support/PluginManager.php (2)
132-173:⚠️ Potential issue | 🟠 MajorGuard the bundled-plugin INSERT
prepare()beforebind_param().Unlike the SELECT and UPDATE branches in the same method, this INSERT path never checks whether
prepare()returnedfalse. If that statement cannot be prepared (e.g., due to a database schema issue), callingbind_param()will fatal and plugin auto-registration stops instead of logging and continuing.Suggested fix
$stmt = $this->db->prepare(" INSERT INTO plugins ( name, display_name, description, version, author, author_url, plugin_url, is_active, path, main_file, requires_php, requires_app, metadata, installed_at ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, NOW()) "); + if ($stmt === false) { + SecureLogger::error("[PluginManager] Failed to prepare bundled plugin insert for $pluginName", [ + 'db_error' => $this->db->error, + ]); + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 132 - 173, The INSERT branch creating $stmt via $this->db->prepare(...) does not check for a false return before calling $stmt->bind_param(...); update the code in PluginManager (around the INSERT prepare) to verify $stmt !== false (or check for mysqli_stmt) immediately after prepare(), and if prepare failed log the error (include $this->db->error or similar) and skip/return from the bundled-plugin registration path instead of calling bind_param() and continuing; ensure the same error-handling pattern used in the SELECT/UPDATE branches is applied here so failures are logged and do not fatal the process.
988-1009:⚠️ Potential issue | 🟠 MajorGuard
prepare()failure before the unconditionalfinallyclose.If
prepare()returnsfalse,bind_param()throws aTypeError, which the catch handles, but thenfinally { $stmt->close(); }attempts to call a method on booleanfalse, causing a fatal error. Add an upfront check afterprepare(), or use a conditional close.Suggested fix
$stmt = $this->db->prepare(" INSERT INTO plugin_settings (plugin_id, setting_key, setting_value, autoload, created_at) VALUES (?, ?, ?, ?, NOW()) ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), autoload = VALUES(autoload), updated_at = NOW() "); + if ($stmt === false) { + SecureLogger::error('[PluginManager] setSetting failed', [ + 'plugin_id' => $pluginId, + 'key' => $key, + 'error' => $this->db->error, + ]); + return false; + } $valueStr = is_array($value) || is_object($value) ? json_encode($value) : (string)$value; try { $valueStr = $this->encryptPluginSettingValue($valueStr); $stmt->bind_param('issi', $pluginId, $key, $valueStr, $autoloadInt); $result = $stmt->execute(); } catch (\Throwable $e) { SecureLogger::error('[PluginManager] setSetting failed', [ 'plugin_id' => $pluginId, 'key' => $key, 'error' => $e->getMessage(), ]); return false; } finally { - $stmt->close(); + if ($stmt instanceof \mysqli_stmt) { + $stmt->close(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 988 - 1009, The code calls $this->db->prepare(...) and then unconditionally calls $stmt->close() in the finally block, which will fatal if prepare() returned false; modify the flow in the method (e.g., setSetting in PluginManager) to check the prepare result immediately after calling $this->db->prepare and handle a false return (log the prepare error and return false) or change the finally block to conditionally call $stmt->close() only when $stmt is an object/instance of mysqli_stmt (or not === false); ensure references to $stmt, $this->db->prepare, bind_param and encryptPluginSettingValue remain consistent.
♻️ Duplicate comments (25)
app/Support/MediaLabels.php (1)
150-152:⚠️ Potential issue | 🔴 Critical
strip_tags()still leaves attacker-controlled attributes on allowed tags.A scraped/imported
<ol><li onclick=...>survives this branch becausestrip_tags()preserves attributes on allowed tags. That keeps an XSS path in the one code path that skips the safer rebuild-and-escape flow below.♻️ Safer fallback
- if (str_contains($text, '<ol') && str_contains($text, '</ol>')) { - return strip_tags($text, '<ol><li><span><br>'); - } + if (str_contains($text, '<ol') && str_contains($text, '</ol>')) { + $text = trim(preg_replace('/<br\s*\/?>/i', "\n", strip_tags($text)) ?? strip_tags($text)); + }If you need to preserve list formatting here, rebuild a fresh
<ol class="tracklist">from extracted<li>text instead of returning source HTML.In PHP, does strip_tags($html, '<ol><li><span><br>') remove attributes such as onclick, onerror, or style from the allowed tags?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/MediaLabels.php` around lines 150 - 152, The current branch in MediaLabels.php returns user-supplied HTML after strip_tags($text, '<ol><li><span><br>'), which preserves attributes and allows XSS; replace that early-return with logic that parses the input for <li> items (e.g. use preg_match_all or DOMDocument to extract LI contents), escape each item (htmlspecialchars/escapes) and rebuild a clean <ol class="tracklist"> with only safe <li> children (no attributes preserved) and return that sanitized HTML instead of the original strip_tags output.app/Support/Updater.php (1)
2304-2333:⚠️ Potential issue | 🟠 MajorHandle symlinked entries before calling
rmdir().
RecursiveDirectoryIteratorcan still surface directory symlinks, and this branch routes them throughrmdir(). On Unix that fails, which can break bundled-plugin cleanup or rollback if a plugin tree contains links.♻️ Suggested fix
- if (!file_exists($path)) { + if (!file_exists($path) && !is_link($path)) { return; } + if (is_link($path)) { + if (!unlink($path)) { + throw new Exception(sprintf(__('Impossibile rimuovere link simbolico: %s'), $path)); + } + return; + } if (!is_dir($path)) { throw new Exception(sprintf(__('Percorso plugin non valido: %s'), $path)); } @@ foreach ($iterator as $item) { + if ($item->isLink()) { + if (!unlink($item->getPathname())) { + throw new Exception(sprintf(__('Impossibile rimuovere link simbolico: %s'), $item->getPathname())); + } + continue; + } if ($item->isDir()) { if (!rmdir($item->getPathname())) { throw new Exception(sprintf(__('Impossibile rimuovere directory: %s'), $item->getPathname()));In PHP, when using RecursiveDirectoryIterator without FOLLOW_SYMLINKS, can SplFileInfo::isDir() return true for symlinks to directories, and should those links be removed with unlink() instead of rmdir()?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/Updater.php` around lines 2304 - 2333, The removal code in removeDirectoryTree can call rmdir() on directory symlinks because RecursiveDirectoryIterator may report them as directories; update removeDirectoryTree so each iterator item is checked for being a symlink (use $item->isLink() or is_link($item->getPathname())) before treating it as a directory and remove symlinks with unlink() instead of rmdir(); likewise handle the final $path: if is_link($path) unlink($path) else rmdir($path); change the existing $item->isDir() branch to require !$item->isLink() for rmdir and add the unlink() branch for links and throw the same exceptions on failure.app/Views/layout.php (1)
364-365:⚠️ Potential issue | 🟡 MinorReplace hardcoded admin path with a route key.
Line 365 uses a hardcoded
url('/admin/libri/bulk-enrich'); switch toroute_path(...)to keep route indirection/localization consistent.♻️ Proposed fix
- href="<?= htmlspecialchars(url('/admin/libri/bulk-enrich'), ENT_QUOTES, 'UTF-8') ?>"> + href="<?= htmlspecialchars(route_path('admin.libri.bulk-enrich'), ENT_QUOTES, 'UTF-8') ?>">As per coding guidelines, "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/layout.php` around lines 364 - 365, Replace the hardcoded url('/admin/libri/bulk-enrich') call inside the anchor with a route helper call to keep route indirection; update the href to use route_path(...) (or RouteTranslator::route(...)) with the appropriate route key (e.g. 'admin.libri.bulk_enrich' or your app's equivalent) while preserving the surrounding htmlspecialchars(...) call and ENT_QUOTES/'UTF-8' arguments so the anchor markup and escaping remain unchanged.app/Views/frontend/catalog.php (1)
1302-1326:⚠️ Potential issue | 🟡 Minor
tipo_mediafilter is still only partially wired client-side.The new filter can update server results, but client UI state is inconsistent after AJAX updates: no dedicated active-state sync for media options, and active-filter tags still fall back to raw enum values instead of display labels.
Proposed direction
+// PHP (before JS block): expose media value->label map +$tipoMediaLabelsJs = json_encode(array_map( + static fn(array $m) => __($m['label']), + \App\Support\MediaLabels::allTypes() +), JSON_UNESCAPED_UNICODE | JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT); <script> +const tipoMediaLabels = {$tipoMediaLabelsJs}; document.addEventListener('DOMContentLoaded', () => { ... syncAvailabilityActiveState(); + syncTipoMediaActiveState(); }); function updateFilter(key, value) { ... if (key === 'disponibilita') { syncAvailabilityActiveState(); + } else if (key === 'tipo_media') { + syncTipoMediaActiveState(); } ... } +function syncTipoMediaActiveState() { + const currentValue = currentFilters.tipo_media || ''; + document.querySelectorAll('[data-filter-key="tipo_media"]').forEach((option) => { + const targetValue = option.dataset.filterValue || ''; + option.classList.toggle('active', targetValue === currentValue); + }); +} // in updateActiveFiltersDisplay(): - } else if (filterKey === 'genere_id') { + } else if (filterKey === 'genere_id') { displayValue = currentGenreName || value; + } else if (filterKey === 'tipo_media') { + displayValue = tipoMediaLabels[value] || value; }And add attributes on each media filter option anchor:
-<a href="#" class="filter-option <?= $isActive ? 'active' : '' ?>" +<a href="#" class="filter-option <?= $isActive ? 'active' : '' ?>" + data-filter-key="tipo_media" + data-filter-value="<?= htmlspecialchars((string)$tmValue, ENT_QUOTES, 'UTF-8') ?>"Also applies to: 1460-1460, 1622-1622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/catalog.php` around lines 1302 - 1326, The tipo_media filter anchors are not syncing UI state after AJAX updates and active-filter tags show raw enum values; update the filter-option anchors to include a data-label attribute (use the display label from MediaLabels::allTypes()) and ensure updateFilter('tipo_media', ...) sets the selected anchor's class "active" and updates any active-filter tag content to use the anchor's data-label instead of the raw value; locate the anchor generation in the catalog.php media block (uses $tipoFilters and the class "filter-option") and the client-side updateFilter handler to apply the DOM changes so the UI stays consistent after AJAX responses.app/Views/frontend/catalog-grid.php (1)
41-45:⚠️ Potential issue | 🟡 MinorNormalize
tipo_mediabefore deciding badge visibility and icon/label.Using raw
tipo_mediahere can misclassify aliases (non-librobadge shown when normalized value islibro, or wrong icon/label). Use normalized media type for the condition and helper calls.Proposed fix
- <?php if (($book['tipo_media'] ?? 'libro') !== 'libro'): ?> - <span class="book-media-badge" title="<?= htmlspecialchars(\App\Support\MediaLabels::tipoMediaDisplayName($book['tipo_media']), ENT_QUOTES, 'UTF-8') ?>" aria-label="<?= htmlspecialchars(\App\Support\MediaLabels::tipoMediaDisplayName($book['tipo_media']), ENT_QUOTES, 'UTF-8') ?>"> - <i class="fas <?= htmlspecialchars(\App\Support\MediaLabels::icon($book['tipo_media']), ENT_QUOTES, 'UTF-8') ?>" aria-hidden="true"></i> + <?php $normalizedTipoMedia = \App\Support\MediaLabels::normalizeTipoMedia($book['tipo_media'] ?? null) ?? 'libro'; ?> + <?php if ($normalizedTipoMedia !== 'libro'): ?> + <span class="book-media-badge" title="<?= htmlspecialchars(\App\Support\MediaLabels::tipoMediaDisplayName($normalizedTipoMedia), ENT_QUOTES, 'UTF-8') ?>" aria-label="<?= htmlspecialchars(\App\Support\MediaLabels::tipoMediaDisplayName($normalizedTipoMedia), ENT_QUOTES, 'UTF-8') ?>"> + <i class="fas <?= htmlspecialchars(\App\Support\MediaLabels::icon($normalizedTipoMedia), ENT_QUOTES, 'UTF-8') ?>" aria-hidden="true"></i> </span> <?php endif; ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/catalog-grid.php` around lines 41 - 45, Normalize $book['tipo_media'] once before the conditional and use that normalized value for the badge visibility and for calls to \App\Support\MediaLabels::tipoMediaDisplayName and ::icon; e.g., compute $tipo_media_normalized = \App\Support\MediaLabels::normalize($book['tipo_media'] ?? 'libro') (or the appropriate normalizer method on MediaLabels), then change the if to check ($tipo_media_normalized !== 'libro') and pass $tipo_media_normalized to tipoMediaDisplayName and icon so aliases are handled consistently.app/Controllers/FrontendController.php (1)
904-908:⚠️ Potential issue | 🟡 Minor
tipo_mediacan still fail silently on pre-migration schemas.When the column is missing, the filter is ignored without surfacing that state to UI/API, so users can keep an active
tipo_mediafilter that is not applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 904 - 908, The code silently ignores a requested tipo_media filter when the database schema lacks the column; modify the request validation in FrontendController (the block that checks $filters['tipo_media'] and calls hasLibriColumn) to detect the missing column and surface that to the client instead of silently skipping it: either return a validation error/400 indicating tipo_media is unsupported, or remove the filter and add a warning flag in the response payload; specifically update the check around hasLibriColumn($db, 'tipo_media') so that when it returns false you set an explicit error/warning (using the same request/response flow) rather than just omitting the condition that populates $conditions, $params and $types for tipo_media.app/Views/libri/partials/book_form.php (1)
3615-3624:⚠️ Potential issue | 🟡 MinorNormalize/whitelist imported
tipo_mediabefore selecting option.Raw assignment can drift when sources send labels/aliases (e.g., uppercase or non-key values), so imported media type may not map to rendered keys.
🛡️ Proposed guard
// Auto-set tipo_media from scraped data try { if (data.tipo_media) { const tipoMediaSelect = document.getElementById('tipo_media'); - if (tipoMediaSelect) { - tipoMediaSelect.value = data.tipo_media; + const allowedTipoMedia = new Set(<?= json_encode(array_keys(\App\Support\MediaLabels::allTypes()), JSON_HEX_TAG) ?>); + const normalizedTipoMedia = String(data.tipo_media).trim().toLowerCase(); + if (tipoMediaSelect && allowedTipoMedia.has(normalizedTipoMedia)) { + tipoMediaSelect.value = normalizedTipoMedia; } } } catch (err) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 3615 - 3624, The code directly assigns data.tipo_media to the select element value, which can fail if incoming labels/aliases differ; instead implement a normalization/whitelist step: create a small mapping (e.g., { "book":"libro", "BOOK":"libro", "libro":"libro", "audio":"audiobook", ... }) or normalize (toLowerCase/trim) and map data.tipo_media to a canonical option value, then verify that the canonical value exists in the select's options (use tipoMediaSelect.options to check values or text) before setting tipoMediaSelect.value; if no matching canonical value is found, do nothing (or set a safe default) to avoid selecting an invalid option.scripts/bulk-enrich-cron.php (2)
69-83:⚠️ Potential issue | 🟠 MajorAcquire the cron lock atomically.
file_exists()+unlink()+file_put_contents()still leaves a race window where two schedulers can both start a batch. Hold anflock(LOCK_EX | LOCK_NB)on an open handle for the whole run instead of using a sentinel file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bulk-enrich-cron.php` around lines 69 - 83, Replace the fragile sentinel-file logic around $lockFile with an exclusive non-blocking file lock held for the whole run: open the lock file handle (e.g., fopen($lockFile, 'c')), attempt flock($handle, LOCK_EX | LOCK_NB) and if it fails log via logMessage and exit; on success write the PID to the open handle (ftruncate/fwrite) and keep $handle open for the duration of the script, removing any unlink() call for normal flows; at shutdown release the lock with flock($handle, LOCK_UN) and fclose($handle) (use same symbols $lockFile, logMessage and ensure $db->close() still runs before exit).
120-130:⚠️ Potential issue | 🟠 MajorReturn a non-zero exit code on fatal failures.
The catch logs the exception, but the script still falls through to
exit(0), so cron and monitoring will record a broken run as success.Possible fix
+$exitCode = 0; + try { logMessage('Starting bulk enrichment...', $logFile); @@ } catch (\Throwable $e) { logMessage('FATAL ERROR: ' . $e->getMessage(), $logFile); + $exitCode = 1; } finally { // Release lock if (file_exists($lockFile)) { unlink($lockFile); } $db->close(); } -exit(0); +exit($exitCode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bulk-enrich-cron.php` around lines 120 - 130, The catch block logs fatal exceptions but the script unconditionally calls exit(0), so failures are reported as success; modify the control flow around catch (\Throwable $e) / logMessage(...) so a non-zero exit code is used when an exception occurs (e.g., introduce an $exitCode = 0 default, set $exitCode = 1 inside the catch that handles the Throwable, then call exit($exitCode) instead of exit(0)), keeping the existing finally behavior that unlinks $lockFile and calls $db->close().app/Views/libri/index.php (1)
887-888:⚠️ Potential issue | 🟡 MinorRender the selected media filter with its localized label.
This chip still shows the raw enum key (
disco,audiolibro, …), while the dropdown and table renderer already use display labels. Reuse the same label map here so the active filter stays translated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 887 - 888, The chip is pushing the raw enum key from document.getElementById('tipo_media_filter').value (tipoMedia) into filters; instead map tipoMedia to its localized display label using the same label map used by the dropdown/table renderer (the media-type label map used elsewhere in the code) before pushing: look up the label map the dropdown/table uses, derive label = labelMap[tipoMedia] || tipoMedia, and then push { key: 'tipo_media_filter', label: `${LocalizedKey}: ${label}`, icon: 'fa-compact-disc' } into filters so the active filter shows the translated label.app/Controllers/CsvImportController.php (1)
1256-1280:⚠️ Potential issue | 🟠 MajorPreserve existing
formatowhen CSV updates omit media columns.
parseCsvRow()now leaves both fieldsnullwhen the CSV omits them, butupdateBook()still converts that case into'cartaceo'and writes it unconditionally. Any matched CD/vinyl row updated from a generic CSV will be downgraded to book format. UseCOALESCE(?, formato)here and keep the'cartaceo'fallback only ininsertBook().Suggested fix
- formato = ?{$tipoMediaSet}, + formato = COALESCE(?, formato){$tipoMediaSet}, @@ - $formato = !empty($data['formato']) ? $data['formato'] : (empty($tipoMedia) || $tipoMedia === 'libro' ? 'cartaceo' : null); + $formato = !empty($data['formato']) ? $data['formato'] : null;Also applies to: 1292-1313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CsvImportController.php` around lines 1256 - 1280, The UPDATE in updateBook() currently forces formato to a concrete value even when parseCsvRow() yields NULL; change the SQL for the formato column to use COALESCE(?, formato) so an omitted CSV value preserves the existing formato (i.e., replace "formato = ?{$tipoMediaSet}" with "formato = COALESCE(?, formato){$tipoMediaSet}") and ensure the bound parameter position still matches; leave the 'cartaceo' default only in insertBook() where new rows need a default.app/Controllers/LibraryThingImportController.php (1)
1311-1328:⚠️ Potential issue | 🟠 MajorDon’t force
formato = 'cartaceo'when an update omits media data.If a LibraryThing row has no
Media,parseLibraryThingRow()leaves bothformatoandtipo_mediaempty, but both update paths still bind'cartaceo'and write it unconditionally. That can silently downgrade matched music/video records back to book format. Preserve the current DB value on update and keep the fallback only in insert paths.Suggested fix
- genere_id = ?, descrizione = ?{$descPlainSet}, formato = ?{$tipoMediaSet}, prezzo = ?, editore_id = ?, + genere_id = ?, descrizione = ?{$descPlainSet}, formato = COALESCE(?, formato){$tipoMediaSet}, prezzo = ?, editore_id = ?, @@ - $params[] = !empty($data['formato']) ? $data['formato'] : (empty($tipoMedia) || $tipoMedia === 'libro' ? 'cartaceo' : null); + $params[] = !empty($data['formato']) ? $data['formato'] : null;Also applies to: 1348-1352, 1396-1404, 1424-1428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1311 - 1328, The update SQLs (prepared statements that set formato and tipo_media) are overwriting existing media type with the hardcoded 'cartaceo' when parseLibraryThingRow() returns empty media; change the update logic to preserve the DB value by only binding formato and tipo_media when parseLibraryThingRow() provides them (or bind NULL and remove the assignment from the UPDATE), and keep the 'cartaceo' fallback strictly in INSERT paths; locate the affected prepared statements and the variables formato / tipo_media (and parseLibraryThingRow) and modify the UPDATE branches so they do not unconditionally set formato='cartaceo' or force tipo_media, ensuring updates omit those columns unless new media data exists.app/Controllers/BulkEnrichController.php (1)
55-62:⚠️ Potential issue | 🟠 MajorReturn a generic 500 here instead of exposing
$e->getMessage().This leaks internal failure details to the client and still returns
200 OK, so callers cannot reliably distinguish a failed batch from a successful one.Suggested fix
+use App\Utils\SecureLogger; @@ } catch (\Throwable $e) { + SecureLogger::error('[BulkEnrichController] Manual batch failed', [ + 'exception' => $e->getMessage(), + ]); $response->getBody()->write(json_encode([ 'success' => false, - 'error' => $e->getMessage(), + 'error' => __('Errore interno.'), ], JSON_UNESCAPED_UNICODE | JSON_HEX_TAG)); + return $response + ->withHeader('Content-Type', 'application/json') + ->withStatus(500); } return $response->withHeader('Content-Type', 'application/json');Based on learnings,
Use SecureLogger::error() not error_log() for sensitive contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/BulkEnrichController.php` around lines 55 - 62, Replace the current catch block in BulkEnrichController that writes $e->getMessage() to the response body and returns 200 with a safer flow: log the full throwable internally using SecureLogger::error($e) (or SecureLogger::error($e->getMessage(), ['exception' => $e]) if signature requires context), then write a generic JSON error payload (e.g., ['success' => false, 'error' => 'Internal server error']) using the existing response->getBody()->write call, and return the response with HTTP status 500 via response->withStatus(500)->withHeader('Content-Type', 'application/json'); remove any exposure of $e->getMessage() to clients.app/Views/frontend/book-detail.php (1)
2255-2255:⚠️ Potential issue | 🟡 MinorTreat blank
autorias missing on related cards.
??only falls back onnull. Ifautoriis''or whitespace, this still renders an empty author line instead of the music-aware fallback.Suggested fix
- <?= htmlspecialchars($related['autori'] ?? __($relatedIsMusic ? 'Artista sconosciuto' : 'Autore sconosciuto'), ENT_QUOTES, 'UTF-8') ?> + <?php $relatedAuthorsLabel = trim((string) ($related['autori'] ?? '')); ?> + <?= htmlspecialchars( + $relatedAuthorsLabel !== '' ? $relatedAuthorsLabel : __($relatedIsMusic ? 'Artista sconosciuto' : 'Autore sconosciuto'), + ENT_QUOTES, + 'UTF-8' + ) ?>🤖 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 2255, The code uses the null-coalescing operator on $related['autori'] so empty strings or whitespace still render blank; update the rendering to treat blank/whitespace as missing by trimming and checking emptiness before falling back to the music-aware label: compute a $author value like trim($related['autori'] ?? '') and if that is === '' use __($relatedIsMusic ? 'Artista sconosciuto' : 'Autore sconosciuto'), then pass that result to htmlspecialchars for output (reference $related['autori'] and $relatedIsMusic in the change).tests/discogs-advanced.spec.js (1)
125-150:⚠️ Potential issue | 🟡 MinorAssert the control record's
BookJSON-LD too.The test title promises both sides, but it only checks
MusicAlbumon the music page. A regression that makes normal books emit the wrong schema would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-advanced.spec.js` around lines 125 - 150, The test only validates a MusicAlbum schema for the disco page but the title promises validating both sides; add a parallel assertion that the control "book" page emits a Book JSON-LD: fetch the book page (similar to musicResp -> page.request.get with bookBookId), extract jsonLdBlocks and schemas the same way, then find a schema with ['@type'] === 'Book' (or appropriate Book schema) and assert it is truthy; also keep the existing DB checks (bookTipoMedia) to ensure you're validating the correct record. Ensure you reuse the same parsing logic (jsonLdBlocks, schemas) and error message style when asserting the Book schema.storage/plugins/discogs/plugin.json (1)
10-12:⚠️ Potential issue | 🟠 MajorDon't advertise app-version compatibility gates unless they are enforced.
If install/activation still ignores
requires_appandmax_app_version, this manifest gives users a false compatibility guarantee. Either wire those checks into the plugin loader or drop the fields for now.Run this to confirm whether the plugin loader actually validates these manifest keys. Expected result: there should be a code path that reads
requires_app/max_app_versionand rejects incompatible versions during install or activation.#!/bin/bash set -euo pipefail echo "== Compatibility-key references ==" rg -n -C2 "requires_php|requires_app|max_app_version|version_compare\(" --glob '*.php' echo echo "== Likely plugin-manager implementations ==" fd -i 'PluginManager.php' . -x sh -c ' echo "--- $1 ---" rg -n -C3 "requires_php|requires_app|max_app_version|version_compare|activate|install|manifest|plugin.json" "$1" ' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/plugin.json` around lines 10 - 12, The manifest advertises requires_php/requires_app/max_app_version but the plugin loader doesn't enforce them; either remove those fields from storage/plugins/discogs/plugin.json or add validation in the plugin manager: update the code paths that parse the plugin manifest (e.g. manifest parsing used by PluginManager::install and PluginManager::activate) to read requires_php/requires_app/max_app_version and use PHP_VERSION and version_compare() (and the runtime app version constant) to reject installs/activations that are incompatible, logging a clear error and failing the operation.app/Routes/web.php (1)
1374-1378:⚠️ Potential issue | 🟠 MajorAdd back-pressure to the manual bulk-enrich start endpoint.
This can still launch overlapping enrichment runs on double-submit/retry, unlike
/admin/libri/sync-coversright above it. That risks duplicated external calls and conflicting progress state.Minimal route-level mitigation
$app->post('/admin/libri/bulk-enrich/start', function ($request, $response) use ($app) { $db = $app->getContainer()->get('db'); $controller = new \App\Controllers\BulkEnrichController(); return $controller->start($request, $response, $db); - })->add(new CsrfMiddleware())->add(new AdminAuthMiddleware()); + })->add(new \App\Middleware\RateLimitMiddleware(1, 120)) + ->add(new CsrfMiddleware()) + ->add(new AdminAuthMiddleware());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Routes/web.php` around lines 1374 - 1378, The bulk-enrich start route can launch overlapping runs; fix by adding a route-level back-pressure middleware that prevents concurrent runs: implement or reuse a lock middleware (e.g., EnrichmentLockMiddleware or the same lock used by the /admin/libri/sync-covers route) and attach it to the POST '/admin/libri/bulk-enrich/start' route (which wraps BulkEnrichController::start) before CsrfMiddleware/AdminAuthMiddleware, making the middleware try to acquire a non-blocking lock keyed for "bulk-enrich", return a 409/appropriate error if the lock exists, and ensure the lock is released when the controller finishes or on error.app/Views/admin/bulk-enrich.php (2)
141-149:⚠️ Potential issue | 🟡 MinorGenerate these endpoints from route helpers.
Hardcoding
/admin/libri/bulk-enrich/...inside a PHP view makes this page drift when routing or base-path handling changes. Build the URLs in PHP and pass them into JS viajson_encode(..., JSON_HEX_TAG).As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login" and "Usejson_encode(..., JSON_HEX_TAG)for any PHP→JS output in views".Also applies to: 197-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/bulk-enrich.php` around lines 141 - 149, Replace the hardcoded URL strings used in the fetch calls (e.g., the '/admin/libri/bulk-enrich/toggle' used inside the fetch in bulk-enrich.php) with PHP-generated route helpers and pass them into the JS scope via json_encode(..., JSON_HEX_TAG); specifically, create PHP variables using route_path('your_route_key') or RouteTranslator::route('your_route_key') for each endpoint (toggle and the other endpoints around lines 197-205), echo them into a safe JS object with json_encode(..., JSON_HEX_TAG), and update the fetch calls to use those JS variables instead of hardcoded paths.
85-87:⚠️ Potential issue | 🟠 MajorGive the switch a programmatic name.
role="switch"plusaria-checkedis good, but this control still has no accessible name. Wire it to#toggle-labelwitharia-labelledbyor add an explicitaria-label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/bulk-enrich.php` around lines 85 - 87, The toggle button with id "toggle-enrichment" lacks an accessible name; update the element to include either aria-labelledby="toggle-label" (and ensure an element exists with id "toggle-label" containing the visible label) or add an explicit aria-label (e.g., aria-label="Enable enrichment") so screen readers can announce its purpose; modify the <button id="toggle-enrichment"> attributes accordingly and verify aria-checked remains present for state.tests/discogs-import.spec.js (2)
75-81:⚠️ Potential issue | 🟠 MajorDon't hard-delete imported
librirows in teardown.Saving through the admin form can create dependent rows. This raw
DELETE FROM librican fail on FK constraints, and the emptycatchhides that cleanup never happened. Prefer the app's delete/soft-delete path, or delete dependents first and surface cleanup failures.Possible fix
test.afterAll(async () => { // Cleanup test data try { if (createdId !== '') { - dbExec(`DELETE FROM libri WHERE id = ${Number(createdId)} AND deleted_at IS NULL`); + dbExec(`DELETE FROM copie WHERE libro_id = ${Number(createdId)}`); + dbExec( + `UPDATE libri + SET deleted_at = NOW(), ean = NULL, isbn13 = NULL, isbn10 = NULL + WHERE id = ${Number(createdId)} AND deleted_at IS NULL` + ); } - } catch {} + } catch (err) { + throw new Error(`Discogs cleanup failed for libri.id=${createdId}: ${err.message}`); + } await context?.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 75 - 81, The teardown uses a raw DELETE via dbExec in test.afterAll which can violate FK constraints and swallows errors; instead call the application's delete/soft-delete flow or remove dependent rows before deleting and propagate failures. Replace the dbExec(`DELETE FROM libri ...`) call (referenced by createdId and dbExec in test.afterAll) with an awaited call to the app's delete endpoint or service method that performs soft-delete, or run explicit deletions of dependent tables first, and remove the empty catch so any exception surfaces (or rethrow/log it) so cleanup failures are visible.
64-71:⚠️ Potential issue | 🟠 MajorReplace the real Discogs barcode before submit.
This suite still persists
0720642442524intoean, and setup now soft-deletes any active row already using that public identifier. That makes the test mutate unrelated fixtures in a shared E2E database instead of staying self-contained.Also applies to: 145-151, 154-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 64 - 71, The test is mutating a real public barcode (TEST_BARCODE = "0720642442524") and soft-deleting rows in the shared libri table; update the test to use a generated or clearly namespaced test-only barcode/identifier and avoid touching real fixtures: replace occurrences of TEST_BARCODE (and the raw literal) with a test-specific constant (e.g., TEST_ONLY_BARCODE or a random/test UUID-derived value) and ensure dbExec calls that update libri (the UPDATE SQL in the test and similar blocks around lines referenced) use that test-only value (prefer parameterized binding if your dbExec supports it) so the setup only affects records created by this test and not shared E2E fixtures.tests/bulk-enrich.spec.js (1)
164-171:⚠️ Potential issue | 🟠 MajorMirror the service's pending predicate in these DB assertions.
All of these counts still require
isbn13, so EAN-only and ISBN10-only rows are excluded even though the enrichment feature processes them. The page and the test will diverge as soon as non-ISBN13 identifiers exist.Also applies to: 186-190, 202-206, 230-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bulk-enrich.spec.js` around lines 164 - 171, The DB assertions currently require isbn13 which excludes ISBN10-only and EAN-only rows; update the WHERE identifier predicate to match the service's pending predicate by checking for any identifier present (e.g., (isbn13 IS NOT NULL AND isbn13 != '') OR (isbn10 IS NOT NULL AND isbn10 != '') OR (ean IS NOT NULL AND ean != '')) instead of only isbn13, and apply this change to the dbQuery calls that compute pendingCount and the other similar counts (references: pendingCount variable and the dbQuery calls around it).storage/plugins/discogs/DiscogsPlugin.php (1)
55-63:⚠️ Potential issue | 🟡 MinorValidate UPC/EAN checksums here, not just length.
Any 12/13-digit garbage currently passes this hook, which burns Discogs/MusicBrainz quota and can misclassify junk input as a valid music barcode. Compute the UPC-A / EAN-13 check digit before returning
true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/DiscogsPlugin.php` around lines 55 - 63, The validateBarcode(bool $isValid, string $isbn) currently only strips non-digits and checks length; instead compute and verify the UPC-A (12-digit) and EAN-13 (13-digit) checksum before returning true: keep the early return if $isValid, otherwise clean digits into $clean and if length==12 compute the UPC-A check digit from the first 11 digits and compare to digit 12, if length==13 compute the EAN-13 check from the first 12 digits and compare to digit 13; only return true when the calculated check equals the provided check digit, otherwise return false.tests/pr100-review-fixes.spec.js (1)
313-323:⚠️ Potential issue | 🟡 MinorExercise the full soft-delete contract here.
This test seeds
isbn10andisbn13asNULLand then manually clears onlyean, so a regression that stops nullifying the other unique columns would still pass. Seed all three identifiers and assert all three are cleared after the real soft-delete path.Based on learnings "Nullify unique-indexed columns (isbn10, isbn13, ean) on soft-delete of
libritable records".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pr100-review-fixes.spec.js` around lines 313 - 323, Update the test to seed all three unique barcode columns (isbn10, isbn13, ean) for the record identified by bookId (use dbExec to set isbn10 = '${TEST_BARCODE}', isbn13 = '${TEST_BARCODE}', ean = '${TEST_BARCODE}'), then invoke the actual soft-delete code path under test (instead of only manually clearing ean) so the application logic nullifies all fields; after soft-delete, query with dbQuery to assert isbn10, isbn13 and ean are all NULL (e.g. COUNT for non-deleted rows by each column or explicit SELECT to confirm NULL), and finally restore all three columns in the cleanup using dbExec to set isbn10, isbn13 and ean back to '${TEST_BARCODE}' for bookId.tests/discogs-plugin.spec.js (1)
239-244:⚠️ Potential issue | 🟡 MinorConsole listener registered after the action and
logsnever asserted.The
elsebranch attachespage.on('console', ...)only afterpage.click()/waitForTimeouthave already completed, so any errors fired during the scrape are missed.logsis also populated but never asserted, so this branch provides no real coverage and will always pass. Move the listener registration beforeimportBtn.click()and addexpect(logs).toEqual([])(or an allow-list filter) in the empty-title branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 239 - 244, The console listener is being attached after the scrape actions so errors during the scrape are missed and logs is never asserted; move the page.on('console', ...) registration (the logs array and page.on call) to before importBtn.click()/await page.waitForTimeout(...) so it captures errors during the action, and in the empty-title branch add an assertion like expect(logs).toEqual([]) (or an allow-list filter) to fail the test on unexpected console errors; reference the page, importBtn, logs, and the console listener registration when making the change.
🧹 Nitpick comments (8)
.coderabbit.yaml (2)
131-141: Enabled tools match the schema; note these are all already on by default.
phpstan,shellcheck,semgrep,gitleaks, andyamllinteach default toenabled: truein the schema, so this block is effectively a no-op re-declaration. Keeping it as explicit documentation is fine, but if the intent was to change behavior (e.g., setphpstan.level), that's not happening here.Optional: set an explicit PHPStan level to match the PR's "PHPStan level 1 passes" test-plan acceptance criterion
tools: phpstan: enabled: true + level: "1" shellcheck: enabled: trueNote: this is ignored if
phpstan.neon/phpstan.neon.distalready setslevel:.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 131 - 141, The tools block explicitly re-declares defaults (tools.phpstan.enabled, tools.shellcheck.enabled, tools.semgrep.enabled, tools.gitleaks.enabled, tools.yamllint.enabled) which is a no-op; either remove the entire tools section to rely on schema defaults, or if you intended to change behavior add the actual configuration (e.g., set tools.phpstan.level: 1 under tools.phpstan) so the file meaningfully alters defaults instead of redundantly re-asserting them.
37-123: Path-specific instructions are well-scoped and within schema limits.Each entry uses the required
{path, instructions}shape, globs target directories that actually exist per the PR (app/Controllers/**,app/Models/**,app/Views/**,app/Support/**,storage/plugins/**,installer/database/migrations/**,locale/**,tests/**,scripts/**), and all instruction blocks are comfortably under the 20,000-char limit. The guidance also aligns with concrete fixes surfaced in this PR (soft-deletedeleted_at IS NULL,\Throwablecapture, SSRF/cURL hardening in plugins, elapsed-based rate limiting, migration version gating).One nit: consider whether
path: "tests/**"is intended to also coverplaywright.config.*and any top-level E2E helpers — those files live outsidetests/**in some PR diffs and wouldn't inherit these instructions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 37 - 123, The path rule "path: \"tests/**\"" may miss top-level E2E config/helpers (e.g. playwright.config.* and any top-level E2E helper files) that should follow the same E2E requirements; update the YAML by adding an additional path entry (or expanding the glob) targeting those top-level files—e.g. add a new rule for "path: \"playwright.config.*\"" or change to "path: \"{tests/**,playwright.config.*,e2e_helpers/**}\""—and ensure it contains the same E2E instructions (workers=1, /tmp/run-e2e.sh, SweetAlert handling, Flatpickr JS interaction, test data cleanup) so the top-level configs inherit the same constraints as tests/**.tests/smoke-install.spec.js (1)
292-318: Verification logic is correct but asserts a non-RUN_ID substring.The search is scoped by
RUN_ID(line 310), which correctly isolates this run, but the assertionb.titolo.includes('Edizione Rivista')(line 315) uses a hardcoded literal. If the update failed and the pre-update record — which also containsRUN_ID— is returned, thefindwill returnundefinedand the test will correctly fail. So the logic is sound.Consider tightening to
b.titolo === BOOK_TITLE_UPDATEDfor a stricter match, which guards against partial updates where only the suffix was appended.Proposed tighter assertion
const match = listData.data.find( - (b) => typeof b.titolo === 'string' && b.titolo.includes('Edizione Rivista') + (b) => typeof b.titolo === 'string' && b.titolo === BOOK_TITLE_UPDATED ); - expect(match, 'Updated title must contain Edizione Rivista').toBeTruthy(); + expect(match, `Updated title must equal "${BOOK_TITLE_UPDATED}"`).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke-install.spec.js` around lines 292 - 318, The test "Edit book: change title" currently finds a match using a hardcoded substring ('Edizione Rivista'); change the find predicate to require an exact match against the updated constant by using b.titolo === BOOK_TITLE_UPDATED (reference BOOK_TITLE_UPDATED, RUN_ID, listData, and the local variable match) and update the expect assertion message accordingly so the test strictly verifies the title was set to the new value rather than just containing a substring.locale/it_IT.json (1)
35-36: Localization additions look consistent.New Discogs token and media-type strings are correctly added and the JSON remains valid. Note: legacy key
"Impossibile scaricare (libro senza ISBN):"(line 109) is still present alongside the new"Impossibile scaricare (senza ISBN/barcode):"(line 150); if the old key is no longer emitted by any view/controller, consider removing it in a follow-up cleanup to avoid dead translation strings.Also applies to: 141-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/it_IT.json` around lines 35 - 36, The translations file contains a legacy key "Impossibile scaricare (libro senza ISBN):" alongside the newer "Impossibile scaricare (senza ISBN/barcode):"; verify whether the old key is still emitted by any view/controller and if not, remove the legacy entry from locale/it_IT.json to avoid dead translations, ensuring you only delete "Impossibile scaricare (libro senza ISBN):" (and any exact-duplicate legacy variants) and keep the new "Impossibile scaricare (senza ISBN/barcode):" intact; if you discover code still references the old key, update those references to the new key instead.app/Support/BundledPlugins.php (1)
8-16: Optional: consider a more descriptive constant name.
LISTis a legal class-constant name in PHP 7+, but it shadows thelist()language construct visually. A name likeSLUGSorNAMESwould read a bit clearer at call sites (BundledPlugins::SLUGS). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/BundledPlugins.php` around lines 8 - 16, The class constant BundledPlugins::LIST visually conflicts with the PHP list() construct; rename the constant (e.g., to SLUGS or NAMES) and update all references to it throughout the codebase (replace BundledPlugins::LIST with BundledPlugins::SLUGS or chosen name) so callers and documentation use the new identifier; ensure the array contents and visibility remain unchanged and run tests/grep to catch any remaining usages.app/Controllers/FrontendController.php (1)
921-924: Free schema-check result to avoid leakingmysqli_result.Cache is good, but the
SHOW COLUMNSresult should be released like indescriptionExpr().♻️ Proposed fix
if (!array_key_exists($column, $columnCache)) { $result = $db->query("SHOW COLUMNS FROM libri LIKE '" . $db->real_escape_string($column) . "'"); - $columnCache[$column] = $result !== false && $result->num_rows > 0; + $columnCache[$column] = $result !== false && $result->num_rows > 0; + if ($result instanceof \mysqli_result) { + $result->free(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 921 - 924, The SHOW COLUMNS query stores a mysqli_result in $result and isn't freed, which can leak resources; modify the block in FrontendController (the code using $column, $columnCache and $db->query("SHOW COLUMNS ...")) to check $result !== false, set $columnCache[$column] = ($result->num_rows > 0), then call $result->free() (or $result->close()) before leaving the block so the mysqli_result is released (mirroring descriptionExpr()).app/Services/BulkEnrichmentService.php (1)
417-425: Leakedmysqli_resultwhensystem_settingsis absent.When
num_rows === 0, the method returns without calling$tableCheck->free(). Move thefree()to run in both branches (or use atry/finally). Tiny, but hit on everyisEnabled()call on installs that don't yet have thesystem_settingstable.♻️ Suggested fix
- $tableCheck = $this->db->query("SHOW TABLES LIKE 'system_settings'"); - if ($tableCheck === false || $tableCheck->num_rows === 0) { - return false; - } - if ($tableCheck instanceof \mysqli_result) { - $tableCheck->free(); - } + $tableCheck = $this->db->query("SHOW TABLES LIKE 'system_settings'"); + if ($tableCheck === false) { + return false; + } + $hasTable = $tableCheck->num_rows > 0; + $tableCheck->free(); + if (!$hasTable) { + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/BulkEnrichmentService.php` around lines 417 - 425, The code leaks a mysqli_result when system_settings is absent because the early return happens before calling $tableCheck->free(); modify isEnabled() so that after calling $this->db->query(...) and assigning $tableCheck you always free the result (or use a try/finally) regardless of whether $tableCheck->num_rows === 0; specifically ensure that the $tableCheck instanceof \mysqli_result check and $tableCheck->free() execute before any return in the isEnabled() flow (or wrap the query and checks in a try/finally that calls free() in the finally block).tests/discogs-plugin.spec.js (1)
156-174: Test 5 does not actually discriminate between music and standard labels.The assertion only checks that
Editore/Publisherappears somewhere on the admin book detail page. Sinceeditore_idis a column on everylibrirow (regardless offormato), this string is likely to appear for music formats too, so the test will pass even ifMediaLabelsregresses and incorrectly showsEditoreforcd_audio. Consider also asserting absence of music-specific labels (e.g.Etichetta,Anno di Uscita) to make the test meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 156 - 174, The test "5. MediaLabels: regular book keeps standard labels" currently only asserts presence of 'Editore'/'Publisher' via the content variable and hasEditore, which doesn't rule out music-specific labels appearing; update the test (in the same test block) to also assert that music-only strings are absent for a non-music format (formato 'cartaceo'): check that content does NOT include 'Etichetta' and 'Anno di Uscita' (and their English equivalents like 'Label'/'Release Year') by adding negative assertions (e.g., expect(content.includes('Etichetta')).toBe(false) and expect(content.includes('Anno di Uscita')).toBe(false)) alongside the existing hasEditore check so the test fails if MediaLabels incorrectly shows music labels for books.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3336d368-276d-4f88-87bd-4b306f7767e0
📒 Files selected for processing (58)
.coderabbit.yaml.gitattributes.gitignoreapp/Controllers/BulkEnrichController.phpapp/Controllers/CsvImportController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/PublicApiController.phpapp/Controllers/ScrapeController.phpapp/Models/BookRepository.phpapp/Routes/web.phpapp/Services/BulkEnrichmentService.phpapp/Support/BundledPlugins.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Support/RouteTranslator.phpapp/Support/Updater.phpapp/Views/admin/bulk-enrich.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog-grid.phpapp/Views/frontend/catalog.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpdata/dewey/dewey_completo_it.jsoninstaller/classes/Installer.phpinstaller/database/migrations/migrate_0.5.4.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonscripts/bulk-enrich-cron.phpstorage/plugins/discogs/DiscogsPlugin.phpstorage/plugins/discogs/README.mdstorage/plugins/discogs/plugin.jsonstorage/plugins/discogs/views/settings.phpstorage/plugins/discogs/wrapper.phptests/bulk-enrich.spec.jstests/discogs-advanced.spec.jstests/discogs-import.spec.jstests/discogs-plugin.spec.jstests/full-test.spec.jstests/multisource-scraping.spec.jstests/playwright.config.jstests/pr100-media-types.spec.jstests/pr100-review-fixes.spec.jstests/seed-catalog.spec.jstests/smoke-install.spec.jsvendor/composer/autoload_classmap.phpvendor/composer/autoload_static.phpvendor/composer/installed.jsonvendor/composer/installed.phpversion.json
💤 Files with no reviewable changes (2)
- vendor/composer/autoload_static.php
- vendor/composer/autoload_classmap.php
Comment on lines
+29
to
+31
| - "!*.min.js" | ||
| - "!*.min.css" | ||
| - "!*.map" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CodeRabbit path_filters glob syntax recursive ** vs * matching
💡 Result:
In CodeRabbit's path_filters, glob patterns follow the minimatch library syntax. The '' pattern enables recursive directory matching (zero or more directories/subdirectories), while '' matches zero or more characters within a single path segment (non-recursive, does not cross directory boundaries). Key differences and examples from CodeRabbit docs: - '!dist/': Excludes the 'dist' directory and all files/subdirectories recursively within it. - '!/dist/': Excludes any 'dist' directory (at any level) and its recursive contents. - '!/node_modules/': Excludes any 'node_modules' directory anywhere and recursively. - '!/.min.js': Excludes all minified JS files anywhere in the repo (recursive due to leading '/', combined with file pattern). - 'src/': Includes 'src' directory and all subdirectories/files recursively. In contrast: - '*' alone would match any file or directory name in the current level only (e.g., equivalent to '' with noglobstar option enabled). - Use '/' to separate path segments; patterns are evaluated relative to the repository root. This syntax keeps reviews focused by excluding build artifacts, dependencies, etc. Patterns with '!' prefix exclude; without include/override exclusions. Defaults include many recursive excludes like '!/dist/'. For full details, see CodeRabbit's reference to minimatch documentation.
Citations:
- 1: https://coderabbit.mintlify.app/configuration/path-instructions
- 2: https://docs.coderabbit.ai/configuration/path-instructions
- 3: https://isaacs.github.io/minimatch/
- 4: https://github.com/isaacs/minimatch/blob/master/README.md
- 5: https://aicodereview.cc/blog/coderabbit-configuration/
🏁 Script executed:
# Check the .coderabbit.yaml file at the specified lines
cat -n .coderabbit.yaml | sed -n '25,35p'Repository: fabiodalez-dev/Pinakes
Length of output: 415
🏁 Script executed:
# Search for minified JS, CSS, and map files in the repository to see if they'd be affected
fd -e min.js -e min.css -e map --type f | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 1069
🏁 Script executed:
# Also check the full path_filters section to understand context
cat -n .coderabbit.yaml | sed -n '20,35p'Repository: fabiodalez-dev/Pinakes
Length of output: 615
Use recursive glob patterns for minified and map file exclusions.
The patterns !*.min.js, !*.min.css, and !*.map match only files at the repository root. Minified assets and source maps exist in nested paths (e.g., public/assets/js/sortable.min.js, public/assets/css/sortable.min.css, public/assets/star-rating/dist/star-rating.min.js), so these exclusions fail to filter them. While specific paths like !public/assets/tinymce/** are excluded, other build artifacts in subdirectories will still be reviewed.
🛠️ Proposed fix
- - "!*.min.js"
- - "!*.min.css"
- - "!*.map"
+ - "!**/*.min.js"
+ - "!**/*.min.css"
+ - "!**/*.map"📝 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
| - "!*.min.js" | |
| - "!*.min.css" | |
| - "!*.map" | |
| - "!vendor/**" | |
| - "!node_modules/**" | |
| - "!public/assets/tinymce/**" | |
| - "!public/assets/fontawesome/**" | |
| - "!public/assets/choices/**" | |
| - "!public/assets/flatpickr/**" | |
| - "!public/assets/sweetalert2/**" | |
| - "!**/*.min.js" | |
| - "!**/*.min.css" | |
| - "!**/*.map" | |
| - "!pinakes-*.zip" | |
| - "!pinakes-*.sha256" | |
| - "!test-results/**" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 29 - 31, The exclusion globs "!*.min.js",
"!*.min.css", and "!*.map" only match files at repo root; update them to
recursive patterns so minified assets and source maps in subdirectories are
excluded—replace those lines with "!**/*.min.js", "!**/*.min.css", and
"!**/*.map" (keeping existing specific exclusions like
"!public/assets/tinymce/**" untouched).
Comment on lines
+230
to
+236
|
|
||
| // Also generate plain-text description | ||
| $plain = strip_tags($scraped['description']); | ||
| $sets[] = 'descrizione_plain = ?'; | ||
| $types .= 's'; | ||
| $values[] = $plain; | ||
| } |
There was a problem hiding this comment.
descrizione_plain retains HTML entities.
strip_tags() removes tags but leaves entities intact, so Discogs/Google‑Books descriptions containing &, ', , etc. will end up literally in descrizione_plain and hurt search/preview quality. Decode entities and collapse whitespace before storing.
♻️ Suggested fix
- // Also generate plain-text description
- $plain = strip_tags($scraped['description']);
+ // Also generate plain-text description
+ $plain = html_entity_decode(
+ strip_tags($scraped['description']),
+ ENT_QUOTES | ENT_HTML5,
+ 'UTF-8'
+ );
+ $plain = trim((string) preg_replace('/\s+/u', ' ', $plain));
$sets[] = 'descrizione_plain = ?';
$types .= 's';
$values[] = $plain;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Services/BulkEnrichmentService.php` around lines 230 - 236, The
descrizione_plain assignment currently uses strip_tags($scraped['description'])
which removes tags but leaves HTML entities and extra whitespace; update the
logic that builds $plain (in BulkEnrichmentService::... where $plain is set from
$scraped['description']) to run html_entity_decode on the stripped text and then
normalize/collapse consecutive whitespace (including non-breaking spaces) into
single spaces and trim the result before pushing it into $values for
descrizione_plain so the stored plain description is decoded and
whitespace-cleaned.
Comment on lines
+249
to
+266
| if ($schemaType === 'MusicAlbum') { | ||
| // MusicAlbum: use byArtist, recordLabel, numTracks | ||
| if (!empty($schemaAuthors)) { | ||
| $bookSchema["byArtist"] = count($schemaAuthors) === 1 ? $schemaAuthors[0] : $schemaAuthors; | ||
| } | ||
| if ($bookPublisher) { | ||
| $bookSchema["recordLabel"] = ["@type" => "Organization", "name" => $bookPublisher]; | ||
| } | ||
| if ($bookPages) { | ||
| $bookSchema["numTracks"] = (int) $bookPages; | ||
| } | ||
| if (!empty($book['ean'])) { | ||
| $bookSchema["identifier"] = [ | ||
| "@type" => "PropertyValue", | ||
| "propertyID" => "EAN", | ||
| "value" => $book['ean'], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Preserve existing barcode data in non-book JSON-LD.
These branches only emit identifier when ean is populated. We already handle music rows whose barcode still lives in isbn13; those pages now render correctly in the UI, but their structured data silently drops the barcode. Fall back to the existing barcode field here as well.
Suggested fix
+$schemaBarcode = trim((string) (($book['ean'] ?? '') !== '' ? $book['ean'] : ($book['isbn13'] ?? '')));
+
if ($schemaType === 'MusicAlbum') {
@@
- if (!empty($book['ean'])) {
+ if ($schemaBarcode !== '') {
$bookSchema["identifier"] = [
"@type" => "PropertyValue",
"propertyID" => "EAN",
- "value" => $book['ean'],
+ "value" => $schemaBarcode,
];
}
} elseif ($schemaType === 'Movie') {
@@
- if (!empty($book['ean'])) {
+ if ($schemaBarcode !== '') {
$bookSchema["identifier"] = [
"@type" => "PropertyValue",
"propertyID" => "EAN",
- "value" => $book['ean'],
+ "value" => $schemaBarcode,
];
}
} elseif ($schemaType === 'CreativeWork') {
@@
- if (!empty($book['ean'])) {
+ if ($schemaBarcode !== '') {
$bookSchema["identifier"] = [
"@type" => "PropertyValue",
"propertyID" => "EAN",
- "value" => $book['ean'],
+ "value" => $schemaBarcode,
];
}Also applies to: 275-280, 304-309
🤖 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 249 - 266, When emitting
identifier JSON-LD, use the existing barcode fallback instead of only checking
$book['ean']; compute a $barcode = $book['ean'] ?? $book['isbn13'] ?? null and,
if set, assign $bookSchema["identifier"] with "@type"=>"PropertyValue",
"propertyID" set appropriately (e.g. "EAN" when $book['ean'] exists else
"ISBN13"), and "value"=>$barcode; apply the same change to the other branches
that currently only check $book['ean'] so the identifier is preserved for
music/non-book pages.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai Please review the latest commits: e1a58ed (bulk enrichment feature), 2bdba5a (set_time_limit fix), a383add (json.results path), and the test fixes. Focus on app/Services/BulkEnrichmentService.php, app/Controllers/BulkEnrichController.php, and scripts/bulk-enrich-cron.php |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12002 🏁 Script executed: Length of output: 13441
Review: e1a58ed · 2bdba5a · a383add + test fixes✅
|
| Area | Status |
|---|---|
set_time_limit(300) in start() |
✅ Correct |
enrichBook() missing-field-only writes |
✅ Correct |
enrichBatch() rate limiting & skipped counter |
✅ Correct |
toggle() boolean input (!empty) |
✅ Correct |
setEnabled / isEnabled DB safety |
✅ Correct |
storage/tmp/ missing directory in cron |
mkdir + file_put_contents return check |
| Test json.results path + unique ISBNs | ✅ Correct |
The only actionable item is the lock-file directory guard in the cron script. Everything else is solid.
- PluginManager: guard prepare()===false before bind_param in INSERT (line 132)
and before finally{close()} in setSetting (line 994). Without this, a schema
issue causes fatal instead of logged-and-continued.
- LibriApiController: multi-column ORDER BY for position sort now applies the
direction to each component (s.codice, m.numero_livello, COALESCE), not just
the last one. Descending position sort was partially reversed.
- MediaLabels::formatTracklist: strip_tags preserves attributes (onclick etc.)
on allowed tags — replaced with preg_match_all to extract <li> text, then
rebuild clean <ol class="tracklist"> with htmlspecialchars on each item.
Closes the XSS vector in the tracklist rendering path.
git archive converts symlinks to 22-byte files containing the target path. When PHP ZipArchive extracts this, it becomes a regular file. Updater.copyDirectory() then tries copy(file, existing_dir) which PHP rejects with: 'The second argument to copy() function cannot be a directory' This broke every manual update on installs that had public/installer/assets materialized as a directory (which is every install since the first one — ZipArchive never recreated it as a symlink). Fix: duplicate installer.js + style.css (16KB total) as real files under public/installer/assets/. No more symlink, no more conversion surprises. Discovered via production breakage on bibliodoc.fabiodalez.it after v0.5.4 manual upgrade attempt. Log: Updater.php:2441 copy() warning. Original symlink was added in commit a2b995b (2026-01-21).
…ory" This reverts commit 7f6c6a1.
…tory" This reverts commit b15d7c1.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Controllers & Services: - BulkEnrichController::start — do not leak raw exception messages to clients; log full details via SecureLogger, return 500 + generic i18n error (CR #4268310236) - BulkEnrichController::toggle — filter_var(FILTER_VALIDATE_BOOL) so "false"/"0"/"off" correctly disable (previously !empty could misfire) (CR #4268310236) - BulkEnrichmentService::setEnabled returns bool — swallowed execute() failures became silent, now propagates to controller which 500s - BulkEnrichmentService — NULLIF(TRIM(col), '') on isbn13/isbn10/ean so empty strings don't inflate pending stats or shadow populated ids; new firstNonEmpty() helper for priority selection (CR #3092...) - BulkEnrichmentService::enrichBook — check UPDATE execute() result and bail with status='error' if failed (avoids false-positive "enriched") - ScrapeController::normalizeIsbnFields — distinguish validated ISBN requests (byIsbn) from plugin-accepted barcode requests (Discogs UPC): validated ISBNs always reach backfill regardless of media signals, barcode-only stays gated by format/tipo_media (fixes book-flow regression introduced by the prior music-vs-book guard) Routes: - /admin/libri/bulk-enrich/start — rate-limit 1 req / 2 min (same as sync-covers) to prevent concurrent batches burning upstream quota (CR #3098885301) Cron: - scripts/bulk-enrich-cron.php — flock(LOCK_EX|LOCK_NB) atomic locking replaces racy file_exists+file_put_contents; non-zero exit on fatal errors so cron/monitoring record failures correctly Views: - admin/bulk-enrich.php — accessible switch with aria-label + aria-labelledby pointing at #toggle-label Tests: - bulk-enrich.spec.js — postWithCsrf now accepts request options separately (was sending timeout as form field); persistence test reads #toggle-enrichment aria-checked instead of stray page text - discogs-advanced.spec.js — /login fallback instead of /accedi; derive public URL from admin detail link rather than hardcoding /libro/ - discogs-import.spec.js — soft-delete + null unique cols in teardown instead of raw DELETE that breaks on FK; surface failures - discogs-plugin.spec.js — attach console listener before scrape and assert no errors (previously listener set after, no assertion) - multisource-scraping.spec.js — soft-delete teardown + surfaced errors (raw DELETE + empty catch hid FK failures) Release scripts: - create-release.sh / create-release-local.sh — zipinfo-based symlink detection in ZIP verification (replaces unreliable post-unzip find; macOS unzip recreates symlinks while Linux PHP ZipArchive doesn't)
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Support/PluginManager.php (1)
586-592:⚠️ Potential issue | 🟠 MajorClean up extracted files when the plugin row can't be prepared.
If
prepare()fails here, the nextbind_param()call throws into the outer catch, but the extracted directory is left on disk. A retry then hitsDirectory plugin già esistente.even though the plugin was never registered.Suggested fix
$stmt = $this->db->prepare(" INSERT INTO plugins ( name, display_name, description, version, author, author_url, plugin_url, is_active, path, main_file, requires_php, requires_app, metadata, installed_at ) VALUES (?, ?, ?, ?, ?, ?, ?, 0, ?, ?, ?, ?, ?, NOW()) "); + if ($stmt === false) { + $this->deleteDirectory($pluginPath); + SecureLogger::error('[PluginManager] Failed to prepare plugin insert', [ + 'plugin' => $pluginMeta['name'], + 'db_error' => $this->db->error, + ]); + return ['success' => false, 'message' => __('Errore durante il salvataggio nel database.'), 'plugin_id' => null]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 586 - 592, The INSERT prepare call ($this->db->prepare(...)) can fail and leave the extracted plugin directory on disk before bind_param() throws; update the code around prepare() in PluginManager.php to check the return value of $this->db->prepare and if it returns false/throws, remove the extracted plugin directory (the temporary path variable used just above, e.g. $extractedDir/$extractedPath) before re-throwing or returning; perform the same cleanup in the catch that currently catches the bind_param()/execute failure so the extracted files are always deleted on any DB prepare/bind/execute error, using the existing directory removal helper (or implement a small recursive rmdir helper) and log the cleanup.
♻️ Duplicate comments (3)
tests/bulk-enrich.spec.js (1)
173-180:⚠️ Potential issue | 🟠 MajorMirror
BulkEnrichmentService::getStats()instead of counting onlyisbn13.These queries still ignore
isbn10andean, whileapp/Services/BulkEnrichmentService.phpcounts all three identifiers withNULLIF(TRIM(...), ''). The page can be correct and this test can still fail as soon as the fixture set includes EAN-only or ISBN-10-only rows.Also applies to: 193-199, 209-215, 237-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bulk-enrich.spec.js` around lines 173 - 180, The test's SQL only counts rows with isbn13 and ignores isbn10 and ean, so update the dbQuery SQL used for pendingCount (and the other similar queries at the other noted blocks) to mirror BulkEnrichmentService::getStats(): treat isbn10, isbn13 and ean equally and use NULLIF(TRIM(column), '') to exclude blank strings (i.e., count a row as having an identifier if NULLIF(TRIM(isbn10), '') IS NOT NULL OR NULLIF(TRIM(isbn13), '') IS NOT NULL OR NULLIF(TRIM(ean), '') IS NOT NULL), and apply the same NULL/empty checks for copertina_url and descrizione and deleted_at; adjust the other dbQuery occurrences (the queries around the other reported ranges) the same way so test logic matches getStats().tests/discogs-plugin.spec.js (1)
37-60:⚠️ Potential issue | 🟡 MinorPre-clean
TEST_BARCODEbefore the suite starts.
afterAll()is best-effort. If a previous run dies after inserting the fixed EAN used in Line 180, the next run will trip the UNIQUE index onlibri.eanbefore test 6 even reaches the frontend assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-plugin.spec.js` around lines 37 - 60, The suite can fail on a UNIQUE constraint for libri.ean if a previous run left TEST_BARCODE inserted; add a pre-clean in test.beforeAll that calls dbExec to delete any rows with ean = TEST_BARCODE before creating the browser context and running the login flow. Target the test.beforeAll block and use the existing dbExec helper (same one used in test.afterAll) to run a DELETE FROM libri WHERE ean = TEST_BARCODE (or equivalent) so test 6 won't hit the duplicate EAN; keep test.afterAll cleanup as-is.app/Views/admin/bulk-enrich.php (1)
144-144:⚠️ Potential issue | 🟡 MinorHardcoded fetch endpoints still violate routing conventions.
Lines 144 and 199 concatenate
window.BASE_PATH + '/admin/libri/bulk-enrich/toggle'/'/admin/libri/bulk-enrich/start'. These URLs should be generated server-side viaroute_path('...')(orRouteTranslator::route('...')) and injected into JS withjson_encode(..., JSON_HEX_TAG), e.g. awindow.BULK_ENRICH_URLSconfig object. Otherwise any routing or base-path change will silently break this page.As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login" and "Usejson_encode(..., JSON_HEX_TAG)for any PHP→JS output in views".♻️ Sketch of the fix
<script> const csrfToken = <?= json_encode(Csrf::ensureToken(), JSON_HEX_TAG) ?>; +const BULK_ENRICH_URLS = <?= json_encode([ + 'toggle' => route_path('admin.libri.bulkEnrich.toggle'), + 'start' => route_path('admin.libri.bulkEnrich.start'), +], JSON_HEX_TAG) ?>; @@ - const response = await fetch(window.BASE_PATH + '/admin/libri/bulk-enrich/toggle', { + const response = await fetch(BULK_ENRICH_URLS.toggle, { @@ - const response = await fetch(window.BASE_PATH + '/admin/libri/bulk-enrich/start', { + const response = await fetch(BULK_ENRICH_URLS.start, {Adjust the route keys to whatever is registered in
app/Routes/web.phpfor the bulk-enrich toggle/start endpoints.Also applies to: 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/bulk-enrich.php` at line 144, Replace hardcoded fetch URLs in app/Views/admin/bulk-enrich.php by generating the toggle/start routes server-side and injecting them into JS; call route_path('...') or RouteTranslator::route('...') for the bulk-enrich toggle and start endpoints, json_encode the resulting array with JSON_HEX_TAG into a window.BULK_ENRICH_URLS object, and update the fetch calls (the occurrences currently building window.BASE_PATH + '/admin/libri/bulk-enrich/toggle' and '/admin/libri/bulk-enrich/start') to use window.BULK_ENRICH_URLS.toggle and window.BULK_ENRICH_URLS.start instead.
🧹 Nitpick comments (6)
public/installer/assets/style.css (1)
20-20: Stylelint: drop quotes aroundInterinfont-family.Per the repo's stylelint config (
font-family-name-quotes), unquoted identifiers are preferred for names that are valid CSS identifiers.♻️ Suggested fix
- font-family: 'Inter', -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji', sans-serif; + font-family: Inter, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji', sans-serif;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/installer/assets/style.css` at line 20, Remove the unnecessary quotes around the Inter font name in the font-family declaration in public/installer/assets/style.css (the line that sets font-family: 'Inter', ...); update the identifier to Inter (unquoted) to satisfy the stylelint rule `font-family-name-quotes` while leaving the rest of the font stack unchanged.public/installer/assets/installer.js (1)
96-102:autoDetectSocketcan clobber a user-entered socket path.On
DOMContentLoadedthe handler runs unconditionally whendb_host === 'localhost', and on everychangeof the host field. If the backend detects any socket, it will overwrite whatever the user already typed intodb_socket(e.g., a pre-populated value rendered server-side from a previous submission). Consider only overwriting when the socket field is empty.♻️ Suggested tweak
- if (host === 'localhost' && socketField) { + if (host === 'localhost' && socketField && !socketField.value) { fetch('index.php?action=detect_socket')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/installer/assets/installer.js` around lines 96 - 102, The autoDetectSocket call currently overwrites the db_socket input whenever run (on DOMContentLoaded and on db_host change); update the autoDetectSocket logic (and its invocation) so it only sets document.getElementById('db_socket').value when that element exists and its current value is empty (trimmed) to avoid clobbering user-entered or server-populated values; keep using the same function name autoDetectSocket and the same event hookup for db_host but add the empty check before assigning the detected socket.tests/discogs-import.spec.js (3)
209-210: Add thedeleted_at IS NULLguard for consistency.Line 195 correctly filters by
AND deleted_at IS NULL LIMIT 1, but line 209 queries the same row without that guard. Within this test run it doesn't matter (teardown hasn't executed yet), but it's a footgun if someone later reorders steps or reuses this pattern. Also consider wrappingbookIdwithNumber(...)to match line 195's style.- const tipoMedia = dbQuery(`SELECT tipo_media FROM libri WHERE id = ${bookId}`); + const tipoMedia = dbQuery( + `SELECT tipo_media FROM libri WHERE id = ${Number(bookId)} AND deleted_at IS NULL LIMIT 1` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 209 - 210, The SELECT used to assert tipo_media should include the same deleted_at guard and the same numeric casting as the earlier query: update the dbQuery call that assigns tipoMedia (dbQuery(...)) to add "AND deleted_at IS NULL" to the WHERE clause and wrap bookId with Number(bookId) so the query matches the earlier selection logic (same guard and numeric id usage).
148-153:authorCountis computed but never asserted.Lines 149‑150 locate
authorItemsand readauthorCount, but the value is discarded — the subsequent check at line 153 only re-assertstitleValue.length, which was already verified on line 145. Either drop the unused locator/count or add an actual assertion (e.g.expect(authorCount).toBeGreaterThan(0)) so the "author/artist is populated" intent of this block is real coverage rather than dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 148 - 153, The test computes authorItems and authorCount but never asserts it; update the test so the intent “author/artist is populated” is validated by adding an assertion for authorCount (e.g., expect(authorCount).toBeGreaterThan(0)) or remove the unused authorItems/authorCount lines; locate the authorItems and authorCount variables in the test and either add the assertion referencing authorCount or delete those lines and keep the existing titleValue checks.
128-136: Replace the fixed 8s sleep with explicit waiting.The
page.waitForTimeout(8000)at line 130 is the only signal that scraping finished. On slow CI or when Discogs throttles, 8s can be too short (test becomes flaky); on fast runs it wastes ~8s per invocation. Your sister suite (tests/multisource-scraping.spec.jslines 65‑71) already usesexpect(page.locator('input[name="titolo"]')).not.toHaveValue('', { timeout: 20000 })plusexpect.pollon#scrapeSourceName. Mirroring that pattern here would make this test both faster and more reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discogs-import.spec.js` around lines 128 - 136, Replace the fixed 8s sleep by explicitly waiting for the scraping UI to finish: remove page.waitForTimeout(8000) and instead use Playwright expectations on the same locators used elsewhere — assert that page.locator('input[name="titolo"]') has a non-empty value with a timeout (e.g. expect(...).not.toHaveValue('', { timeout: 20000 })) and also poll/wait for the scrape status element '#scrapeSourceName' to indicate completion (using expect.poll or expect(locator).toHaveText/contain with a timeout). Update the assertions around titleField/titleValue to rely on those explicit waits so the test is both faster and resilient to throttling.tests/multisource-scraping.spec.js (1)
192-200: Hardcodedclassificazione_dewey === '188'is fragile.This test asserts an exact Dewey code returned from an upstream Italian enrichment source for
9788804671664. If the upstream ever returns188.7(more specific) or a different classification for that ISBN, the test flakes without any regression in this codebase. Consider a looser check (expect(payload.classificazione_dewey).toMatch(/^188/)or just.toBeTruthy()), or pin the test input to a record whose Dewey is controlled by a local fixture/plugin. Optional — if the value is known to be stable in your target scraper config, leave as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/multisource-scraping.spec.js` around lines 192 - 200, The test "3. Scrape API returns enriched Italian metadata for a second book ISBN" is brittle because it asserts payload.classificazione_dewey === '188'; update the assertion in tests/multisource-scraping.spec.js to be looser by matching the Dewey prefix or just checking presence (e.g., change the expect for payload.classificazione_dewey to use a regex like expect(payload.classificazione_dewey).toMatch(/^188/) or expect(payload.classificazione_dewey).toBeTruthy()); keep the rest of the test using getScrapePayload and ITALIAN_ISBN 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/Controllers/LibriApiController.php`:
- Around line 141-145: The current filter adds AND l.tipo_media = ? only for
explicit matches; change the predicate so when $tipo_media === 'libro' it
matches explicit 'libro' plus NULL or empty values (e.g. "AND (l.tipo_media = ?
OR l.tipo_media IS NULL OR l.tipo_media = '')") and still push $tipo_media and
's' into $params/$types, while for other $tipo_media values keep the existing
"AND l.tipo_media = ?" behavior; use the existing hasColumn($db, 'tipo_media')
check to guard this change and update the $where, $params and $types
manipulation accordingly.
In `@public/installer/assets/installer.js`:
- Around line 20-27: The code directly dereferences
document.getElementById(...).value for multiple fields (db_host, db_username,
db_password, db_database, db_port, db_socket) which can throw if an input is
missing; in the testDatabaseConnection flow, first query each element into a
variable (e.g., const hostEl = document.getElementById('db_host')), then guard
before reading .value by using a safe access or default (e.g., const host =
hostEl ? hostEl.value : '' or hostEl?.value || ''), then append those safe
values to the FormData (or only append when the element exists), and keep
existing btn/result handling so the button cannot be left stuck if an element is
absent. Ensure you update the code around the formData construction that
currently references those IDs and leave the rest of the fetch/finally logic
unchanged.
- Around line 51-54: The current restore logic uses btn.textContent which strips
child elements (removing the <i> icon); instead update only the label node so
the icon remains: change the code around the .finally() handler that references
btn and getInstallerTranslation to either set btn.innerHTML to include the icon
plus the translated text or, preferably, modify the DOM to wrap the text in a
span (e.g., a .btn-text child) and update that span's textContent via
btn.querySelector('.btn-text')?.textContent = getInstallerTranslation(...);
ensure the installer button markup in step2 includes the span so the icon
element (the <i> with classes like fas fa-plug) is preserved.
- Around line 136-145: Delete the unused dead-code function
simulateInstallProgress (and any internal references to animateProgress within
it) from the assets file; remove the entire simulateInstallProgress declaration
since step3's template implements its own inline progress animation and there
are no callers to simulateInstallProgress or animateProgress in the codebase.
In `@public/installer/assets/style.css`:
- Around line 287-297: .alert-warning and .alert-info currently share identical
background and text colors making them indistinguishable; update .alert-warning
to use a distinct warning palette by adding/using dedicated CSS variables (for
example --brand-warning-bg, --brand-warning-border, --brand-warning-text) and
apply those to the .alert-warning rule (background, border-color, color) so it
contrasts clearly with .alert-info (e.g., amber/yellow-tinted background and
darker warning text), and add sensible fallback values for the new variables in
the root if needed.
- Around line 95-128: The header sets a black background with white text on
.installer-header but its child rules override the text to dark colors, making
them unreadable; fix by making the color rules consistent—either change
.installer-header's background to a light value (e.g., use the existing brand
variable like var(--brand-light-bg)) or change the child selectors
.installer-header h1 and .installer-header p to light text colors (e.g., white
or a lighter brand variable) so the heading and paragraph remain visible against
the header background; update the selectors .installer-header, .installer-header
h1, and .installer-header p accordingly.
In `@scripts/bulk-enrich-cron.php`:
- Around line 137-143: The code currently releases the lock and then deletes the
lock file (using `@unlink`($lockFile)), which reintroduces a race where another
process can reuse the pathname/inode and allow overlapping runs; remove the
unlink step so the finally block only releases the lock and closes the
descriptor (i.e., keep flock($lockHandle, LOCK_UN) and fclose($lockHandle) but
do not call `@unlink`($lockFile)), ensuring lockFile and its inode persist and
prevent overlapping batches; refer to lockHandle, lockFile, flock(), fclose() in
the change.
In `@scripts/create-release.sh`:
- Around line 189-195: SYMLINKS_IN_ZIP is currently populated with the symlink
target because awk '/^l/ {print $NF}' prints the last field; change the
assignment so it captures the symlink entry path instead (update the
SYMLINKS_IN_ZIP line that reads zipinfo "$ZIPFILE" | awk ...). Specifically,
parse the zipinfo long listing to print the field immediately before the literal
"->" (or stop at "->" and return the previous field) rather than $NF, so the
error output shows the offending repo path; keep the rest of the block (echo
messages and MISSING increment) unchanged and reference ZIPFILE, SYMLINKS_IN_ZIP
and MISSING when making the edit.
In `@tests/bulk-enrich.spec.js`:
- Around line 388-393: The test currently asserts a DB cover for targetId based
only on results.enriched > 0 which is flaky; update the test to first search
results.details for an entry where book_id === targetId (or equivalent property
name) and only if that entry exists assert that dbQuery(`SELECT
IFNULL(copertina_url, '') FROM libri WHERE id = ${targetId}`) returns a
non-empty cover; if no detail for targetId is present (meaning the batch
enriched other rows or the API was rate-limited), skip or do not perform the DB
assertion so the test no longer fails when a different record was updated.
In `@tests/multisource-scraping.spec.js`:
- Around line 305-309: Remove the weak generic "Barcode" substring check from
the music-badge detection and instead rely on the music-specific marker(s):
update the hasMusicBadge expression (variable name hasMusicBadge) to only check
for the FontAwesome music icon ('fa-compact-disc') or a more specific music-only
label emitted by MediaLabels (e.g. "Etichetta"/"Label"/"Anno di Uscita") rather
than 'Barcode'; likewise, for the later assertion use the
hasMusicLabel/hasBarcode flow so the test asserts music labeling explicitly by
matching the literal NEVERMIND_BARCODE constant as a secondary signal (replace
any html.includes('Barcode') checks with a check against NEVERMIND_BARCODE and
ensure the expectation is keyed off hasMusicLabel/hasMusicBadge rather than the
generic 'Barcode' string).
- Around line 120-129: The suite relies on Discogs being active but
test.beforeAll (which calls loginAsAdmin) does not enable it, creating an
implicit cross-file dependency; add an explicit activation step in
test.beforeAll that mirrors the activation used in the other test (call the same
helper used to enable Discogs—e.g., activateDiscogs() or
ensureSourceActive('discogs')—or perform the equivalent API/DB update) so that
Discogs is set to is_active=1 before assertions run, keeping loginAsAdmin and
the rest of the setup intact.
---
Outside diff comments:
In `@app/Support/PluginManager.php`:
- Around line 586-592: The INSERT prepare call ($this->db->prepare(...)) can
fail and leave the extracted plugin directory on disk before bind_param()
throws; update the code around prepare() in PluginManager.php to check the
return value of $this->db->prepare and if it returns false/throws, remove the
extracted plugin directory (the temporary path variable used just above, e.g.
$extractedDir/$extractedPath) before re-throwing or returning; perform the same
cleanup in the catch that currently catches the bind_param()/execute failure so
the extracted files are always deleted on any DB prepare/bind/execute error,
using the existing directory removal helper (or implement a small recursive
rmdir helper) and log the cleanup.
---
Duplicate comments:
In `@app/Views/admin/bulk-enrich.php`:
- Line 144: Replace hardcoded fetch URLs in app/Views/admin/bulk-enrich.php by
generating the toggle/start routes server-side and injecting them into JS; call
route_path('...') or RouteTranslator::route('...') for the bulk-enrich toggle
and start endpoints, json_encode the resulting array with JSON_HEX_TAG into a
window.BULK_ENRICH_URLS object, and update the fetch calls (the occurrences
currently building window.BASE_PATH + '/admin/libri/bulk-enrich/toggle' and
'/admin/libri/bulk-enrich/start') to use window.BULK_ENRICH_URLS.toggle and
window.BULK_ENRICH_URLS.start instead.
In `@tests/bulk-enrich.spec.js`:
- Around line 173-180: The test's SQL only counts rows with isbn13 and ignores
isbn10 and ean, so update the dbQuery SQL used for pendingCount (and the other
similar queries at the other noted blocks) to mirror
BulkEnrichmentService::getStats(): treat isbn10, isbn13 and ean equally and use
NULLIF(TRIM(column), '') to exclude blank strings (i.e., count a row as having
an identifier if NULLIF(TRIM(isbn10), '') IS NOT NULL OR NULLIF(TRIM(isbn13),
'') IS NOT NULL OR NULLIF(TRIM(ean), '') IS NOT NULL), and apply the same
NULL/empty checks for copertina_url and descrizione and deleted_at; adjust the
other dbQuery occurrences (the queries around the other reported ranges) the
same way so test logic matches getStats().
In `@tests/discogs-plugin.spec.js`:
- Around line 37-60: The suite can fail on a UNIQUE constraint for libri.ean if
a previous run left TEST_BARCODE inserted; add a pre-clean in test.beforeAll
that calls dbExec to delete any rows with ean = TEST_BARCODE before creating the
browser context and running the login flow. Target the test.beforeAll block and
use the existing dbExec helper (same one used in test.afterAll) to run a DELETE
FROM libri WHERE ean = TEST_BARCODE (or equivalent) so test 6 won't hit the
duplicate EAN; keep test.afterAll cleanup as-is.
---
Nitpick comments:
In `@public/installer/assets/installer.js`:
- Around line 96-102: The autoDetectSocket call currently overwrites the
db_socket input whenever run (on DOMContentLoaded and on db_host change); update
the autoDetectSocket logic (and its invocation) so it only sets
document.getElementById('db_socket').value when that element exists and its
current value is empty (trimmed) to avoid clobbering user-entered or
server-populated values; keep using the same function name autoDetectSocket and
the same event hookup for db_host but add the empty check before assigning the
detected socket.
In `@public/installer/assets/style.css`:
- Line 20: Remove the unnecessary quotes around the Inter font name in the
font-family declaration in public/installer/assets/style.css (the line that sets
font-family: 'Inter', ...); update the identifier to Inter (unquoted) to satisfy
the stylelint rule `font-family-name-quotes` while leaving the rest of the font
stack unchanged.
In `@tests/discogs-import.spec.js`:
- Around line 209-210: The SELECT used to assert tipo_media should include the
same deleted_at guard and the same numeric casting as the earlier query: update
the dbQuery call that assigns tipoMedia (dbQuery(...)) to add "AND deleted_at IS
NULL" to the WHERE clause and wrap bookId with Number(bookId) so the query
matches the earlier selection logic (same guard and numeric id usage).
- Around line 148-153: The test computes authorItems and authorCount but never
asserts it; update the test so the intent “author/artist is populated” is
validated by adding an assertion for authorCount (e.g.,
expect(authorCount).toBeGreaterThan(0)) or remove the unused
authorItems/authorCount lines; locate the authorItems and authorCount variables
in the test and either add the assertion referencing authorCount or delete those
lines and keep the existing titleValue checks.
- Around line 128-136: Replace the fixed 8s sleep by explicitly waiting for the
scraping UI to finish: remove page.waitForTimeout(8000) and instead use
Playwright expectations on the same locators used elsewhere — assert that
page.locator('input[name="titolo"]') has a non-empty value with a timeout (e.g.
expect(...).not.toHaveValue('', { timeout: 20000 })) and also poll/wait for the
scrape status element '#scrapeSourceName' to indicate completion (using
expect.poll or expect(locator).toHaveText/contain with a timeout). Update the
assertions around titleField/titleValue to rely on those explicit waits so the
test is both faster and resilient to throttling.
In `@tests/multisource-scraping.spec.js`:
- Around line 192-200: The test "3. Scrape API returns enriched Italian metadata
for a second book ISBN" is brittle because it asserts
payload.classificazione_dewey === '188'; update the assertion in
tests/multisource-scraping.spec.js to be looser by matching the Dewey prefix or
just checking presence (e.g., change the expect for
payload.classificazione_dewey to use a regex like
expect(payload.classificazione_dewey).toMatch(/^188/) or
expect(payload.classificazione_dewey).toBeTruthy()); keep the rest of the test
using getScrapePayload and ITALIAN_ISBN unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc02e9e7-da85-40f1-bb3a-fa9302e8c6c4
📒 Files selected for processing (20)
app/Controllers/BulkEnrichController.phpapp/Controllers/LibriApiController.phpapp/Controllers/ScrapeController.phpapp/Routes/web.phpapp/Services/BulkEnrichmentService.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Views/admin/bulk-enrich.phplocale/de_DE.jsonlocale/en_US.jsonpublic/installer/assetspublic/installer/assets/installer.jspublic/installer/assets/style.cssscripts/bulk-enrich-cron.phpscripts/create-release.shtests/bulk-enrich.spec.jstests/discogs-advanced.spec.jstests/discogs-import.spec.jstests/discogs-plugin.spec.jstests/multisource-scraping.spec.js
💤 Files with no reviewable changes (1)
- public/installer/assets
✅ Files skipped from review due to trivial changes (2)
- app/Services/BulkEnrichmentService.php
- app/Routes/web.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/discogs-advanced.spec.js
Comment on lines
+136
to
+145
| function simulateInstallProgress() { | ||
| let progress = 0; | ||
| const interval = setInterval(() => { | ||
| progress += 10; | ||
| animateProgress(progress); | ||
|
|
||
| if (progress >= 100) { | ||
| clearInterval(interval); | ||
| } | ||
| }, 300); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\b(simulateInstallProgress|animateProgress)\s*\(' --type=php --type=html --type=jsRepository: fabiodalez-dev/Pinakes
Length of output: 512
🏁 Script executed:
rg -r 'simulateInstallProgress' --type-list | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 625
🏁 Script executed:
rg 'simulateInstallProgress' -iRepository: fabiodalez-dev/Pinakes
Length of output: 206
🏁 Script executed:
fd -type f -name '*step*' -o -name '*Step*' | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
git ls-files | grep -i 'installer\|step' | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 1308
🏁 Script executed:
find installer -name "*.html" -o -name "*.php" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 357
🏁 Script executed:
rg -i 'step.?3|step3' --max-count=20Repository: fabiodalez-dev/Pinakes
Length of output: 2579
🏁 Script executed:
fd '' installer -type f | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
cat installer/steps/step3.phpRepository: fabiodalez-dev/Pinakes
Length of output: 6095
🏁 Script executed:
cat public/installer/assets/installer.js | head -200Repository: fabiodalez-dev/Pinakes
Length of output: 5172
Remove simulateInstallProgress — it's unused dead code.
Step 3's template (installer/steps/step3.php) implements its own inline progress animation and never calls simulateInstallProgress() or animateProgress(). The function exists in the asset file but has no invocation anywhere in the codebase and should be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/installer/assets/installer.js` around lines 136 - 145, Delete the
unused dead-code function simulateInstallProgress (and any internal references
to animateProgress within it) from the assets file; remove the entire
simulateInstallProgress declaration since step3's template implements its own
inline progress animation and there are no callers to simulateInstallProgress or
animateProgress in the codebase.
- version.json: 0.5.4 → 0.5.5
- README.md: badge + What's New for v0.5.5 + v0.5.4 changelog block
- All 10 plugin.json: max_app_version → 0.5.5
- BundledPlugins::LIST: + deezer, + musicbrainz (now 9 plugins)
- create-release.sh / create-release-local.sh: BUNDLED_PLUGINS list
aligned with BundledPlugins::LIST (9 entries)
- .gitignore: whitelist storage/plugins/{deezer,musicbrainz}/ so these
ship in the release ZIP; add scraping-pro-*.zip.sha256 pattern so the
premium plugin checksum never leaks to git
scraping-pro stays gitignored and out of the release ZIP (premium,
distributed separately as scraping-pro-v1.4.2.zip).
Major:
- PluginManager::installPlugin — deleteDirectory($pluginPath) when INSERT
prepare() fails; otherwise next install retry would hit "directory già
esistente" without a plugin row
- LibriApiController::list — tipo_media='libro' filter now also matches
NULL/empty (legacy rows on upgraded installs) via OR predicate
- installer.js: null-safe getElementById()?.value in
testDatabaseConnection (no more TypeError leaving the button stuck);
restore button via safe DOM rebuild preserving the <i class="fa-plug">
icon instead of textContent (which stripped it)
- installer.js: autoDetectSocket only writes when socket field is empty —
never clobber user/server-prefilled values
- installer.js: removed dead simulateInstallProgress + animateProgress
- style.css: .installer-header background now white (child h1/p use dark
brand color; black bg made them invisible)
- style.css: .alert-warning uses amber palette (#fff8e1 / #f59e0b /
#78350f) — was indistinguishable from .alert-info
- style.css: drop 'Inter' single-quotes (stylelint
font-family-name-quotes)
- bulk-enrich-cron.php: finally{} no longer @Unlink($lockFile) — keeping
the inode stable prevents a race where another process could recreate
the pathname with a new inode and bypass the lock; ftruncate+rewind
clears stale PID for the next run
- create-release.sh + create-release-local.sh: fixed zipinfo symlink
extraction — awk $NF printed the link TARGET, now parses the field
before "->" so the error reports the offending repo path
- bulk-enrich.spec.js queries (4 blocks) mirror getStats() —
NULLIF(TRIM(...)) on isbn10/isbn13/ean so EAN-only / ISBN-10-only
fixtures don't make the test flake
- bulk-enrich.spec.js test 9: assert cover only when results.details
contains an 'enriched' entry for targetId (prevents false negatives
when the batch touched a different pending row)
- bulk-enrich.php: server-side URL injection via BULK_ENRICH_URLS object
built with url() helper — no more window.BASE_PATH + '/...'
concatenation in JS
- multisource-scraping.spec.js: explicit discogs activation in beforeAll
instead of relying on alphabetical test-file ordering
- multisource-scraping.spec.js: music-badge check drops the weak
'Barcode' substring — now keyed on fa-compact-disc / 'Etichetta' /
'Anno di Uscita' or the actual stored EAN value
Nitpick:
- discogs-plugin.spec.js: pre-clean TEST_BARCODE in beforeAll
(soft-delete + null unique cols) so a crashed prior run can't trip
the UNIQUE index on libri.ean
- discogs-import.spec.js: replace waitForTimeout(8000) with
expect(titleField).not.toHaveValue('', { timeout: 20000 }) — faster
on happy path, tolerant on slow CI / Discogs throttling
- discogs-import.spec.js: authorCount assertion (regression guard on
the Choices.js DOM selector) + tipo_media SELECT gets deleted_at
guard + Number() cast
- multisource-scraping.spec.js: Dewey assertion loosened to
toMatch(/^188/) — upstream may return '188.7'
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
New optional plugin that integrates with the Discogs API for music metadata scraping (CD, LP, vinyl, cassette).
Closes #87
How it works
scrape.sources,scrape.fetch.custom,scrape.data.modify)Configuration
Files
storage/plugins/discogs/DiscogsPlugin.php— Main plugin classstorage/plugins/discogs/wrapper.php— PluginManager compatibility wrapperstorage/plugins/discogs/plugin.json— Plugin manifeststorage/plugins/discogs/README.md— Documentation.gitignore— Added discogs plugin exceptionsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI Improvements
Localization
Documentation
Tests
Chores / Bug Fixes