Repository navigation
fix: single album page cache entry, working 304s, hero template in dark theme - #137
Merged
Merged
Conversation
…and invalidation The album page was rendered from the album_<id> entry while a second, partially built album:<slug> entry was written by every warm. That second entry was never rendered, but CacheMiddleware took the page ETag from it, so the validator did not follow the page: a settings change that altered the album layout left the ETag unchanged. PageCacheService::albumPageKey() is now the single key format. The ETag is read from that entry (a ?template=N request falls back to hashing the body), invalidateAlbum() targets it, and the partial album warm is removed: home and galleries are still pre-built, album pages are cached on their first anonymous visit and rebuilt single-flight after an edit. cache:warm keeps accepting --albums and says so; the admin Cache page clears album entries instead of pretending to regenerate them. The file backend now lists and clears every page entry, not just home and galleries. ETag comparison also ignores the -gzip / -br suffix Apache appends when it compresses the response; with it, a compressed page could never revalidate to 304. e2e: ECA-03 in album-edit-cache.spec.js covers the 304 on an unchanged page and the 200 after a layout change.
The readability veil over the hero image was a fixed white gradient and the panel forced dark text, while the category, tag and date pills inside it switched to their dark styles: dark pills with dark text on a white panel, and album body text that turned light on white. The veil now uses the theme surface colour (--color-bg-secondary-rgb), so the panel is white in the light theme and near-black in the dark one and everything inside uses its normal light/dark styles. The .preserve-light-text override, only used here, is removed. Light theme rendering is unchanged.
The test only read the admin layout, so a plugin subtitle going back to a fixed string, or a language pack missing a plugin key, would have gone unnoticed (the trans() default hides a missing key).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughLe cache album passano a chiavi basate sull’ID e non vengono più preriscaldate. Le richieste anonime generano le pagine album e gli ETag considerano i parametri query. Il pannello hero adatta i propri stili al tema. I test delle traduzioni includono anche i plugin. ChangesCache delle pagine
Tema del pannello hero
Traduzioni della sidebar
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ClientAnonimo
participant CacheMiddleware
participant PageCacheService
participant PageController
ClientAnonimo->>CacheMiddleware: Richiede la pagina album con i parametri query
CacheMiddleware->>PageCacheService: Risolve la chiave album dallo slug
PageCacheService-->>CacheMiddleware: Restituisce la chiave o null
CacheMiddleware->>PageController: Prosegue la richiesta
PageController->>PageCacheService: Legge o salva la pagina con albumPageKey
PageCacheService-->>PageController: Restituisce o registra la pagina
CacheMiddleware-->>ClientAnonimo: Restituisce la risposta con l'ETag
Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 10 files. (4 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check Il titolo descrive in modo chiaro i principali cambiamenti: cache delle pagine album, risposte 304 tramite ETag e tema scuro per il template hero. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 10 files. (4 skipped: 4 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 PHPStan (2.2.14)
-
PHPStan was skipped because the config uses disallowed
bootstrapFilesorbootstrapFiledirectives.
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.
Un coniglio saltella tra chiavi e cache,
la home si scalda, l’album attende.
Un tema scuro veste l’hero,
nuove chiavi brillano nei test.
«Tutto in ordine», squittisce allegro,
e lascia impronte sulla neve.
Comment @coderabbitai help to get the list of available commands.
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
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.
Follow-up to #132 and #136: two loose ends I noted while reviewing, plus what turned up while fixing them.
1. One cache entry per album page
The album page was rendered from the
album_<id>entry, while every warm wrote a second, partially builtalbum:<slug>entry. That second entry was never rendered, butCacheMiddlewaretook the page ETag from it, so the validator did not follow the page: a settings change that altered the album layout left the ETag unchanged.PageCacheService::albumPageKey()is now the single key format, used byPageController(render),CacheMiddleware(ETag) andinvalidateAlbum(). A?template=Nrequest falls back to hashing the body.buildGalleriesCache()already does when it cannot reproduce the live page faithfully.cache:warmkeeps accepting--albumsand says album pages need no pre-build; the admin Cache page clears album entries instead of reporting a regeneration that never fed the page.2. 304 never happened behind a compressing Apache
Apache appends
-gzip/-brto the ETag when it compresses the response, and the browser sends that back inIf-None-Match. The comparison did not strip the suffix, so a compressed page could never revalidate to 304.normalizeEtag()now ignores the content-coding suffix.3. Hero album template in the dark theme
The readability veil over the hero image was a fixed white gradient and the panel forced dark text, while the pills inside switched to their dark styles: dark pills with dark text on a white panel, and body text that turned light on white. The veil now uses the theme surface colour (
--color-bg-secondary-rgb), so the panel follows the theme and everything inside uses its normal styles. The.preserve-light-textoverride, only used here, is removed. Light theme rendering is unchanged.4. Test coverage
album-edit-cache.spec.jsECA-03: 304 on an unchanged album page, 200 with a new ETag after a layout change.HeroPanelThemeTest: the hero veil must use the theme surface colour.AdminSidebarTranslationsTestnow also covers the two bundled plugins (CodeRabbit's note on Translate admin sidebar subtitles in English and Italian #128).Verification
app/(two baseline entries for the removedPageCacheService::warm()dropped).--config auto, baseline v1.4.23): no new findings.Home template: classicscreenshot spec, which fails the same way on v1.4.23 when it runs right afteralbum-state-transitions: the analytics beacon hits its 60/min per-IP limit, the 429 keeps the page from reaching network idle. Not caused by this PR.Summary by CodeRabbit
--albums.