feat: hreflang tags, RSS feed, sitemap events#92
Open
fabiodalez-dev wants to merge 11 commits intomainfrom
Open
feat: hreflang tags, RSS feed, sitemap events#92fabiodalez-dev wants to merge 11 commits intomainfrom
fabiodalez-dev wants to merge 11 commits intomainfrom
Conversation
Add multi-locale hreflang alternate links to all frontend pages via HreflangHelper, which reverse-maps the current URL to a route key and translates it for every active locale. Works dynamically with any number of installed languages. Add RSS 2.0 feed at /feed.xml with the latest 50 books, including autodiscovery link in layout head and Feed directive in robots.txt. Expand sitemap with event entries (from events table) and feed.xml as a static entry. Add RSS icon link in the footer. Include 19 E2E tests covering hreflang, RSS feed, sitemap, and robots.txt.
|
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 an RSS feed endpoint (/feed.xml) and FeedController to emit RSS 2.0 from recent books, HreflangHelper for per-locale alternate URLs, sitemap/robots updates (feed + events), llms.txt support and route, UI/layout changes (hreflang links + RSS icon), settings wiring, translations, and new E2E tests covering these features. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FeedController.php`:
- Around line 22-32: Replace the dynamic gmdate('r') stamp with a stable
lastBuildDate derived from the latest item timestamp returned by
getLatestBooks($db, $baseUrl): inspect the $items array (or query the latest
matching row) to find the newest publication/updated timestamp, format that
timestamp with gmdate('r') and use it for the <lastBuildDate> value; only use
current time as a fallback when $items is empty or no timestamps exist so
unchanged feeds keep a consistent lastBuildDate.
In `@app/Support/HreflangHelper.php`:
- Around line 45-52: The code incorrectly truncates locales via substr(..., 0,
2) to build $prefix and hreflang, causing collisions for locales that share the
same language; update the logic in the foreach over $locales (and the other
occurrences around the same helper) to use the full locale key (e.g. en_US) and
a configured locale->prefix map instead of deriving a two-letter prefix: replace
usages of strtolower(substr($localeCode, 0, 2)) with a lookup like $prefix =
$localePrefixMap[$localeCode] (falling back to the full locale token if
missing), set $currentLocale = $localeCode (not truncated) and emit hreflang
using the proper tag format (convert underscore to hyphen, e.g. en_US -> en-US);
apply the same fix in the blocks referenced (lines ~79-92 and ~97-99) so
prefixes and emitted hreflang values remain unique per installed locale.
In `@tests/seo-feed.spec.js`:
- Around line 11-19: Add an assertion to verify successful HTTP status before
parsing HTML: in the test "homepage has IT, EN, and x-default hreflang links"
(and similarly in the tests covering lines 21-34, 36-48, 50-78, 79-100) call
expect(resp.status()).toBe(200) immediately after obtaining resp from
request.get(`${BASE}/`) (and for the book page request referenced around the
book test at Line 63) before calling resp.text() and performing hreflang
assertions so the test fails on non-200 responses instead of falsely passing on
error pages.
- Around line 102-112: The test labeled "single-locale site emits no hreflang
tags" is running against the multi-locale fixture and therefore cannot validate
the single-locale behavior; update the test to actually exercise a single-locale
app instance (or rename/remove it). Specifically, modify the test harness used
by this spec so that for the test function 'test("single-locale site emits no
hreflang tags", ...)' the app is started/configured with only one active locale
(so request.get(`${BASE}/catalogo`) returns HTML for a single-locale site) and
then assert that hreflangCount is 0 (or alternatively rename the test to reflect
that it checks multi-locale behavior if you choose to remove the single-locale
scenario). Ensure changes reference this test name and the hreflangCount logic
so reviewers can see the corrected expectation or fixture change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90dfa7ef-313c-4095-880a-09973c91c038
📒 Files selected for processing (7)
app/Controllers/FeedController.phpapp/Controllers/SeoController.phpapp/Routes/web.phpapp/Support/HreflangHelper.phpapp/Support/SitemapGenerator.phpapp/Views/frontend/layout.phptests/seo-feed.spec.js
- Use stable lastBuildDate from latest item timestamp instead of
dynamic gmdate('r') on every request (FeedController)
- Add expect(resp.status()).toBe(200) assertions to all hreflang
and feed tests to catch non-200 responses early
- Rename misleading "single-locale" test to accurately describe
multi-locale hreflang count verification
- Fix EN catalog test that was hitting a non-routable /en/ prefix
URL; now verifies hreflang content from the IT page instead
- Add new test: lastBuildDate stability across consecutive requests
Owner
Author
CodeRabbit findings addressedFixed (commit 1e5d8fc):
Not fixing:HreflangHelper locale truncation (en_US → en) — This is a system-wide architectural pattern, not an HreflangHelper bug. The entire routing layer ( @coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Controllers/FeedController.php (1)
131-139: Consider consolidating identical escape methods.
xmlEscape()andxmlAttrEscape()have identical implementations. While having semantically distinct methods is reasonable for future-proofing, you could simplify by having one delegate to the other or extracting a shared private method.♻️ Optional: Consolidate escape methods
private function xmlEscape(string $text): string { return htmlspecialchars($text, ENT_XML1 | ENT_QUOTES, 'UTF-8'); } private function xmlAttrEscape(string $text): string { - return htmlspecialchars($text, ENT_XML1 | ENT_QUOTES, 'UTF-8'); + return $this->xmlEscape($text); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FeedController.php` around lines 131 - 139, Both xmlEscape() and xmlAttrEscape() use the exact same htmlspecialchars call; consolidate by having one delegate to the other (e.g., implement xmlEscape(string $text): string as the single implementation and have xmlAttrEscape(string $text): string simply return xmlEscape($text)), or extract a shared private helper (e.g., escapeForXml) invoked by both; update all callers to use either method as appropriate and keep the semantic names for clarity.
🤖 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/seo-feed.spec.js`:
- Around line 188-203: The test 'lastBuildDate is stable and derived from latest
item' makes two GET requests (resp1 and resp2) but only asserts resp1.status();
add an assertion for the second response by calling
expect(resp2.status()).toBe(200) immediately after obtaining resp2 (before
calling resp2.text()) so the test consistently verifies both HTTP responses.
---
Nitpick comments:
In `@app/Controllers/FeedController.php`:
- Around line 131-139: Both xmlEscape() and xmlAttrEscape() use the exact same
htmlspecialchars call; consolidate by having one delegate to the other (e.g.,
implement xmlEscape(string $text): string as the single implementation and have
xmlAttrEscape(string $text): string simply return xmlEscape($text)), or extract
a shared private helper (e.g., escapeForXml) invoked by both; update all callers
to use either method as appropriate and keep the semantic names for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa9b782a-6fd2-4eb5-8d91-b7c04592b911
📒 Files selected for processing (2)
app/Controllers/FeedController.phptests/seo-feed.spec.js
|
The architectural rationale for leaving locale truncation as-is is well-reasoned. Since Resolving all open review comments now. ✏️ Learnings added
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
- xmlAttrEscape now delegates to xmlEscape (identical implementation) - Add expect(resp2.status()).toBe(200) for second request in lastBuildDate stability test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/seo-feed.spec.js (1)
82-94: x-default test is coupled to IT being the default locale.
x-defaultis derived from installation default locale. Hardcoding IT makes this test brittle across environments.🔧 Suggested fix
- test('x-default points to the default (IT) locale version', async ({ request }) => { + test('x-default points to one of the active locale versions', async ({ request }) => { const resp = await request.get(`${BASE}/catalogo`); expect(resp.status()).toBe(200); const html = await resp.text(); const xDefaultMatch = html.match(/hreflang="x-default"[^>]*href="([^"]+)"/); - const itMatch = html.match(/hreflang="it"[^>]*href="([^"]+)"/); expect(xDefaultMatch).not.toBeNull(); - expect(itMatch).not.toBeNull(); + const localeMatches = [...html.matchAll(/hreflang="(?!x-default)[^"]+"[^>]*href="([^"]+)"/g)]; + expect(localeMatches.length).toBeGreaterThan(0); - // x-default should be identical to IT version - expect(xDefaultMatch[1]).toBe(itMatch[1]); + // x-default should match one of the active locale URLs + const localeHrefs = localeMatches.map((m) => m[1]); + expect(localeHrefs).toContain(xDefaultMatch[1]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/seo-feed.spec.js` around lines 82 - 94, The test currently assumes IT is the default by matching hreflang="it"; instead, change the assertion to discover the locale link that matches the x-default href dynamically: after extracting xDefaultMatch from the HTML, scan all hreflang links (e.g., via regex matching /hreflang="([^"]+)"[^>]*href="([^"]+)"/) and assert there exists an entry whose href equals xDefaultMatch[1] (store matches in an array and find by href) rather than specifically using itMatch; update the test name/expectations in the 'x-default points to the default (IT) locale version' test accordingly so the test no longer hardcodes 'it' and will pass regardless of the installation default.
🤖 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/seo-feed.spec.js`:
- Around line 96-105: The test "hreflang links are absolute URLs" currently
iterates over matchAll results but never asserts that any matches exist, so the
check can pass vacuously; change the code to collect matches into an array
(e.g., const matches =
[...html.matchAll(/hreflang="[^"]*"[^>]*href="([^"]+)"/g)]), assert
matches.length > 0 with expect(matches.length).toBeGreaterThan(0), then iterate
over matches and validate each match[1] with toMatch(/^https?:\/\//); make the
identical change for the other test referenced (lines 166-176) that uses
hrefMatches.
- Around line 237-250: The test "sitemap includes event URLs when events exist"
currently sets hasEvents = xml.includes('/eventi/') || xml.includes('/events/')
and then asserts the same condition, which is tautological; change the assertion
so that when hasEvents is true you explicitly assert both locale variants are
present (i.e., expect(xml.includes('/eventi/')).toBe(true) and
expect(xml.includes('/events/')).toBe(true) or equivalent), updating the test
body around the hasEvents check and using the xml variable and test name to
locate the code.
- Around line 72-80: The test only asserts enHref contains '/en/' but doesn't
verify the slug path is identical; update the assertions in
tests/seo-feed.spec.js (around variables itHref and enHref) to extract the
localized slug portion from each href (e.g., capture the path segment after the
locale prefix) and assert those slug parts are equal — for example, strip the
leading '/it/' and '/en/' (or use a regex capture for the path after the locale)
and compare the resulting strings so the test fails if localized slug paths
diverge.
---
Nitpick comments:
In `@tests/seo-feed.spec.js`:
- Around line 82-94: The test currently assumes IT is the default by matching
hreflang="it"; instead, change the assertion to discover the locale link that
matches the x-default href dynamically: after extracting xDefaultMatch from the
HTML, scan all hreflang links (e.g., via regex matching
/hreflang="([^"]+)"[^>]*href="([^"]+)"/) and assert there exists an entry whose
href equals xDefaultMatch[1] (store matches in an array and find by href) rather
than specifically using itMatch; update the test name/expectations in the
'x-default points to the default (IT) locale version' test accordingly so the
test no longer hardcodes 'it' and will pass regardless of the installation
default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ec5b1c5-2375-4ce0-8bca-34772a0a8a2c
📒 Files selected for processing (2)
app/Controllers/FeedController.phptests/seo-feed.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Controllers/FeedController.php
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- x-default test: dynamically verify it matches one of the active locale URLs instead of hardcoding IT - Book slug test: compare path after stripping locale prefix to verify slugs are actually identical across locales - Absolute URL tests: assert at least one match exists before iterating (prevents vacuous pass on zero matches) - Event sitemap test: assert both IT and EN variants independently instead of tautological re-check of the same condition
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Support/SitemapGenerator.php (1)
160-173:⚠️ Potential issue | 🟠 MajorFeed sitemap entry is being locale-prefixed, but the route is global.
At Line 160,
'/feed.xml'is emitted inside the locale loop, so non-default locales generate URLs like/en/feed.xml. Inapp/Routes/web.php, Line 142 registers only/feed.xml. This can produce sitemap URLs that are not served.💡 Proposed fix
@@ - ['path' => '/feed.xml', 'changefreq' => 'daily', 'priority' => '0.3'], ]; @@ foreach ($this->activeLocales as $locale) { $localePrefix = $this->getLocalePrefix($locale); @@ $entries[] = [ 'loc' => $this->baseUrl . $localePrefix . $path, 'changefreq' => $page['changefreq'], 'priority' => $page['priority'], ]; } } + + // Feed endpoint is global (/feed.xml), not locale-prefixed. + $entries[] = [ + 'loc' => $this->baseUrl . '/feed.xml', + 'changefreq' => 'daily', + 'priority' => '0.3', + ];
🧹 Nitpick comments (2)
app/Support/HreflangHelper.php (1)
58-59: Remove the unusedbuildReverseMapparameter.
$localesis passed and declared but not used, which adds noise and triggered static analysis.♻️ Proposed cleanup
- $reverseMap = self::buildReverseMap($locales, $currentLocale); + $reverseMap = self::buildReverseMap($currentLocale); @@ - private static function buildReverseMap(array $locales, string $currentLocale): array + private static function buildReverseMap(string $currentLocale): arrayAlso applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/HreflangHelper.php` around lines 58 - 59, The method buildReverseMap currently declares and callers pass an unused $locales parameter (e.g. self::buildReverseMap($locales, $currentLocale)); remove the unused parameter by updating the buildReverseMap declaration to accept only $currentLocale and update all invocations to call self::buildReverseMap($currentLocale) (or the equivalent single-argument call) so the signature and callers are consistent and the unused variable is eliminated.tests/seo-feed.spec.js (1)
38-49: Consider consolidating with the test on lines 22-36.This test fetches the same endpoint (
/catalogo) and performs overlapping assertions with the "catalog page hreflang translates route correctly" test. The only additional assertion is line 48 (not.toContain('/catalogo')).♻️ Suggested consolidation
test('catalog page hreflang translates route correctly', async ({ request }) => { const resp = await request.get(`${BASE}/catalogo`); expect(resp.status()).toBe(200); const html = await resp.text(); // IT version should point to /catalogo const itMatch = html.match(/hreflang="it"[^>]*href="([^"]+)"/); expect(itMatch).not.toBeNull(); expect(itMatch[1]).toContain('/catalogo'); // EN version should point to /en/catalog const enMatch = html.match(/hreflang="en"[^>]*href="([^"]+)"/); expect(enMatch).not.toBeNull(); expect(enMatch[1]).toContain('/en/catalog'); + // EN should NOT contain the IT route + expect(enMatch[1]).not.toContain('/catalogo'); }); - test('hreflang EN variant uses the correct English translated route', async ({ request }) => { - ... - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/seo-feed.spec.js` around lines 38 - 49, The new test "hreflang EN variant uses the correct English translated route" duplicates assertions already covered by the existing "catalog page hreflang translates route correctly" test; consolidate by removing the duplicate test or merging its extra assertion (expect(enMatch[1]).not.toContain('/catalogo')) into the original test so all hreflang checks for /catalogo live in one place (locate the tests by their names in tests/seo-feed.spec.js and update the remaining test to include the negative assertion about not containing '/catalogo').
🤖 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/FeedController.php`:
- Around line 109-117: The pubDate variable may remain empty when created_at is
missing; before attempting to parse, initialize $pubDate to a safe default
(e.g., gmdate('r')), then if created_at exists try to parse with new
\DateTimeImmutable((string)$row['created_at']) and overwrite $pubDate with
$dt->format('r') inside the try block and fall back to the default in the catch;
update the code paths around the $pubDate assignment in FeedController (the
block using $row['created_at'], DateTimeImmutable, and gmdate('r')) so <pubDate>
is never emitted as an empty string.
---
Nitpick comments:
In `@app/Support/HreflangHelper.php`:
- Around line 58-59: The method buildReverseMap currently declares and callers
pass an unused $locales parameter (e.g. self::buildReverseMap($locales,
$currentLocale)); remove the unused parameter by updating the buildReverseMap
declaration to accept only $currentLocale and update all invocations to call
self::buildReverseMap($currentLocale) (or the equivalent single-argument call)
so the signature and callers are consistent and the unused variable is
eliminated.
In `@tests/seo-feed.spec.js`:
- Around line 38-49: The new test "hreflang EN variant uses the correct English
translated route" duplicates assertions already covered by the existing "catalog
page hreflang translates route correctly" test; consolidate by removing the
duplicate test or merging its extra assertion
(expect(enMatch[1]).not.toContain('/catalogo')) into the original test so all
hreflang checks for /catalogo live in one place (locate the tests by their names
in tests/seo-feed.spec.js and update the remaining test to include the negative
assertion about not containing '/catalogo').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d5ad5e8-3d30-49a6-a449-13730b52fac6
📒 Files selected for processing (7)
app/Controllers/FeedController.phpapp/Controllers/SeoController.phpapp/Routes/web.phpapp/Support/HreflangHelper.phpapp/Support/SitemapGenerator.phpapp/Views/frontend/layout.phptests/seo-feed.spec.js
Owner
Author
|
All 4 findings from this review were already addressed in commit f882111 (pushed before this review completed):
@coderabbitai resolve |
|
Resolving all open review comments now. 🧠 Learnings used✅ Actions performedComments resolved. Auto-approval is disabled; enable |
- Move feed.xml outside locale loop in SitemapGenerator (global endpoint)
- Remove unused $locales param from HreflangHelper::buildReverseMap()
- Default pubDate to gmdate('r') in FeedController instead of empty string
- Consolidate duplicate EN hreflang test into catalog route test
- Update sitemap test: assert single global feed.xml (not locale-prefixed)
Owner
Author
|
All four findings from the full review have been addressed in 5db1fb1:
PHPStan 0 errors, 19/19 E2E tests pass. @coderabbitai full review |
|
Triggering a full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
- Add hreflang, RSS feed, sitemap expansion, RSS icon to v0.4.9.9 section - Update OPAC feature description with hreflang and RSS - Add FeedController and HreflangHelper to developer paths
- Add seo.llms_txt_enabled toggle in Settings > Advanced tab - Add llmsTxt() method in SeoController generating structured markdown per llms.txt spec (H1, blockquote summary, sections with links) - Register GET /llms.txt route; returns 404 page when disabled - Add llms.txt directive to robots.txt when enabled - Add seo category mapping in ConfigStore::loadDatabaseSettings() - All strings use __() for i18n; translations added for EN and DE - Add 6 E2E tests in seo-feed.spec.js covering format, content, and robots.txt integration
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
locale/de_DE.json (1)
4064-4064: Remove duplicated translation key to avoid ambiguous JSON resolution.Line 4064 (
"Informazioni di contatto") duplicates an existing key already present at Line 1870. In JSON objects, duplicate keys are ambiguous and typically resolve to the last occurrence, which makes future maintenance error-prone.♻️ Suggested cleanup
- "Informazioni di contatto": "Kontaktinformationen",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/de_DE.json` at line 4064, Remove the duplicate JSON key "Informazioni di contatto" that appears twice (once at the earlier occurrence and again in the diff) to avoid ambiguous resolution; keep a single entry mapping "Informazioni di contatto" to "Kontaktinformationen" (delete the duplicated line from the diff) or consolidate any differing translations into that one canonical key; ensure the resulting locale JSON has only one "Informazioni di contatto" key.app/Support/HreflangHelper.php (1)
45-50: Centralize the locale-prefix rule.This helper hardcodes the
substr($localeCode, 0, 2)rule twice, andapp/Support/SitemapGenerator.php::getLocalePrefix()repeats it again. Pulling that into one shared utility will keep hreflang and sitemap generation aligned.Based on learnings, all locale handling uses 2-letter prefixes derived via
substr($locale, 0, 2).Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/HreflangHelper.php` around lines 45 - 50, Centralize the 2-letter locale-prefix rule by adding a single utility (e.g., a static method getLocalePrefix(string $locale): string on a new or existing Support/LocaleHelper or Locale class) that returns '/' . strtolower(substr($locale, 0, 2)); then replace the inline uses in HreflangHelper (the foreach using substr($localeCode, 0, 2) and the later occurrence around lines 79-80) and in SitemapGenerator::getLocalePrefix() to call LocaleHelper::getLocalePrefix($locale) instead so all code reuses the same implementation.app/Controllers/SettingsController.php (1)
626-629: Keepseo.llms_txt_enabledin one runtime type.This writes a boolean into
ConfigStore, butapp/Controllers/SeoController.phpreads the flag with=== '1'. The toggle now behaves differently depending on whether it came from the DB or the in-memory config cache.♻️ Proposed fix
- ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled === '1'); + ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SettingsController.php` around lines 626 - 629, The runtime type for seo.llms_txt_enabled is inconsistent: SettingsController currently writes a string to the DB via $repository->set('seo','llms_txt_enabled',$llmsTxtEnabled) but writes a boolean into ConfigStore via ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled === '1'), while SeoController expects a string comparison (=== '1'). Fix by making the in-memory config use the same string values as the DB: set ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled) (or alternatively change SeoController to normalize to boolean everywhere), ensuring all reads/writes use the same type for the llms_txt_enabled flag.app/Controllers/SeoController.php (1)
59-80: Drop the dead locale plumbing fromllmsTxt().
$locale,$code, and thegenressubquery are unused. They keep PHPMD warnings alive and add an unnecessary count query to everyllms.txtrequest.♻️ Proposed cleanup
- $locale = I18n::getInstallationLocale(); $locales = I18n::getAvailableLocales(); @@ - (SELECT COUNT(*) FROM generi) AS genres, (SELECT COUNT(*) FROM events WHERE is_active = 1) AS events" ); @@ - foreach ($locales as $code => $name) { + foreach ($locales as $name) { $langNames[] = $name; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SeoController.php` around lines 59 - 80, The llmsTxt() function contains dead locale plumbing and an unnecessary genres count: remove the unused assignment $locale = I18n::getInstallationLocale(); remove the genres subquery from the stats SQL and any extraction of $row['genres'], and stop using the unused loop key $code by iterating values only (e.g., foreach ($locales as $name) { $langNames[] = $name; }) so PHPMD warnings are gone and the extra COUNT query is removed; keep the remaining COUNTs (books, authors, publishers, events) and existing $langNames population intact.
🤖 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/SitemapGenerator.php`:
- Around line 226-255: The getEventEntries method currently adds event URLs
regardless of the site-wide events visibility; before querying/iterating, gate
the logic by checking the global events visibility flag the frontend uses (e.g.
call the existing config/settings accessor or method used elsewhere such as
isSectionEnabled('events') or $this->settings['events_enabled']), and return an
empty array if events are disabled; ensure you add this check at the top of
getEventEntries so the RouteTranslator::getRouteForLocale and
$this->activeLocales loop only run when the global events section is enabled.
In `@app/Views/settings/advanced-tab.php`:
- Around line 338-352: The checkbox with id/name llms_txt_enabled is unlabeled
for assistive tech because the visible text is outside the <label>; give it an
accessible name by adding an id to the visible label text (the span containing
__("Abilita llms.txt")) and referencing that id from the input via
aria-labelledby="that-id" (or alternatively move/wrap the visible text inside a
<label for="llms_txt_enabled">). Ensure you update the span (e.g.,
id="llms_txt_label") and set aria-labelledby="llms_txt_label" on the input
llms_txt_enabled so screen readers correctly announce the control.
In `@locale/en_US.json`:
- Around line 4063-4064: The file contains a duplicate translation key
"Informazioni di contatto" which overrides the earlier definition; remove this
later entry (the "Informazioni di contatto": "Contact information" pair) and, if
you intended to change the wording, update the existing earlier "Informazioni di
contatto" entry instead of adding a duplicate so only one mapping remains for
that key.
In `@tests/seo-feed.spec.js`:
- Around line 68-72: The test's stripLocalePrefix currently uses new
URL(href).pathname which still contains the app base path so on subfolder
installs the regex /^\/[a-z]{2}(?=\/)/ doesn't match; update stripLocalePrefix
(used for enHref and itHref comparisons) to first remove the configured BASE
prefix (e.g. strip BASE or use the app's base constant) from new
URL(href).pathname and then apply the locale-prefix regex so the returned path
is normalized relative to BASE before comparing enHref[1] and itHref[1].
- Around line 225-233: The current assertion uses toContain('/feed.xml</loc>')
which also matches locale-prefixed entries; replace it with a strict regex match
against the exact <loc> element for the global feed (e.g. use
xml.match(/<loc>\/feed\.xml<\/loc>/g) to find occurrences) and assert that the
resulting feedCount (computed from that exact-regex match) equals 1; update the
test block (the test named "sitemap has only one global feed.xml entry (not
locale-prefixed)") to use the xml variable, a strict regex as shown, and the
existing expect(feedCount).toBe(1).
- Around line 304-328: Save the initial llms setting before toggling in
test.beforeAll by reading the checkbox state from the locator
'#llms_txt_enabled' (e.g., const initialLlmsState = await checkbox.isChecked()),
then only change it if necessary; in test.afterAll restore the original state
using the same locator and submit button '[data-settings-panel="advanced"]
button[type="submit"]' if the current state differs from initialLlmsState so the
global llms toggle is returned to its previous value after the suite finishes.
---
Nitpick comments:
In `@app/Controllers/SeoController.php`:
- Around line 59-80: The llmsTxt() function contains dead locale plumbing and an
unnecessary genres count: remove the unused assignment $locale =
I18n::getInstallationLocale(); remove the genres subquery from the stats SQL and
any extraction of $row['genres'], and stop using the unused loop key $code by
iterating values only (e.g., foreach ($locales as $name) { $langNames[] = $name;
}) so PHPMD warnings are gone and the extra COUNT query is removed; keep the
remaining COUNTs (books, authors, publishers, events) and existing $langNames
population intact.
In `@app/Controllers/SettingsController.php`:
- Around line 626-629: The runtime type for seo.llms_txt_enabled is
inconsistent: SettingsController currently writes a string to the DB via
$repository->set('seo','llms_txt_enabled',$llmsTxtEnabled) but writes a boolean
into ConfigStore via ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled
=== '1'), while SeoController expects a string comparison (=== '1'). Fix by
making the in-memory config use the same string values as the DB: set
ConfigStore::set('seo.llms_txt_enabled', $llmsTxtEnabled) (or alternatively
change SeoController to normalize to boolean everywhere), ensuring all
reads/writes use the same type for the llms_txt_enabled flag.
In `@app/Support/HreflangHelper.php`:
- Around line 45-50: Centralize the 2-letter locale-prefix rule by adding a
single utility (e.g., a static method getLocalePrefix(string $locale): string on
a new or existing Support/LocaleHelper or Locale class) that returns '/' .
strtolower(substr($locale, 0, 2)); then replace the inline uses in
HreflangHelper (the foreach using substr($localeCode, 0, 2) and the later
occurrence around lines 79-80) and in SitemapGenerator::getLocalePrefix() to
call LocaleHelper::getLocalePrefix($locale) instead so all code reuses the same
implementation.
In `@locale/de_DE.json`:
- Line 4064: Remove the duplicate JSON key "Informazioni di contatto" that
appears twice (once at the earlier occurrence and again in the diff) to avoid
ambiguous resolution; keep a single entry mapping "Informazioni di contatto" to
"Kontaktinformationen" (delete the duplicated line from the diff) or consolidate
any differing translations into that one canonical key; ensure the resulting
locale JSON has only one "Informazioni di contatto" key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb2463f9-6b3b-4a61-b976-4a42001e1f0f
📒 Files selected for processing (12)
README.mdapp/Controllers/FeedController.phpapp/Controllers/SeoController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Support/HreflangHelper.phpapp/Support/SitemapGenerator.phpapp/Views/settings/advanced-tab.phplocale/de_DE.jsonlocale/en_US.jsontests/seo-feed.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Routes/web.php
- app/Controllers/FeedController.php
Comment on lines
+226
to
+255
| private function getEventEntries(): array | ||
| { | ||
| $entries = []; | ||
| $sql = "SELECT slug, updated_at, created_at FROM events WHERE is_active = 1 ORDER BY event_date DESC"; | ||
|
|
||
| if ($result = $this->db->query($sql)) { | ||
| while ($row = $result->fetch_assoc()) { | ||
| $slug = trim((string)($row['slug'] ?? '')); | ||
| if ($slug === '') { | ||
| continue; | ||
| } | ||
|
|
||
| $lastmod = $row['updated_at'] ?? $row['created_at'] ?? null; | ||
|
|
||
| foreach ($this->activeLocales as $locale) { | ||
| $localePrefix = $this->getLocalePrefix($locale); | ||
| $eventsPath = RouteTranslator::getRouteForLocale('events', $locale); | ||
|
|
||
| $entries[] = [ | ||
| 'loc' => $this->baseUrl . $localePrefix . $eventsPath . '/' . rawurlencode($slug), | ||
| 'changefreq' => 'monthly', | ||
| 'priority' => '0.6', | ||
| 'lastmod' => $lastmod, | ||
| ]; | ||
| } | ||
| } | ||
| $result->free(); | ||
| } | ||
|
|
||
| return $entries; |
There was a problem hiding this comment.
Respect the global events visibility setting before publishing event URLs.
This method only filters on events.is_active. If the site-wide events section is disabled, the frontend hides those pages, but the sitemap will still advertise their detail URLs. Please gate event entries behind the same visibility check the frontend uses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Support/SitemapGenerator.php` around lines 226 - 255, The getEventEntries
method currently adds event URLs regardless of the site-wide events visibility;
before querying/iterating, gate the logic by checking the global events
visibility flag the frontend uses (e.g. call the existing config/settings
accessor or method used elsewhere such as isSectionEnabled('events') or
$this->settings['events_enabled']), and return an empty array if events are
disabled; ensure you add this check at the top of getEventEntries so the
RouteTranslator::getRouteForLocale and $this->activeLocales loop only run when
the global events section is enabled.
Comment on lines
+225
to
+233
| test('sitemap has only one global feed.xml entry (not locale-prefixed)', async ({ request }) => { | ||
| const resp = await request.get(`${BASE}/sitemap.xml`); | ||
| expect(resp.status()).toBe(200); | ||
| const xml = await resp.text(); | ||
|
|
||
| // feed.xml is a global endpoint — only one entry, no /en/ prefix | ||
| expect(xml).toContain('/feed.xml</loc>'); | ||
| const feedCount = (xml.match(/feed\.xml<\/loc>/g) || []).length; | ||
| expect(feedCount).toBe(1); |
There was a problem hiding this comment.
Assert the exact global feed.xml location.
toContain('/feed.xml</loc>') also matches /en/feed.xml</loc>, so this can still pass with a locale-prefixed entry and no global one.
🔧 Proposed fix
- expect(xml).toContain('/feed.xml</loc>');
+ const escapedBase = BASE.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ expect(xml).toMatch(new RegExp(`<loc>${escapedBase}/feed\\.xml</loc>`));
+ expect(xml).not.toMatch(new RegExp(`<loc>${escapedBase}/[a-z]{2}/feed\\.xml</loc>`));
const feedCount = (xml.match(/feed\.xml<\/loc>/g) || []).length;
expect(feedCount).toBe(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/seo-feed.spec.js` around lines 225 - 233, The current assertion uses
toContain('/feed.xml</loc>') which also matches locale-prefixed entries; replace
it with a strict regex match against the exact <loc> element for the global feed
(e.g. use xml.match(/<loc>\/feed\.xml<\/loc>/g) to find occurrences) and assert
that the resulting feedCount (computed from that exact-regex match) equals 1;
update the test block (the test named "sitemap has only one global feed.xml
entry (not locale-prefixed)") to use the xml variable, a strict regex as shown,
and the existing expect(feedCount).toBe(1).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
app/Support/SitemapGenerator.php (1)
226-255:⚠️ Potential issue | 🟠 MajorGate event URL emission on site-wide events visibility.
At Line 229, this method only checks
events.is_active. If the global events section is disabled, sitemap entries can still be published for hidden pages. Add an early return using the same events-enabled source used by the frontend before running the query/locale loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/SitemapGenerator.php` around lines 226 - 255, In getEventEntries(), add an early return when the site-wide events section is disabled by checking the same feature flag or settings call the frontend uses (e.g. the application's events-enabled config/method such as Settings::isEventsEnabled(), config('features.events'), or the project's equivalent) before running the DB query/locale loop; this prevents emitting URLs when events are globally hidden while leaving the existing slug filtering, DB query ($this->db->query()), locale iteration ($this->activeLocales, getLocalePrefix(), RouteTranslator::getRouteForLocale()) and entry construction untouched.locale/en_US.json (1)
4064-4064:⚠️ Potential issue | 🟡 MinorRemove the duplicated
Informazioni di contattokey.Line 4064 redefines a key already present earlier in
locale/en_US.json, so one value silently overrides the other.🧹 Minimal fix
"Informazioni sulla biblioteca": "About this library", - "Informazioni di contatto": "Contact information", "Calendario eventi culturali": "Cultural events calendar",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` at line 4064, Remove the duplicated JSON key "Informazioni di contatto" by deleting the later occurrence (the entry at the reported change) so only the original translation remains; search for the "Informazioni di contatto" key in locale/en_US.json, remove the redundant object entry, ensure surrounding commas are adjusted to keep valid JSON, and run a quick JSON validation to confirm no syntax errors.app/Views/settings/advanced-tab.php (1)
338-352:⚠️ Potential issue | 🟠 MajorLabel the
llms_txt_enabledswitch for assistive tech.The visible copy sits outside the
<label>, so thesr-onlycheckbox still has no accessible name and only the small switch is clickable.♿ Suggested fix
<div class="flex items-center justify-between"> <div> - <span class="text-sm font-semibold text-gray-900"><?= __("Abilita llms.txt") ?></span> - <p class="text-xs text-gray-600"><?= __("Rende disponibile /llms.txt e lo aggiunge a robots.txt") ?></p> + <span id="llms_txt_enabled_label" class="text-sm font-semibold text-gray-900"><?= __("Abilita llms.txt") ?></span> + <p id="llms_txt_enabled_description" class="text-xs text-gray-600"><?= __("Rende disponibile /llms.txt e lo aggiunge a robots.txt") ?></p> </div> <label class="relative inline-flex items-center cursor-pointer"> <input type="checkbox" id="llms_txt_enabled" name="llms_txt_enabled" value="1" + aria-labelledby="llms_txt_enabled_label" + aria-describedby="llms_txt_enabled_description" <?php echo $llmsTxtEnabled ? 'checked' : ''; ?> class="toggle-checkbox sr-only">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/settings/advanced-tab.php` around lines 338 - 352, The checkbox input with id/name "llms_txt_enabled" currently has the visible label text outside the <label>, leaving the sr-only input without an accessible name and making only the small switch clickable; fix by associating the visible text with the input—either wrap the visible text and description inside the same <label> element that contains the input, or add a for="llms_txt_enabled" attribute to the existing <label> and ensure the label contains the span with __("Abilita llms.txt") (and optionally the descriptive paragraph) so the input with id "llms_txt_enabled" receives an accessible name and the entire label is clickable.tests/seo-feed.spec.js (3)
69-72:⚠️ Potential issue | 🟡 MinorNormalize the alternate path relative to
BASEbefore stripping the locale prefix.On subfolder installs,
new URL(href).pathnamestill includes the app base path, so this comparison fails even when the hreflang URLs are correct.🧪 Suggested fix
- const stripLocalePrefix = (href) => - new URL(href).pathname.replace(/^\/[a-z]{2}(?=\/)/, ''); + const basePath = new URL(BASE).pathname.replace(/\/$/, ''); + const stripLocalePrefix = (href) => { + const pathname = new URL(href).pathname; + const relativePath = basePath && pathname.startsWith(basePath) + ? pathname.slice(basePath.length) || '/' + : pathname; + return relativePath.replace(/^\/[a-z]{2}(?=\/)/, ''); + }; expect(stripLocalePrefix(enHref[1])).toBe(stripLocalePrefix(itHref[1]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/seo-feed.spec.js` around lines 69 - 72, The test is stripping locale prefixes from paths but uses new URL(href).pathname which keeps the app base on subfolder installs; update the normalization to resolve the href against the app BASE before extracting the pathname (e.g., use new URL(href, BASE).pathname), then apply the existing stripLocalePrefix logic (the local stripLocalePrefix helper and the enHref/itHref comparison) so both URLs are compared relative to the base without the locale segment.
299-328:⚠️ Potential issue | 🟠 MajorRestore the persisted
llms.txtsetting after the suite.This serial block turns on a global toggle and leaves it enabled, so later specs become order-dependent and local E2E databases stay dirty after the run.
🧪 Suggested fix
test.describe.serial('llms.txt', () => { /** `@type` {import('@playwright/test').BrowserContext} */ let context; /** `@type` {import('@playwright/test').Page} */ let page; + let llmsWasEnabled = false; test.beforeAll(async ({ browser }) => { test.skip(!ADMIN_EMAIL || !ADMIN_PASS, 'E2E admin credentials not configured'); context = await browser.newContext(); @@ await page.goto(`${BASE}/admin/settings?tab=advanced`); await page.waitForLoadState('networkidle'); const checkbox = page.locator('#llms_txt_enabled'); - if (!await checkbox.isChecked()) { + llmsWasEnabled = await checkbox.isChecked(); + if (!llmsWasEnabled) { await checkbox.check(); await page.locator('[data-settings-panel="advanced"] button[type="submit"]').first().click(); await page.waitForLoadState('networkidle'); } }); test.afterAll(async () => { + if (page && !llmsWasEnabled) { + await page.goto(`${BASE}/admin/settings?tab=advanced`); + await page.waitForLoadState('networkidle'); + const checkbox = page.locator('#llms_txt_enabled'); + if (await checkbox.isChecked()) { + await checkbox.uncheck(); + await page.locator('[data-settings-panel="advanced"] button[type="submit"]').first().click(); + await page.waitForLoadState('networkidle'); + } + } await context?.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/seo-feed.spec.js` around lines 299 - 328, Before changing the global llms.txt toggle in test.beforeAll, read and store its initial state using the checkbox locator '#llms_txt_enabled' (e.g., within test.beforeAll capture const initialLlmsChecked = await checkbox.isChecked()), then only change it if needed; in test.afterAll restore that saved state by navigating back to the advanced settings, setting the checkbox to initialLlmsChecked (check or uncheck as appropriate) and submitting the settings form so the toggle is returned to its original value; reference the test.beforeAll/test.afterAll blocks and the '#llms_txt_enabled' locator when implementing this restore logic.
230-233:⚠️ Potential issue | 🟡 MinorMake the “global
feed.xml” assertion exact.
/feed.xml</loc>andfeed\.xml</loc>still match locale-prefixed entries, so this test can pass without ever proving that the sitemap contains the one non-localized feed URL.🧪 Suggested fix
// feed.xml is a global endpoint — only one entry, no /en/ prefix - expect(xml).toContain('/feed.xml</loc>'); - const feedCount = (xml.match(/feed\.xml<\/loc>/g) || []).length; + const escapedBase = BASE.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + expect(xml).toMatch(new RegExp(`<loc>${escapedBase}/feed\\.xml</loc>`)); + expect(xml).not.toMatch(new RegExp(`<loc>${escapedBase}/[a-z]{2}/feed\\.xml</loc>`)); + const feedCount = (xml.match(new RegExp(`<loc>${escapedBase}/feed\\.xml</loc>`, 'g')) || []).length; expect(feedCount).toBe(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/seo-feed.spec.js` around lines 230 - 233, The current assertions match any occurrence of "feed.xml</loc>" including locale-prefixed entries; update the test in tests/seo-feed.spec.js to assert an exact non-localized feed URL by changing the simple substring check and the feedCount regex to match a full <loc> element whose path ends with "/feed.xml" (for example use a regex like <loc>(?:https?:\/\/[^\/]+)?\/feed\.xml<\/loc>) so it only counts the global /feed.xml entry and not /en/feed.xml; replace the existing expect(xml).toContain(...) and the feedCount regex with this stricter pattern and assert count === 1.
🧹 Nitpick comments (2)
app/Support/ConfigStore.php (1)
565-570: Consider handling boolean flags consistently with other categories.The SEO settings are loaded as plain strings, but
llms_txt_enabledis a boolean flag. Other boolean flags in ConfigStore (e.g.,api.enabled,system.catalogue_mode,cookie_banner.show_*) usefilter_var()to convert to actual booleans when loading from the database.While this works (PHP treats
'0'as falsy), it creates an API inconsistency:ConfigStore::get('api.enabled')returnsbool, butConfigStore::get('seo.llms_txt_enabled')returnsstring.♻️ Proposed fix for consistency
if (!empty($raw['seo'])) { self::$dbSettingsCache['seo'] = []; foreach ($raw['seo'] as $key => $value) { - self::$dbSettingsCache['seo'][$key] = (string) $value; + // Handle boolean flag for llms_txt_enabled + if ($key === 'llms_txt_enabled') { + $boolValue = filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + if ($boolValue === null) { + $boolValue = in_array((string) $value, ['1', 'true', 'yes'], true); + } + self::$dbSettingsCache['seo'][$key] = $boolValue; + } else { + self::$dbSettingsCache['seo'][$key] = (string) $value; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/ConfigStore.php` around lines 565 - 570, The SEO block stores every value as string into self::$dbSettingsCache['seo'], causing boolean flags like llms_txt_enabled to remain strings; update the loop in ConfigStore so values that represent booleans are converted with filter_var($value, FILTER_VALIDATE_BOOLEAN) (either for the specific key 'llms_txt_enabled' or for keys matching boolean patterns such as ending with '_enabled' or starting with 'show_'), otherwise keep casting to string—this ensures ConfigStore::get('seo.llms_txt_enabled') returns a bool consistently like other categories.app/Controllers/SeoController.php (1)
59-79: Remove unused locals (and unused stats column) to keepllmsTxtclean.Line 59 (
$locale) and Line 79 ($code) are unused, andgenresis queried but never read.♻️ Suggested cleanup
- $locale = I18n::getInstallationLocale(); $locales = I18n::getAvailableLocales(); // Gather stats $stats = $db->query( "SELECT (SELECT COUNT(*) FROM libri WHERE deleted_at IS NULL) AS books, (SELECT COUNT(*) FROM autori) AS authors, (SELECT COUNT(*) FROM editori) AS publishers, - (SELECT COUNT(*) FROM generi) AS genres, (SELECT COUNT(*) FROM events WHERE is_active = 1) AS events" ); // Language list $langNames = []; - foreach ($locales as $code => $name) { + foreach ($locales as $name) { $langNames[] = $name; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/SeoController.php` around lines 59 - 79, Remove the unused local variable $locale and stop selecting the unused "generi" column in the stats query; update the SQL in SeoController (the query assigned to $stats) to omit the generi subselect and adjust any dependent parsing of $row accordingly, and simplify the locales loop by removing the unused $code variable in the foreach over $locales (e.g., iterate only over $name) so $langNames is built from the used $name value; ensure $authorCount, $publisherCount, $bookCount and $eventCount remain parsed from $row as before.
🤖 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/HreflangHelper.php`:
- Around line 29-31: The alternate URL builder is dropping the installation
subfolder because SeoController::resolveBaseUrl() can return a host-only base
and the code later simply concatenates $baseUrl . $fullPath; update the logic in
HreflangHelper (use the $basePath from HtmlHelper::getBasePath() together with
$baseUrl and $fullPath) so that when $baseUrl has no path you prepend or inject
$basePath (or always join rtrim($baseUrl,'/') and
ltrim($basePath.'/'.$fullPath,'/')) before emitting alternates; reference
$basePath, $baseUrl, $fullPath and SeoController::resolveBaseUrl() to locate and
fix the concatenation where alternates are built.
In `@app/Views/frontend/layout.php`:
- Around line 1571-1576: The RSS link in layout.php currently uses hard-coded
English for title and aria-label; update the anchor in the footer (the <a ...
title="RSS Feed" aria-label="RSS Feed"> with the RSS SVG) to use the app's
localization helper (e.g., lang('...') or __(...)) instead of literal "RSS Feed"
so both title and aria-label are translated; locate the anchor element in
layout.php and replace both attributes with calls to the chosen translation
function (use an appropriate key like 'rss.feed' or 'rss_feed' that you add to
the locale files).
---
Duplicate comments:
In `@app/Support/SitemapGenerator.php`:
- Around line 226-255: In getEventEntries(), add an early return when the
site-wide events section is disabled by checking the same feature flag or
settings call the frontend uses (e.g. the application's events-enabled
config/method such as Settings::isEventsEnabled(), config('features.events'), or
the project's equivalent) before running the DB query/locale loop; this prevents
emitting URLs when events are globally hidden while leaving the existing slug
filtering, DB query ($this->db->query()), locale iteration
($this->activeLocales, getLocalePrefix(), RouteTranslator::getRouteForLocale())
and entry construction untouched.
In `@app/Views/settings/advanced-tab.php`:
- Around line 338-352: The checkbox input with id/name "llms_txt_enabled"
currently has the visible label text outside the <label>, leaving the sr-only
input without an accessible name and making only the small switch clickable; fix
by associating the visible text with the input—either wrap the visible text and
description inside the same <label> element that contains the input, or add a
for="llms_txt_enabled" attribute to the existing <label> and ensure the label
contains the span with __("Abilita llms.txt") (and optionally the descriptive
paragraph) so the input with id "llms_txt_enabled" receives an accessible name
and the entire label is clickable.
In `@locale/en_US.json`:
- Line 4064: Remove the duplicated JSON key "Informazioni di contatto" by
deleting the later occurrence (the entry at the reported change) so only the
original translation remains; search for the "Informazioni di contatto" key in
locale/en_US.json, remove the redundant object entry, ensure surrounding commas
are adjusted to keep valid JSON, and run a quick JSON validation to confirm no
syntax errors.
In `@tests/seo-feed.spec.js`:
- Around line 69-72: The test is stripping locale prefixes from paths but uses
new URL(href).pathname which keeps the app base on subfolder installs; update
the normalization to resolve the href against the app BASE before extracting the
pathname (e.g., use new URL(href, BASE).pathname), then apply the existing
stripLocalePrefix logic (the local stripLocalePrefix helper and the
enHref/itHref comparison) so both URLs are compared relative to the base without
the locale segment.
- Around line 299-328: Before changing the global llms.txt toggle in
test.beforeAll, read and store its initial state using the checkbox locator
'#llms_txt_enabled' (e.g., within test.beforeAll capture const
initialLlmsChecked = await checkbox.isChecked()), then only change it if needed;
in test.afterAll restore that saved state by navigating back to the advanced
settings, setting the checkbox to initialLlmsChecked (check or uncheck as
appropriate) and submitting the settings form so the toggle is returned to its
original value; reference the test.beforeAll/test.afterAll blocks and the
'#llms_txt_enabled' locator when implementing this restore logic.
- Around line 230-233: The current assertions match any occurrence of
"feed.xml</loc>" including locale-prefixed entries; update the test in
tests/seo-feed.spec.js to assert an exact non-localized feed URL by changing the
simple substring check and the feedCount regex to match a full <loc> element
whose path ends with "/feed.xml" (for example use a regex like
<loc>(?:https?:\/\/[^\/]+)?\/feed\.xml<\/loc>) so it only counts the global
/feed.xml entry and not /en/feed.xml; replace the existing
expect(xml).toContain(...) and the feedCount regex with this stricter pattern
and assert count === 1.
---
Nitpick comments:
In `@app/Controllers/SeoController.php`:
- Around line 59-79: Remove the unused local variable $locale and stop selecting
the unused "generi" column in the stats query; update the SQL in SeoController
(the query assigned to $stats) to omit the generi subselect and adjust any
dependent parsing of $row accordingly, and simplify the locales loop by removing
the unused $code variable in the foreach over $locales (e.g., iterate only over
$name) so $langNames is built from the used $name value; ensure $authorCount,
$publisherCount, $bookCount and $eventCount remain parsed from $row as before.
In `@app/Support/ConfigStore.php`:
- Around line 565-570: The SEO block stores every value as string into
self::$dbSettingsCache['seo'], causing boolean flags like llms_txt_enabled to
remain strings; update the loop in ConfigStore so values that represent booleans
are converted with filter_var($value, FILTER_VALIDATE_BOOLEAN) (either for the
specific key 'llms_txt_enabled' or for keys matching boolean patterns such as
ending with '_enabled' or starting with 'show_'), otherwise keep casting to
string—this ensures ConfigStore::get('seo.llms_txt_enabled') returns a bool
consistently like other categories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98ffc07e-58f3-4398-b94e-ff435d214f13
📒 Files selected for processing (13)
README.mdapp/Controllers/FeedController.phpapp/Controllers/SeoController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Support/HreflangHelper.phpapp/Support/SitemapGenerator.phpapp/Views/frontend/layout.phpapp/Views/settings/advanced-tab.phplocale/de_DE.jsonlocale/en_US.jsontests/seo-feed.spec.js
Comment on lines
+29
to
+31
| $defaultLocale = I18n::getInstallationLocale(); | ||
| $basePath = HtmlHelper::getBasePath(); | ||
| $baseUrl = SeoController::resolveBaseUrl(); |
There was a problem hiding this comment.
Preserve the installation subfolder when building alternate URLs.
Lines 39-40 strip $basePath from the request, but Line 92 only concatenates $baseUrl . $fullPath. When SeoController::resolveBaseUrl() falls back to host-only output, subfolder installs emit broken alternates like https://example.com/en/catalog instead of https://example.com/pinakes/en/catalog.
🔧 Suggested fix
$defaultLocale = I18n::getInstallationLocale();
$basePath = HtmlHelper::getBasePath();
$baseUrl = SeoController::resolveBaseUrl();
+ $baseHref = rtrim($baseUrl, '/');
+ $resolvedBasePath = rtrim((string)(parse_url($baseHref, PHP_URL_PATH) ?? ''), '/');
+ if ($basePath !== '' && $resolvedBasePath !== rtrim($basePath, '/')) {
+ $baseHref .= $basePath;
+ }
@@
$alternates[] = [
'hreflang' => $langCode,
- 'href' => $baseUrl . $fullPath,
+ 'href' => $baseHref . $fullPath,
];Also applies to: 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Support/HreflangHelper.php` around lines 29 - 31, The alternate URL
builder is dropping the installation subfolder because
SeoController::resolveBaseUrl() can return a host-only base and the code later
simply concatenates $baseUrl . $fullPath; update the logic in HreflangHelper
(use the $basePath from HtmlHelper::getBasePath() together with $baseUrl and
$fullPath) so that when $baseUrl has no path you prepend or inject $basePath (or
always join rtrim($baseUrl,'/') and ltrim($basePath.'/'.$fullPath,'/')) before
emitting alternates; reference $basePath, $baseUrl, $fullPath and
SeoController::resolveBaseUrl() to locate and fix the concatenation where
alternates are built.
- Remove duplicate "Informazioni di contatto" key from en_US/de_DE JSON - Store seo.llms_txt_enabled as string in ConfigStore (matches === '1' reads) - Remove unused $locale, genres subquery, $code loop key in llmsTxt() - Add aria-labelledby/aria-describedby to llms_txt toggle for accessibility - Restore llms setting in E2E afterAll to avoid test order dependency
- Replace hardcoded title/aria-label="RSS Feed" with __('Feed RSS')
translation helper in layout footer
- Fix stripLocalePrefix test helper to strip base path before locale
regex, supporting subfolder installations
- Strip \r from CSV description normalization and add \r detection in CSV escaping lambda to prevent column misalignment (#83) - Replace single-link genre display with per-level clickable links on admin book detail page (#90) - Resolve genre names server-side for filter badge instead of #ID (#90) - Add prepare() return checks with graceful degradation in genre lookup - Add error_log on DB query failures in FeedController, SeoController - Add xmlEscape on RSS pubDate/lastBuildDate for consistency - Add (string) cast on ConfigStore::get() in SeoController - Add logging in SitemapGenerator catch blocks and HreflangHelper JSON
- 3 tests verify CSV export strips \r, fields stay aligned, and multi-line descriptions are correctly quoted - 3 tests verify admin genre hierarchy has per-level clickable links with arrow separators, and filter badge shows genre name not #ID - Add "Filtro attivo" translation to en_US and de_DE locale files
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
HreflangHelperclass that generates<link rel="alternate" hreflang="xx">tags for all active locales on every frontend page. Supports translated routes (e.g.,/catalogo↔/en/catalog), entity URLs (books, authors), andx-default. Works dynamically with any number of installed languages./feed.xmlendpoint (FeedController) serving RSS 2.0 with the latest 50 books. Includes autodiscovery<link>in layout<head>,Feed:directive inrobots.txt, and RSS icon in the footer.eventstable to sitemap, plus/feed.xmlas a static entry witheventscounter in stats./llms.txtendpoint with admin toggle in Advanced Settings.Bug fixes
\rfrom descriptions to prevent column misalignment in LibreOffice/Excel#IDError handling improvements
error_log()on DB query failures in FeedController, SeoControllerprepare()return checks with graceful degradation in genre lookupxmlEscape()on RSS date fields for consistency(string)cast onConfigStore::get()in SeoControllerFiles changed
app/Support/HreflangHelper.phpapp/Controllers/FeedController.phpapp/Controllers/SeoController.phpFeed:line to robots.txt, llms.txt endpointapp/Controllers/LibriController.php\rfix, genre filter name resolutionapp/Controllers/SettingsController.phpapp/Views/frontend/layout.phpapp/Views/libri/scheda_libro.phpapp/Views/libri/index.phpapp/Views/settings/advanced-tab.phpapp/Routes/web.php/feed.xmland/llms.txtroutesapp/Support/SitemapGenerator.phpapp/Support/ConfigStore.phpseocategory supportlocale/en_US.json,locale/de_DE.jsontests/seo-feed.spec.jstests/issue-83-90-csv-genre.spec.jsTest plan
\r, columns aligned, quoted fields correct→separatorsGenere: «Fiction»instead ofGenere #226🤖 Generated with Claude Code