feat: Discogs music scraper plugin (#87)#100
Open
fabiodalez-dev wants to merge 42 commits intomainfrom
Open
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)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (52)
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 scraper plugin, a new media-type (tipo_media) enum and migration, MediaLabels helpers, plugin/updater refactors, admin plugin settings and routing, extensive Playwright E2E tests, CSV/import/schema-aware changes, UI/media badges/filters, and related localization and packaging updates. 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/scrape request (barcode/title)
App->>Discogs: Search release by barcode
Discogs-->>App: Release list / none
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 none
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🎯 4 (Complex) | ⏱️ ~60 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)
1. Discogs: register scrape.isbn.validate for EAN/UPC barcodes 2. mergeBookData: dynamic source from payload, not hardcoded 'discogs' 3. enrichWithDiscogsData: resolveTipoMedia (honors explicit tipo_media) 4. Authors array normalize (handle structured ['name' => '...']) 5. strip_tags: br→newline before stripping in tracklist rendering 6. PluginManager getPluginInstance: try/catch \Throwable → null 7. Updater: backup cleanup non-fatal after successful swap 8. Migration: AFTER formato guard (check column exists)
Security: - Cast releaseId to int (SSRF prevention) - Add CURLOPT_PROTOCOLS, CURLOPT_MAXREDIRS, CURLOPT_CONNECTTIMEOUT, CURLOPT_SSL_VERIFYPEER to all 4 curl call sites - Add curl_error() logging on CoverArt, MusicBrainz, Deezer calls Bug fixes: - inferTipoMedia: exact match for short tokens 'cd'/'lp' to prevent false positives on 'CD-ROM' and words containing 'lp' - formatTracklist: detect pre-formatted <ol> HTML and return as-is - Schema.org: separate CreativeWork branch (no isbn/numberOfPages) - Migration SQL: replace %cd%/%lp% LIKE with specific patterns Code quality: - Remove dead $discogsToken parameter from searchMusicBrainz() - Make Discogs rate limiter static (consistent with MusicBrainz) - Make Deezer rate limit elapsed-based (not fixed usleep) - Add missing translation key to en_US and de_DE Config: - Full .coderabbit.yaml with 9 path_instructions sections
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- Optional plugins (discogs) now start inactive on first registration via metadata.optional flag in plugin.json + PluginManager check - Add aria-label/role=img on media type icons in DataTable (a11y) - Add aria-label on token visibility toggle button - Use MediaLabels::label() for ISBN-13 label in frontend (was hardcoded) - Fix test identifiers: EAN/ISBN now use RUN_ID to avoid collisions - Fix test teardown: soft-delete instead of hard DELETE - Translation fixes: Disco→Disc, Cartaceo→Print (en_US) - Add Mostra/nascondi token translation key (en_US, de_DE)
Logic: - ScrapeController::normalizeIsbnFields now skips when neither format nor tipo_media is supplied (inferTipoMedia(null) defaults to 'libro', which would re-introduce the EAN-in-isbn13 bug if a plugin returned partial data) - DiscogsPlugin: new logApiFailure() helper logs every non-200 HTTP with severity by code (401/403=error, 429/5xx=warning, 404=debug). Wired into apiRequest/musicBrainzRequest/fetchCoverArtArchive/enrichFromDeezer - DiscogsPlugin: numero_inventario no longer pre-filled from Discogs catalog number (Cat# already in note_varie) — was clobbering library inventory prefix - PluginManager: 31 error_log() → SecureLogger::warning() per project rule #5 on sensitive-context logging (DB errors contain schema metadata) Metadata: - plugin.json: version 1.0.0→1.1.0 (aligned with getInfo()), requires_app 0.5.0→0.5.4 (tipo_media column dependency), 4th hook scrape.isbn.validate now declared, feature "parole chiave" added - README.md: "tramite tre hook" → "quattro hook"; removed Deezer genre enrichment claim (enrichFromDeezer() only fills cover, never genres) - views/settings.php:80: "per i libri" → "di scraping" Tests: - full-test.spec.js Phase 10.4 (#77): CSV counter now quote-aware. The previous failure was a test bug — controller was correct, but naive split on newlines over-counted rows due to embedded newlines in tracklist cells - discogs-import.spec.js test 3: assertion now strict (ean===barcode && isbn13===''), previously the '||' masked the barcode→isbn13 regression - pr100-media-types.spec.js test 8: added row-count assertion on ?ids= export - seed-catalog.spec.js: skipped by default (set E2E_RUN_SEED=1 to run) — was polluting DB state on pattern-matched runs - NEW tests/pr100-review-fixes.spec.js: 12 tests covering import→DB→admin →frontend lifecycle for every field touched, plus metadata accuracy and soft-delete behavior. All 12 pass (5.6s)
UNIQUE constraint on libri.ean blocked the save step when a previous run left behind a non-soft-deleted record with the same Discogs test barcode (e.g. Nevermind 0720642442524 from the seed catalog). Soft-delete + nullify unique columns in beforeAll so each run starts from a clean slate. Before: 4/6 passed, save step timed out on waitForURL because the POST returned the form with a UNIQUE-violation error. After: 6/6 passed (13.6s).
INSERT into plugins listed 14 columns but VALUES provided only 12 ? + NOW() = 13 values — mysqli threw "Column count doesn't match value count at row 1" on every fresh install, blocking the bundled-plugin auto-registration AND the entire request (exception bubbles up to public/index.php:345). Regression source: commit fc399cb (round 11 CodeRabbit fix) rewrote the VALUES line and dropped the metadata placeholder. bind_param was already typed for 13 args (matching the 13 bound variables) — the SQL was the only side out of sync. Test gap: full-test.spec.js Phase 1 skips when the app is already installed, so autoRegisterBundledPlugins was never exercised in CI. Manual fresh-install testing caught it only when it hit production.
Type string 'ssssssssissss' had positions 8 and 9 swapped — is_active (integer) was typed as 's' and path (string) as 'i'. mysqli silently coerced path='discogs'/'goodlib' to int 0 because the values were not numeric strings. The same-request orphan-plugin detection then saw storage/plugins/0 missing and DELETEd the freshly inserted rows, leaving fresh installs with only 5 of the 7 bundled plugins (and the ones that survived did so only because the legacy installer path at Installer.php:1347 registered them via PDO before autoRegister ran). Discovered while running smoke-install.spec.js against a fresh DB on test-upgrade/. The previous 0.5.4 hotfix (c9bd82c) fixed only the placeholder count — the type-order bug was independent and co-resident in the same bind_param call. Verified: 13/13 smoke-install passed, 7/7 bundled plugins registered with correct path, discogs correctly inactive (optional), goodlib active.
Discogs is bundled+optional, so PluginManager auto-registers it with is_active=0 on fresh install. The previous test 1 only probed is_active=1 and tried to activate via a /admin/plugins page visit, which is a no-op for optional plugins. On fresh install (no previous state) the test failed even though the plugin was correctly registered. New test 1 asserts registration (the real regression signal — catches the bind_param path=0 bug from fb1e881) and then forces activation via direct UPDATE. Subsequent tests 2-6 exercise the scraping flow with an active plugin.
Playwright orders test files alphabetically, so smoke-install.spec.js
(which installs the app) runs AFTER discogs-import/pr100-* when invoked
in one command. On a fresh DB the data-heavy specs were failing with
hard timeouts instead of skipping cleanly.
Changes:
- smoke-install.spec.js: probe installer availability in beforeAll,
skip all 8 installer steps when app is already installed; login +
CRUD tests still run against the existing install.
- discogs-import.spec.js, pr100-review-fixes.spec.js (lifecycle
describe only — offline tests still run), pr100-media-types.spec.js:
check that plugins/libri/utenti tables exist in beforeAll; skip with
clear message otherwise ("run tests/smoke-install.spec.js first").
Result — one-shot run on fresh DB:
20 passed + 21 skipped, 0 failed (was 3 failed before fix).
On re-runs (DB already installed): installer steps skip, data specs
run against populated schema.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Re-running the suite on an already-populated DB hit Choices.js autocomplete-select on 'Umberto Eco' instead of the inline-create path, stalling the form submit and timing out page.fill on subsequent tests. Making title and author unique per run makes the spec idempotent across re-runs.
The final verify step searched 'Edizione Rivista' which also matches prior-run records — data[0] then returned a different book and the toContain assert failed. Search by RUN_ID and find the specific record instead of indexing data[0].
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review Please review the following new commits that were pushed after the last review:
Key changes to inspect:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22116 🏁 Script executed: Length of output: 5350 🏁 Script executed: Length of output: 8737 🏁 Script executed: Length of output: 2557
Review: commits 875a710 · c9bd82c · fb1e881 (+ test commits)✅ Critical Bug Fixes (c9bd82c + fb1e881) — CorrectBoth fixes address a fresh-install regression introduced in the prior round-11 CodeRabbit follow-up (fc399cb):
The inline comment in the code explaining the column-type order is a good addition for future maintainers. ✅
|
| HTTP code | Level |
|---|---|
| 404, 0 | debug |
| 401, 403 | error |
| 429, 5xx, others | warning |
The httpCode === 0 → debug case is safe: cURL connection failures already emit a SecureLogger::warning at line 804 before logApiFailure is called, so there is no silent swallowing — the debug log is simply a secondary breadcrumb. ✅
The helper is also correctly wired into musicBrainzRequest, fetchCoverArtArchive, and enrichFromDeezer. ✅
✅ numero_inventario Removal (875a710 / DiscogsPlugin.php)
Correct. The catalog number was already saved in note_varie (Cat#: ...), so pre-filling numero_inventario was clobbering the library's internal per-copy inventory prefix. The clarifying comment is clear. ✅
⚠️ SecureLogger::warning() Used for Normal Operational Events (PluginManager.php)
The migration from error_log() → SecureLogger::warning() is the right direction (removes raw log leakage of schema metadata), but SecureLogger::info() exists and should be preferred for non-error operational events. Using warning for routine success messages pollutes warning-level log filtering. For example:
// These are informational — not warnings:
SecureLogger::warning("[PluginManager] Updated bundled plugin: $pluginName $dbVersion → $diskVersion");
SecureLogger::warning("[PluginManager] Auto-registered bundled plugin: $pluginName (ID: $pluginId, $activeLabel)");
SecureLogger::warning("[PluginManager] Auto-registered $registered bundled plugin(s)");
SecureLogger::warning("✅ [PluginManager] Plugin installed successfully: {$pluginMeta['name']} (ID: $pluginId)");
SecureLogger::warning("[PluginManager] Cleaned up {$deleted} orphan plugin(s) from database");Suggested replacements:
// Success/normal operational events → info or debug:
SecureLogger::info("[PluginManager] Updated bundled plugin: $pluginName $dbVersion → $diskVersion");
SecureLogger::info("[PluginManager] Auto-registered bundled plugin: $pluginName (ID: $pluginId, $activeLabel)");
SecureLogger::info("[PluginManager] Auto-registered $registered bundled plugin(s)");
SecureLogger::info("[PluginManager] Plugin installed successfully: {$pluginMeta['name']} (ID: $pluginId)");
SecureLogger::info("[PluginManager] Cleaned up {$deleted} orphan plugin(s) from database");
// Actual failures remain at warning:
SecureLogger::warning("[PluginManager] Failed to prepare bundled plugin update for $pluginName: " . $this->db->error);
// ...etcThe same applies to the stack trace log for installation errors — error() would be more appropriate than warning() there:
SecureLogger::error("❌ [PluginManager] Stack trace: " . $e->getTraceAsString());✅ Test Commits
- 4ff5d3b — Pre-cleaning
ean/isbn13/isbn10before each test run solves the UNIQUE-constraint collision correctly. Thetry/catch {}silencing is acceptable for test teardown/setup. - 434b46f — Activating the optional discogs plugin via SQL in test 1 avoids UI flakiness. ✅
- 76daf8d, 402a4a1, 5f1d302 — RUN_ID isolation and installed-state guards are good practices for serial E2E suites. ✅
Summary
| Area | Status |
|---|---|
| Fresh-install placeholder regression (c9bd82c) | ✅ Fixed |
| bind_param type-order regression (fb1e881) | ✅ Fixed |
normalizeIsbnFields no-signal guard |
✅ Correct |
logApiFailure helper |
✅ Well-designed |
numero_inventario removal |
✅ Correct |
error_log → SecureLogger migration |
info() for success events, not warning() |
| Test fixes | ✅ Correct |
Critical: - PluginManager::encryptPluginSettingValue — no longer falls back to plaintext on crypto failure or missing APP_KEY. Throws RuntimeException so callers surface the problem to the admin UI instead of silently storing a secret in cleartext (silent-failure #C2 + backend-security #3). - PluginManager: 8 SecureLogger::warning → ::error for real failure paths (DB prepare/execute on lookup/update, auto-register, orphan cleanup, instantiate, install catch-all, plugin load). The error at line 203 ("Failed to auto-register") is specifically the one that masked the bind_param type-swap bug in fb1e881; error severity ensures it surfaces in monitoring next time. Context arrays replace string concatenation so SecureLogger redaction and Sentry grouping work correctly (silent-failure #C1, project rule #5). - migrate_0.5.4.sql: replace POSIX word-boundary [[:<:]]/[[:>:]] (removed in MySQL 8.0.4+) with ICU \b. Without this the migration throws ERROR 3692 on modern MySQL (db-migration #1). Important: - DiscogsPlugin: UUID-validate MBID before URL concat + rawurlencode as defence in depth. Previously a spoofed upstream response could inject .. / ? / # into the detail URL (security-auditor #1, CWE-918). - DiscogsPlugin: all four cURL handles now set CURLOPT_PROTOCOLS = HTTPS only + CURLOPT_REDIR_PROTOCOLS = HTTPS. Closes HTTPS→HTTP downgrade on redirect that could leak the Authorization header with the Discogs token in cleartext (backend-security #1, CWE-601/CWE-918). - DiscogsPlugin scrape.isbn.validate: priority aligned from 5 to 8 in both activate() and registerHooks() to match plugin.json declaration (code-reviewer #1). - ScrapeController::normalizeIsbnFields no-signal guard promoted from debug to warning with richer context (source + payload keys). Makes misbehaving scrapers visible in production (silent-failure #H1).
CodeRabbit PR review flagged that the error_log→warning sed pass also demoted genuine info-level success events to warning (polluting warning-level log filtering). Reclassify per CodeRabbit suggestion: - Updated bundled plugin → info - Auto-registered bundled plugin → info - Auto-registered N bundled plugin(s) → info - Cleaned up N orphan plugin(s) → info - Plugin installed successfully → info - Installation stack trace → error (was warning, emoji removed)
.gitattributes:5 had 'public/installer/assets export-ignore' which caused git archive to drop installer.js and style.css from the ZIP. Result: fresh install step 2 (DB test connection) silently failed because the JS was never loaded by the browser — the PHP built-in server fell back to the Slim router which served the installer HTML page instead of the JavaScript file. The admin saw a responsive form but "Test Connection" button did nothing. Discovered by running smoke-install.spec.js on the 0.5.4 local ZIP against a clean DB on port 8084. The v0.5.3 release was unaffected because the v0.5.3 ZIP was built before this export-ignore line was added.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review 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)
627-645:⚠️ Potential issue | 🟠 MajorRollback the install when
onInstall()fails.At this point the plugin files are already extracted and the
pluginsrow is already committed. IfonInstall()throws, this returns an error but leaves a partially installed plugin behind, so the UI says "failed" while the system state says "installed".Suggested fix
- // Run plugin installation hook if exists - $this->runPluginMethod($pluginMeta['name'], 'onInstall'); + // Run plugin installation hook if exists + try { + $this->runPluginMethod($pluginMeta['name'], 'onInstall'); + } catch (\Throwable $e) { + $cleanupStmt = $this->db->prepare("DELETE FROM plugins WHERE id = ?"); + if ($cleanupStmt !== false) { + $cleanupStmt->bind_param('i', $pluginId); + $cleanupStmt->execute(); + $cleanupStmt->close(); + } + $this->deleteDirectory($pluginPath); + throw $e; + } SecureLogger::info("[PluginManager] Plugin installed successfully: {$pluginMeta['name']} (ID: $pluginId)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 627 - 645, When calling $this->runPluginMethod($pluginMeta['name'], 'onInstall') in PluginManager, wrap the onInstall invocation in a try/catch so that if it throws you perform a rollback: remove the extracted plugin files and delete the just-created plugins DB row (the same plugin_id you set earlier) before logging the error and returning failure. Use the existing helpers/methods in PluginManager that manage filesystem and DB (the code paths that created the plugin files and inserted the plugins row) to perform the cleanup, ensure setPluginId remains called as before, and rethrow or return the error result after rollback so the system state remains consistent with the failure.
♻️ Duplicate comments (20)
app/Support/Updater.php (1)
2304-2333:⚠️ Potential issue | 🟡 MinorHandle symlinks explicitly in
removeDirectoryTree().This cleanup still assumes every entry can be deleted with
unlink()/rmdir()based only onisDir(). For plugin trees containing symlinks, that is brittle: directory symlinks can make backup/staging cleanup fail and hide the original swap error. Checkis_link($path)for the root and$item->isLink()inside the iterator, then remove symlinks withunlink()before the directory/file branches.In PHP, when using RecursiveDirectoryIterator/RecursiveIteratorIterator without FOLLOW_SYMLINKS, what is the correct cross-platform way to delete symlink entries and a symlink root path: SplFileInfo::isLink()/is_link() + unlink(), or 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 removeDirectoryTree function must explicitly handle symlinks: before checking is_dir on the root $path, detect and unlink symlinks (is_link($path)) and return; inside the RecursiveIteratorIterator loop, check for $item->isLink() first and remove those with unlink() (not rmdir), then proceed to the existing isDir() / unlink() branches for real directories and files; update exception messages/errors accordingly and ensure you use is_link() for the root and SplFileInfo::isLink() for iterator entries to make symlink removal robust across platforms.app/Views/admin/plugins.php (1)
244-244:⚠️ Potential issue | 🟠 MajorUse a dedicated named settings route instead of concatenating path segments.
At Line 244 and Line 300, the URL is still built with
route_path('plugins') . '/{id}/settings', which hardcodes route structure and can break with route changes/localization. Use the named settings route key directly (withidparam), then escape withhtmlspecialchars(..., 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.Also applies to: 300-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` at line 244, Replace the concatenated URL used in the data-settings-url attribute (currently built as route_path('plugins') . '/' . (int) $plugin['id'] . '/settings') with a call to the named settings route and pass the id as a parameter (e.g. route_path('plugins.settings', ['id' => (int) $plugin['id']])) and then wrap that result with htmlspecialchars(..., ENT_QUOTES, 'UTF-8'); update both occurrences referenced (the data-settings-url attribute and the duplicate at line ~300) so you stop hardcoding path segments and use the route key instead.app/Controllers/LibriController.php (1)
679-680:⚠️ Potential issue | 🟠 MajorAvoid always seeding
tipo_mediain defaults.At Line 679 and Line 1220, forcing
'tipo_media' => ''keeps the key present even when the request did not provide it, which can silently override inferred/scraped media-type behavior downstream.Suggested fix
- 'numero_pagine' => null, - 'tipo_media' => '', + 'numero_pagine' => null, @@ foreach ($fields as $k => $v) { if (array_key_exists($k, $data)) $fields[$k] = $data[$k]; } + if (array_key_exists('tipo_media', $data) && trim((string) $data['tipo_media']) !== '') { + $fields['tipo_media'] = trim((string) $data['tipo_media']); + }Use this read-only check to confirm downstream behavior before merging:
#!/bin/bash set -euo pipefail BOOK_REPO=$(fd -i "BookRepository.php" | head -n1) LIBRI_CTRL=$(fd -i "LibriController.php" | head -n1) echo "Inspecting: $BOOK_REPO" echo "Inspecting: $LIBRI_CTRL" rg -n -C4 "function updateBasic|function updateOptionals|tipo_media|scraped_tipo_media" "$BOOK_REPO" "$LIBRI_CTRL"Expected: if repository logic updates on key presence, seeded empty
tipo_mediais unsafe.Also applies to: 1220-1221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 679 - 680, The defaults array in LibriController is always seeding 'tipo_media' => '' which forces the key even when the request omits it; remove that unconditional seed and instead only set tipo_media when the incoming payload/request explicitly provides it (or leave it unset/null) so downstream logic (e.g., BookRepository methods updateBasic/updateOptionals and any scraped_tipo_media handling) can decide whether to infer/scrape the media type; update the code paths in LibriController where defaults are constructed and ensure any callers that merge these defaults respect absence vs empty-string for tipo_media.app/Controllers/FrontendController.php (1)
904-908:⚠️ Potential issue | 🟡 MinorDon’t leave unsupported
tipo_mediaas a silent no-op.If the column is missing, the filter is dropped internally but still appears active to the client, causing mismatched UX/result semantics.
Consider explicitly clearing/flagging unsupported filters in the response path so UI/API state matches applied SQL filters.
Also applies to: 917-927
🤖 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 filters when the DB column is missing (e.g., the filters['tipo_media'] branch using hasLibriColumn), causing client-visible filters to not match applied SQL; modify the branch so that when hasLibriColumn($db, 'tipo_media') is false you either remove/clear filters['tipo_media'] (unset or set to null) or mark an explicit flag (e.g., filters_applied or unsupported_filters[]) that will be returned in the response, and apply the same change for the other similar blocks around the 917-927 area so the API/UI sees which filters were actually applied versus rejected by missing columns.app/Views/frontend/catalog-grid.php (1)
41-44:⚠️ Potential issue | 🟡 MinorAdd an explicit accessible name for the media badge.
At Line 41–44, the badge is icon-only and depends on
title; screen readers may not announce it reliably.Suggested fix
- <span class="book-media-badge" title="<?= 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') ?>"></i> + <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> </span>🤖 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 - 44, The media badge (span with class "book-media-badge" that uses \App\Support\MediaLabels::tipoMediaDisplayName($book['tipo_media']) and \App\Support\MediaLabels::icon($book['tipo_media'])) is icon-only and lacks an explicit accessible name; update the span to provide one — either add a proper aria-label (e.g. aria-label="<?= htmlspecialchars(\App\Support\MediaLabels::tipoMediaDisplayName($book['tipo_media']), ENT_QUOTES, 'UTF-8') ?>") and role="img", or include a visually-hidden text node (screen-reader-only span) containing the output of MediaLabels::tipoMediaDisplayName(...) so screen readers reliably announce the badge.app/Views/libri/partials/book_form.php (1)
3615-3621:⚠️ Potential issue | 🟡 MinorNormalize and whitelist scraped
tipo_mediabefore assigning it.Directly setting
tipoMediaSelect.value = data.tipo_mediacan silently fail when sources return non-canonical labels (CD,Vinyl, etc.), leaving the form out of sync with intended media type.Suggested fix
- try { - if (data.tipo_media) { - const tipoMediaSelect = document.getElementById('tipo_media'); - if (tipoMediaSelect) { - tipoMediaSelect.value = data.tipo_media; - } - } - } catch (err) { - } + try { + if (data.tipo_media) { + const tipoMediaSelect = document.getElementById('tipo_media'); + if (tipoMediaSelect) { + const allowed = new Set( + Array.from(tipoMediaSelect.options).map((opt) => String(opt.value)) + ); + const normalized = String(data.tipo_media).trim().toLowerCase(); + if (allowed.has(normalized)) { + tipoMediaSelect.value = normalized; + } + } + } + } 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 - 3621, The scraped data.tipo_media must be normalized and whitelisted before assigning to the select: implement a mapping of common variants (e.g., "CD", "Compact Disc" -> canonical "cd"; "Vinyl", "LP" -> "vinyl", etc.) and check that the mapped value exists as an option in the tipo_media select; in the code handling data.tipo_media (referencing data.tipo_media and tipoMediaSelect), convert the incoming label via the whitelist/map, then only set tipoMediaSelect.value if the mapped value is non-empty and matches an option in the select, otherwise skip or set a safe default.app/Views/frontend/catalog.php (1)
1302-1326:⚠️ Potential issue | 🟡 Minor
tipo_mediais still only partially wired on the client.After clicking a media type, the sidebar active class is not resynced (only
disponibilitais), and active-filter chips can still show raw enum values instead of human labels.Suggested fix
@@ - <a href="#" - class="filter-option <?= $isActive ? 'active' : '' ?>" + <a href="#" + class="filter-option tipo-media-option <?= $isActive ? 'active' : '' ?>" + data-filter-value="<?= 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;"> <i class="fas <?= htmlspecialchars((string)$tmInfo['icon'], ENT_QUOTES, 'UTF-8') ?> me-1"></i> - <?= htmlspecialchars((string)$tmInfo['label'], ENT_QUOTES, 'UTF-8') ?> + <span class="tipo-media-label"><?= htmlspecialchars((string)$tmInfo['label'], ENT_QUOTES, 'UTF-8') ?></span> </a> @@ function updateFilter(key, value) { @@ - if (key === 'disponibilita') { - syncAvailabilityActiveState(); - } + if (key === 'disponibilita') syncAvailabilityActiveState(); + if (key === 'tipo_media') syncTipoMediaActiveState(); @@ } + +function syncTipoMediaActiveState() { + const currentValue = currentFilters.tipo_media || ''; + const options = document.querySelectorAll('.tipo-media-option'); + options.forEach((option) => { + const targetValue = option.dataset.filterValue || ''; + option.classList.toggle('active', targetValue === currentValue); + }); +} @@ document.addEventListener('DOMContentLoaded', () => { @@ syncAvailabilityActiveState(); + syncTipoMediaActiveState(); }); @@ } else if (filterKey === 'genere_id') { displayValue = currentGenreName || value; + } else if (filterKey === 'tipo_media') { + const match = Array.from(document.querySelectorAll('.tipo-media-option')) + .find((opt) => (opt.dataset.filterValue || '') === String(value)); + const labelNode = match ? match.querySelector('.tipo-media-label') : null; + displayValue = labelNode ? labelNode.textContent.trim() : value; }Also applies to: 1539-1553, 1646-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 1302 - 1326, The media-type sidebar options render anchors with class "filter-option" and call updateFilter('tipo_media', ...), but the client-side updateFilter currently only updates disponibilita and doesn't resync the sidebar active class nor set human-readable labels in the active-filter chips; fix by: add a data-label attribute to each media anchor when rendering (use $tmInfo['label']), and modify the updateFilter JS to (1) find all .filter-option elements for tipo_media, toggle the 'active' class so only the clicked value is active (match by the value passed), and (2) update the active-filter chip UI to use the data-label (human label) instead of the raw enum value; reference the PHP rendering of anchors (class "filter-option", $tipoFilters loop, and updateFilter function) when making these changes.app/Controllers/ScrapeController.php (1)
843-850:⚠️ Potential issue | 🟠 MajorPreserve the “no media signal” case here.
Because Line 148 and Line 218 call this helper before
normalizeIsbnFields(), unconditionally writingtipo_mediahere can make the new no-signal guard unreachable. That reopens the old path where an ambiguous barcode payload gets treated as a book and backfilled intoisbn10/isbn13.Suggested fix
private function ensureTipoMedia(array $payload): array { - $payload['tipo_media'] = \App\Support\MediaLabels::resolveTipoMedia( - $payload['format'] ?? $payload['formato'] ?? null, - $payload['tipo_media'] ?? null - ); + $format = $payload['format'] ?? $payload['formato'] ?? null; + $tipoMedia = $payload['tipo_media'] ?? null; + + if (($format === null || $format === '') && ($tipoMedia === null || $tipoMedia === '')) { + return $payload; + } + + $payload['tipo_media'] = \App\Support\MediaLabels::resolveTipoMedia($format, $tipoMedia); return $payload; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ScrapeController.php` around lines 843 - 850, ensureTipoMedia currently overwrites tipo_media unconditionally which breaks the “no media signal” guard used before normalizeIsbnFields; change it to call \App\Support\MediaLabels::resolveTipoMedia(...) into a local variable and only set $payload['tipo_media'] when the resolver returns a non-null/non-empty value AND the incoming payload did not explicitly signal “no media” (i.e., do not overwrite when tipo_media is already present as the sentinel value or when a specific no-media marker is provided); keep references to ensureTipoMedia and normalizeIsbnFields so the guard remains reachable.storage/plugins/discogs/views/settings.php (2)
23-24:⚠️ Potential issue | 🟠 MajorValidate
api_tokenbefore callingtrim().
$_POST['api_token']can be an array, andtrim()on it turns a bad submission into a 500. Guard withis_string()first and surface a validation error instead.🛡️ Suggested fix
- $apiToken = trim($_POST['api_token'] ?? ''); - if ($apiToken !== '') { - $settings = ['api_token' => $apiToken]; - if ($plugin->saveSettings($settings)) { - $successMessage = __('Impostazioni Discogs salvate correttamente.'); - } else { - $errorMessage = __('Errore nel salvataggio delle impostazioni.'); - } - } + $rawApiToken = $_POST['api_token'] ?? ''; + if (!is_string($rawApiToken)) { + $errorMessage = __('Token Discogs non valido.'); + } else { + $apiToken = trim($rawApiToken); + if ($apiToken !== '') { + $settings = ['api_token' => $apiToken]; + if ($plugin->saveSettings($settings)) { + $successMessage = __('Impostazioni Discogs salvate correttamente.'); + } else { + $errorMessage = __('Errore nel salvataggio delle impostazioni.'); + } + } + }🤖 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 23 - 24, Guard against non-string submissions before calling trim on $_POST['api_token']: check is_string($_POST['api_token']) (or use a local $raw = $_POST['api_token'] ?? null and is_string($raw)) and only call trim() when it is a string; otherwise set $apiToken to '' and add a validation error rather than allowing trim() to run on an array/object. Update the logic around $apiToken and the subsequent if ($apiToken !== '') check to handle the validation error path (surface an error message into the existing validation/response flow).
112-115:⚠️ Potential issue | 🟡 MinorKeep the toggle button's accessible name in sync with its state.
The button has an aria-label now, but it stays static after the visibility flips. Update it alongside the icon so assistive tech announces the current action.
Also applies to: 167-179
🤖 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 112 - 115, The toggle button's aria-label must be updated when visibility changes so screen readers announce the current action; modify the togglePasswordVisibility function to also find the toggle button for the given input id (e.g., target input id 'api_token' and the other instance), switch its aria-label between the localized strings (e.g., __('Mostra token') and __('Nascondi token') or equivalent) when toggling the input type, and ensure each button has an identifiable attribute (id or data-target) so the function can locate and update it; apply the same change for both occurrences referenced in the file (the api_token block and the 167-179 block).app/Views/libri/index.php (2)
314-314:⚠️ Potential issue | 🟡 MinorGive the media column an accessible header name.
This
<th>is still icon-only, so screen readers get an unlabeled column header. Add hidden text or anaria-labelon the header and mark the icon decorative.♿ Suggested fix
- <th><i class="fas fa-compact-disc text-gray-400" title="<?= __("Tipo Media") ?>"></i></th> + <th aria-label="<?= htmlspecialchars(__("Tipo Media"), ENT_QUOTES, 'UTF-8') ?>"> + <span class="sr-only"><?= __("Tipo Media") ?></span> + <i class="fas fa-compact-disc text-gray-400" + aria-hidden="true" + title="<?= htmlspecialchars(__("Tipo Media"), ENT_QUOTES, 'UTF-8') ?>"></i> + </th>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` at line 314, The header TH that currently contains only the icon <i class="fas fa-compact-disc text-gray-400" title="<?= __("Tipo Media") ?>"> is inaccessible to screen readers; update that TH to provide an accessible label by adding an aria-label (e.g., aria-label="<?= __("Tipo Media") ?>") to the <th> and marking the icon decorative (add aria-hidden="true" to the <i>), or alternatively include a visually-hidden span (sr-only/visually-hidden) with the localized text inside the <th> while keeping the icon aria-hidden; modify the <th> with the fa-compact-disc icon accordingly.
688-699:⚠️ Potential issue | 🟡 MinorReuse one localized media-label map for the column and the active filter chip.
The icon column is translated, but the selected-filter chip still shows raw enum keys like
disco/audiolibro. Build the label map once fromMediaLabels::allTypes()and reuse it in both places so the UI stays localized and consistent.Also applies to: 887-888
🤖 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 - 699, The media-labels are built inline in the DataTable column render (the local "labels" object) but the active filter chip still shows raw enum keys; instead, create a single localized label map using MediaLabels::allTypes() and expose it to the JS (e.g., emit a JSON-encoded mediaLabels variable) and then reuse that single map in both the column render (the code handling data: 'tipo_media') and in the active-filter chip rendering code (the place currently displaying raw keys around lines where the filter chip is built). Replace the inline "labels" object with a lookup into the shared mediaLabels variable (use escapeHtml when inserting labels) so both the icon tooltip/title and the filter chips show the localized labels consistently.tests/discogs-import.spec.js (2)
75-81:⚠️ Potential issue | 🟠 MajorCleanup should not hard-delete
libriafter the form save.Saving through this flow creates dependent
copierows. A rawDELETE FROM librican fail on FK constraints or leave cleanup half-done, and the emptycatch {}hides that failure.Use the app's soft-delete path or remove dependent rows first and make cleanup failures visible.
🤖 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 cleanup block in test.afterAll uses a raw DELETE via dbExec on libri (using createdId) which can fail due to FK constraints with dependent copie rows and currently swallows errors; update the cleanup to call the application's soft-delete endpoint or service (the same code path used by the app) to remove the created record, or if you must use raw SQL first delete dependent copie rows (table copie) for the libri id before deleting libri, and replace the empty catch with error handling that surfaces failures (log or rethrow) so test cleanup errors are visible; locate the test.afterAll block and the usage of createdId and dbExec to make these changes.
64-71:⚠️ Potential issue | 🟠 MajorDon't persist the shared public barcode unchanged.
This test still saves
TEST_BARCODEas the row identifier, and the setup now soft-deletes any pre-existing row with that barcode just to make the insert pass. That mutates shared data and leaves the suite exposed to uniqueness failures if a prior run died mid-flow.Overwrite the scraped
ean/isbn13with a run-scoped value before submit, then assert and clean up against that generated identifier instead.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 currently reuses the shared TEST_BARCODE and only soft-deletes existing rows via dbExec on the libri table, which mutates shared data; instead, generate a unique run-scoped barcode (e.g. runBarcode) at test setup, overwrite the scraped payload's ean/isbn13 with that runBarcode before calling the submit helper, then assert and perform cleanup (soft-delete) against runBarcode rather than TEST_BARCODE; update all occurrences that set or check ean/isbn13/TEST_BARCODE (including places around the dbExec usage and the other mentioned blocks) to use the generated run-scoped identifier.tests/discogs-plugin.spec.js (1)
204-207:⚠️ Potential issue | 🟠 MajorThis scrape branch still goes green without validating the failure path.
Once Discogs is activated, a missing import button should fail the test, not skip it. And in the empty-title branch the console listener is attached after the import and
logsis never asserted, so JS errors or silent scrape failures still pass.Also applies to: 217-244
🤖 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 204 - 207, The test currently skips when the import button is missing (locator importBtn via page.locator('#btnImportIsbn')) and attaches the console listener after triggering the import so the logs array is never asserted; change the behavior so a missing import button causes the test to fail (replace the test.skip branch with an explicit failure/throw or use test.fail) and move the console listener attachment (the logs collection) to before any import action is performed so that you can assert logs contains no JS errors (or assert expected log entries) after the import; update both the initial branch (around importBtn/isVisible) and the empty-title branch (lines ~217-244) to follow the same pattern.app/Controllers/CsvImportController.php (1)
794-797:⚠️ Potential issue | 🟠 MajorStop coercing missing
formatotocartaceoin the music import path.These paths still manufacture
formato='cartaceo'when the CSV only carriestipo_media. That means an update can silently reset an existingdiscorow back to a book format, and inserts can persist inconsistent metadata (tipo_media='disco',formato='cartaceo').Keep
formatonullable when the column is absent in CSV, preserve the current DB value on updates, and only derive a fallback in the insert path when you actually need a book default.Also applies to: 1292-1308, 1385-1408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CsvImportController.php` around lines 794 - 797, The code currently forces 'formato' => 'cartaceo' when the CSV key is missing; change the mapping in CsvImportController where 'formato' is assigned (and the associated import/update paths around the 'tipo_media' normalization) to leave 'formato' as null if the CSV doesn't include that column, so updates preserve the existing DB value and inserts do not store an inaccurate 'cartaceo' unless you explicitly need a book default; instead, apply the 'cartaceo' fallback only at the insert decision point (not in the row mapping) and ensure the update code uses the DB value when 'formato' is null.app/Controllers/LibraryThingImportController.php (1)
1348-1351:⚠️ Potential issue | 🟠 MajorDon't force non-book LibraryThing imports back to
cartaceo.These branches still default a missing
formatoto'cartaceo'even whentipo_mediaresolves to something likedisco. That can overwrite an existing music item on update and store inconsistent data on insert.Preserve
formatowhen the source leaves it empty, and only apply the book fallback in the insert path when you truly have no non-book media type to honor.Also applies to: 1423-1426, 1536-1540, 1615-1619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1348 - 1351, The current LibraryThingImportController logic unconditionally defaults a missing $data['formato'] to 'cartaceo', which can overwrite non-book media when $hasTipoMedia is true; update the code paths around where $params[] is set (the blocks using $data['formato'], $hasTipoMedia and \App\Support\MediaLabels::normalizeTipoMedia($data['tipo_media'] ?? null)) so that you only apply the 'cartaceo' fallback during insert when there is no non-book tipo_media to honor, and otherwise preserve an empty/absent formato (i.e., if tipo_media resolves to a non-book value like 'disco' do not substitute 'cartaceo' and on update do not overwrite existing formato when the source leaves it empty). Ensure the same change is applied to the other similar spots (the other instances noted) so inserts get the book default only when appropriate and updates preserve source-empty formato.storage/plugins/discogs/plugin.json (1)
10-12:⚠️ Potential issue | 🟠 MajorThese app-version constraints still aren't enforced.
PluginManagervalidatesrequires_php, but it still doesn't checkrequires_appormax_app_versionon install/activation. Right now this manifest advertises a compatibility gate that the runtime won't enforce.🤖 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, PluginManager currently only validates requires_php but ignores requires_app and max_app_version; update the install/activation flow in PluginManager (e.g., methods installPlugin and activatePlugin) to read the manifest fields requires_app and max_app_version from the plugin manifest and validate them against the current app version using a semantic version comparison (use the existing semver util or add one) so installation/activation fails with a clear error when current app version is lower than requires_app or greater than max_app_version; include informative error messages referencing the plugin id/name and the offending manifest field.app/Views/frontend/book-detail.php (1)
2254-2255:⚠️ Potential issue | 🟡 MinorTreat blank related authors as missing too.
??only falls back onnull; an empty or whitespace-onlyautoristill renders a blank line instead of the music-aware fallback label.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` around lines 2254 - 2255, The current output uses $related['autori'] with null coalescing which doesn't treat empty or whitespace-only strings as missing; update the logic to treat trimmed-empty values as missing by checking trim($related['autori'] ?? '') and falling back to the music-aware label ($relatedIsMusic ? 'Artista sconosciuto' : 'Autore sconosciuto') before passing the final string into htmlspecialchars; reference the $related['autori'] lookup and the $relatedIsMusic flag so you locate and replace the inline expression in the book-detail view.tests/multisource-scraping.spec.js (1)
131-133:⚠️ Potential issue | 🟠 MajorClean up the records this suite creates in
afterAll.The suite keeps the inserted IDs, but
afterAll()still only closes the context. On any reused E2E database this leavesRUN_TAGrows behind, which can collide with later runs and make the catalog assertions flaky.🤖 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 131 - 133, The afterAll hook only closes context but must also delete test data: update test.afterAll in multisource-scraping.spec.js to first delete all records created by the suite (use the suite-scoped variables that hold inserted IDs and the RUN_TAG constant) via the same DB client used in tests, remove rows matching those inserted IDs and/or RUN_TAG, await that deletion, then finally await context?.close(); ensure any promises are awaited and errors are caught/logged so cleanup always runs.
🧹 Nitpick comments (3)
tests/smoke-install.spec.js (1)
101-101: Consider consistent formatting for readability.The
test.skip()call is valid here, but placing it on the same line as the arrow function opening makes the test harder to scan. Line 93 shows the cleaner pattern withtest.skip()on its own line.✨ Suggested formatting (applies to lines 101, 109, 141, 150, 162, 169, 178)
- test('Installer step 1: verify requirements and start', async () => { test.skip(!installerAvailable, 'App already installed'); + test('Installer step 1: verify requirements and start', async () => { + test.skip(!installerAvailable, 'App already installed'); // All requirements should be met🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke-install.spec.js` at line 101, The test declarations use inline test.skip calls that reduce readability; for each affected test (e.g., the 'Installer step 1: verify requirements and start' test and the others at the mentioned locations) move the test.skip(!installerAvailable, 'App already installed') onto its own line immediately before the test(...) call so it matches the cleaner pattern used at line 93, ensuring consistent formatting across tests and improving scanability.installer/classes/Installer.php (1)
649-649: Consider dropping the legacyidx_libri_deleted_atduring upgrades.At Line 649, the new composite index is added, but previous installs may still keep the old single-column index. Keeping both adds unnecessary write overhead on
libri.Proposed migration-safe cleanup idea
+ $legacyIndexesToDrop = [ + 'libri' => ['idx_libri_deleted_at'], + ]; ... foreach ($indexes as $table => $tableIndexes) { + // Drop obsolete indexes (best effort) + foreach (($legacyIndexesToDrop[$table] ?? []) as $legacyIndex) { + if (isset($existingIndexes[$legacyIndex])) { + try { + $pdo->exec("ALTER TABLE `{$table}` DROP INDEX `{$legacyIndex}`"); + unset($existingIndexes[$legacyIndex]); + } catch (\PDOException $e) { + // ignore and continue + } + } + } + // Create missing indexes foreach ($tableIndexes as $indexName => $columns) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/classes/Installer.php` at line 649, In the Installer class, after creating the new composite index 'idx_libri_tipo_media_deleted_at' add a migration-safe step to remove the legacy single-column index 'idx_libri_deleted_at' if it exists: detect its presence (via an information_schema query or use your existing schema helper), and issue a conditional DROP INDEX/ALTER TABLE ... DROP INDEX or equivalent within the same upgrade path so upgrades remove the old index without error on fresh installs; update the method that adds indexes (where 'idx_libri_tipo_media_deleted_at' is defined) to perform this check-and-drop operation.storage/plugins/discogs/wrapper.php (1)
32-37: Preferis_callable()overmethod_exists()for these proxy forwards.
method_exists()is true for protected/private methods too, so this wrapper can still hit a fatal call if the inner plugin ever changes visibility on one of these hooks.is_callable()matches what the proxy can actually invoke.Additionally,
setPluginId()on line 88-90 is called directly without any guard—consider adding the same safety check as the optional lifecycle methods.♻️ Suggested hardening
- if (method_exists($this->instance, 'activate')) { + if (is_callable([$this->instance, 'activate'])) { $this->instance->activate(); } @@ - if (method_exists($this->instance, 'onDeactivate')) { + if (is_callable([$this->instance, 'onDeactivate'])) { $this->instance->onDeactivate(); } @@ - if (method_exists($this->instance, 'onInstall')) { + if (is_callable([$this->instance, 'onInstall'])) { $this->instance->onInstall(); } @@ - if (method_exists($this->instance, 'onActivate')) { + if (is_callable([$this->instance, 'onActivate'])) { $this->instance->onActivate(); - } elseif (method_exists($this->instance, 'activate')) { + } elseif (is_callable([$this->instance, 'activate'])) { $this->instance->activate(); } @@ - if (method_exists($this->instance, 'onUninstall')) { + if (is_callable([$this->instance, 'onUninstall'])) { $this->instance->onUninstall(); } @@ - $this->instance->setPluginId($pluginId); + if (!is_callable([$this->instance, 'setPluginId'])) { + throw new \BadMethodCallException('setPluginId is not callable'); + } + $this->instance->setPluginId($pluginId); @@ - if (method_exists($this->instance, $method)) { + if (is_callable([$this->instance, $method])) { return call_user_func_array([$this->instance, $method], $args); }Applies to lines: 32-37, 42-48, 53-59, 64-72, 77-83, 88-90, 96-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/discogs/wrapper.php` around lines 32 - 37, The wrapper uses method_exists() for proxying lifecycle hooks (e.g., activate(), deactivate(), init(), uninstall(), getMetadata()) which can return true for non-callable (protected/private) methods and risk fatal errors; replace those checks with is_callable([$this->instance, 'activate']) etc. and apply the same guard to setPluginId() (currently called unguarded) so it only calls setPluginId() when is_callable([$this->instance, 'setPluginId']). Update all proxy forwards in this class to use is_callable with the instance and method names to ensure the inner plugin method is actually invocable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bb301e9-a960-438a-b584-28be06a50e80
📒 Files selected for processing (52)
.coderabbit.yaml.gitattributes.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/Routes/web.phpapp/Support/BundledPlugins.phpapp/Support/MediaLabels.phpapp/Support/PluginManager.phpapp/Support/RouteTranslator.phpapp/Support/Updater.phpapp/Views/admin/plugins.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.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.jsonstorage/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.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
+55
to
+63
| public function validateBarcode(bool $isValid, string $isbn): bool | ||
| { | ||
| // If already valid (ISBN), keep it | ||
| if ($isValid) { | ||
| return true; | ||
| } | ||
| // Accept EAN-13 (13 digits) and UPC-A (12 digits) | ||
| $clean = preg_replace('/[^0-9]/', '', $isbn); | ||
| return strlen((string) $clean) === 13 || strlen((string) $clean) === 12; |
There was a problem hiding this comment.
Validate UPC/EAN checksums instead of length only.
Any 12/13-digit string currently passes this hook, which means junk input still triggers Discogs/MusicBrainz lookups and burns rate-limit budget. This validator should verify the UPC-A / EAN-13 check digit before accepting the identifier.
🤖 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 current
validateBarcode method (validateBarcode) only checks digit length and accepts
any 12/13-digit string; change it to validate UPC‑A and EAN‑13 check digits:
keep the existing cleaning step (preg_replace into $clean), then if
strlen($clean) === 12 compute the UPC‑A check digit from the first 11 digits
using the standard odd*3 + even*1 sum mod 10 rule and compare to the 12th digit,
returning true only on match; if strlen($clean) === 13 compute the EAN‑13 check
digit from the first 12 digits using the same weighting (odd/even) rule and
compare to the 13th digit; otherwise return false. Ensure validateBarcode still
returns a boolean and uses $isbn only to produce $clean.
Comment on lines
+123
to
+150
| // Test 3: Schema.org uses MusicAlbum for disco, Book for libro | ||
| // ═══════════════════════════════════════════════════════════════════ | ||
| test('3. Schema.org JSON-LD type is MusicAlbum for disco', async () => { | ||
| const musicResp = await page.request.get(`${BASE}/libro/${musicBookId}`); | ||
| expect(musicResp.status()).toBe(200); | ||
| const musicHtml = await musicResp.text(); | ||
|
|
||
| const jsonLdBlocks = Array.from( | ||
| musicHtml.matchAll(/<script[^>]*type=["']application\/ld\+json["'][^>]*>([\s\S]*?)<\/script>/gi), | ||
| (match) => match[1] | ||
| ); | ||
| const schemas = jsonLdBlocks.flatMap((block) => { | ||
| try { | ||
| const parsed = JSON.parse(block.trim()); | ||
| return Array.isArray(parsed) ? parsed : [parsed]; | ||
| } catch { | ||
| return []; | ||
| } | ||
| }); | ||
|
|
||
| const musicSchema = schemas.find((schema) => schema && schema['@type'] === 'MusicAlbum'); | ||
| expect(musicSchema, 'Frontend JSON-LD is missing MusicAlbum for disco').toBeTruthy(); | ||
|
|
||
| const tipoMedia = dbQuery(`SELECT tipo_media FROM libri WHERE id = ${musicBookId}`); | ||
| expect(tipoMedia).toBe('disco'); | ||
|
|
||
| const bookTipoMedia = dbQuery(`SELECT tipo_media FROM libri WHERE id = ${bookBookId}`); | ||
| expect(bookTipoMedia).toBe('libro'); |
There was a problem hiding this comment.
Also assert the Book JSON-LD on the comparison record.
This test now proves the music page emits MusicAlbum, but the libro side is still only verified through DB state. A frontend regression that renders the regular book with 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 123 - 150, The test '3.
Schema.org JSON-LD type is MusicAlbum for disco' only asserts the MusicAlbum
JSON-LD on the music page and checks the libro record via dbQuery; update the
test to also fetch and parse the frontend JSON‑LD from the book page (use
page.request.get(`${BASE}/libro/${bookBookId}`) to produce bookHtml, reuse the
jsonLdBlocks/schemas parsing logic to find a schema with '@type' === 'Book' and
assert it is present (e.g., bookSchema truthy) so the frontend output for the
libro side is verified in addition to the DB check; reference existing
helpers/variables musicResp, musicHtml, jsonLdBlocks, schemas, dbQuery,
musicSchema, bookBookId to locate where to add this fetch-and-assert.
Critical: - MediaLabels::formatTracklist — sanitize pre-formatted <ol> HTML with strip_tags whitelist instead of returning raw (CWE-79 defense in depth) - migrate_0.5.4.sql — guard backfill UPDATEs with @formato_exists check (migration aborts on installs without formato column) Major: - PluginManager::installPlugin — rollback DB row + files on onInstall() failure (half-installed ghost plugins) - PluginManager::setSetting — try/catch/finally preserves bool contract when encryptPluginSettingValue throws (statement leak fix) - ScrapeController::ensureTipoMedia — preserve no-signal case so normalizeIsbnFields guard remains reachable - BookRepository — skip non-string tipo_media on update (array/null silently normalized to 'libro' = data corruption) - LibriController — remove seeded tipo_media='' from defaults array - CsvImportController + LibraryThingImportController — stop coercing formato='cartaceo' for non-book media (guard on resolved tipo_media) - DiscogsPlugin — MusicBrainz fallback when Discogs detail fetch fails (was returning $currentResult, skipping advertised fallback chain) - discogs/wrapper.php — is_callable() replaces method_exists() on all proxy forwards (method_exists true for private/protected = fatal) Minor: - catalog-grid.php + libri/index.php — aria-label + aria-hidden on media badge/column for screen reader accessibility - de_DE.json — removed duplicate "Disattivato" key - discogs/views/settings.php — is_string guard on $_POST['api_token'] - .coderabbit.yaml — fix 3 schema violations (path_filters format, unsupported phpstan.config_file, unsupported learnings.enabled) Tests: - pr100-review-fixes.spec.js — env skip moved to lifecycle describe only (offline metadata tests run without credentials) - smoke-install.spec.js — test.skip formatting on own line - multisource-scraping.spec.js — afterAll cleanup of created records - seed-catalog.spec.js — assert URL instead of swallowing failures
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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