Emeroteca: HTTP caching, supplements, truncated lists, and clearer 404s - #35
Merged
Merged
Conversation
The server already tags its cacheable GETs — the whole periodicals surface among them — with an ETag and `Cache-Control: private, max-age=0, must-revalidate`, but the client never sent `If-None-Match` back: no okhttp3.Cache was configured, so OkHttp had no stored validator to attach and every navigation re-downloaded a body the device already had. I give the OkHttp client a ~10 MB disk cache in the app cache dir. Revalidation is now transparent and, since `max-age=0` keeps every request going to the server, this trades bandwidth for nothing — a 304 replays the stored body instead of serving stale data. Cache entries are keyed by URL alone, so I evict the whole cache on logout and on instance switch, next to the Room purge that already happens there. The eviction runs on the IO dispatcher and swallows its own failures: an unpurgeable cache is a bandwidth problem and must not turn signing out into an error the user has to fight.
The issue detail endpoint returns a nullable `supplements` string — the free-text note on inserts bound with a fascicolo — and the app was dropping it on decode. I add it to the DTO and render it in the issue header, reusing the masthead detail's InfoRow so the label/value pair reads identically on both screens (InfoRow goes from private to internal for that). The row appears only for a non-null, non-blank value, like every other optional field in this section.
`GET periodicals/years/{id}/issues` caps its result at 400 fascicoli
and now reports the cut in `meta.truncated`. Without that flag the app
silently presented a partial year as the whole thing.
I add the field to the shared Meta DTO as a nullable with a null
default, so an instance that predates it keeps decoding: absent must
read as "complete", never as "truncated". The decision lives in a pure
`isTruncatedList(meta)` so it is testable without a ViewModel, and I
deliberately do not infer truncation from the item count — that would
cry wolf on a year sitting exactly on the cap and would be wrong
outright the day the cap moves.
When the flag is set the issue list shows an informational banner
above the rows. It is not an error state: the issues below are real
and browsable, so nothing is blocked and there is nothing to retry.
PeriodicalsViewModel already degraded gracefully on a 404 — re-probe health, and if the plugin is really off show a terminal "section unavailable" state instead of a retryable error. The three detail screens did not: a masthead, a year or an issue that 404'd produced a generic failure with a retry button that could only 404 again. I extend the same pattern to them, but a 404 on a detail endpoint is ambiguous in a way the list's never was: the one record may have been deleted while the section is perfectly alive. So health stays the oracle — I probe it only on a 404, and only a 404 from health too means the section is gone. Anything else stays "no longer exists", which sends the user back to browsing rather than telling them the whole archive vanished. The decision is a pure function over the failure plus the probe answer, unit-tested for both 404 branches and for the guarantee that a non-404 can never degrade to "gone" even if a stale probe says so. The gone/not-found states drop the server's bare "Not found." message on purpose: resolvedMessage() prefers a non-blank message, so keeping it would have shown that instead of the wording that distinguishes the two cases.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
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
Four improvements to the Emeroteca (periodicals) section, from a full review of the client/server contract against the Pinakes plugin.
Cache-Control: private, max-age=0, must-revalidateon every/api/v1/periodicals/*GET, but the client never sentIf-None-Match— no OkHttp cache was configured, so every refresh re-downloaded the full payload. A 10 MB disk cache now makes revalidation transparent. The cache is keyed by URL and the app is single-account, so isolation rests on eviction:clearHttpCache()runs on sign-out and on instance switch (AuthRepository.logout()/forgetInstance()), insiderunCatchingonDispatchers.IOso a cache IO failure can never break sign-out.InfoRowcomponent as the periodical detail screen.meta.truncatedflag (nullable, so older servers still read as "complete") drives a non-blocking banner. The subtitle interpolates the real item count instead of hardcoding the cap.Tests
./gradlew testDebugUnitTest→ 157 tests, 0 failures.PeriodicalsUiStateTestgrew 12 → 24: the new cases cover the truncated-flag decision (true / explicit false / omitted / null meta), 404 detection, both branches of the failure classifier including the guarantee that a non-404 never degrades to Gone, and the error-state builder. Per the repo rule the ViewModels are not faked — the decision logic lives in pure functions inPeriodicalsUi.ktand is tested there. Each of the first three commits also compiles on its own, so the history bisects cleanly.Strings
Six keys added to all four locales (en/it/fr/de) in
i18n/*.json, inserted next to their siblings so key order stays identical across files.Server side
meta.truncatedis being added to the Pinakes plugin in a parallel PR. The field is optional on the wire, so this app works against current servers as-is.