feat(web-reader): HTTP Caching & Cache Storage for content responses#75
Open
NamLe029 wants to merge 16 commits intogrimmory-tools:developfrom
Open
feat(web-reader): HTTP Caching & Cache Storage for content responses#75NamLe029 wants to merge 16 commits intogrimmory-tools:developfrom
NamLe029 wants to merge 16 commits intogrimmory-tools:developfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughBackend streaming endpoints changed to return ResponseEntity with dynamic Content-Type and optional Last-Modified; CORS updated to allow HEAD and cache headers. Added FileUtils.getFileLastModified. Frontend: introduced CacheStorageService and LocalSettingsService, a Local Settings UI, and integrated cache-aware loading into BookFileService and PDF reader (blob/object-URL handling). Changes
Sequence DiagramsequenceDiagram
actor Browser as Client/Browser
participant App as Frontend App
participant Cache as CacheStorage (Browser)
participant Http as HttpClient/Network
participant API as Backend API
Browser->>App: request book/audio URI
App->>Cache: getCache(uri)
alt cache hit
Cache-->>App: cached Response
alt validate freshness (noValidate = false)
App->>Http: HEAD uri (If-Modified-Since: cached.last-modified)
Http-->>App: 304 Not Modified
App-->>Browser: serve cached blob / object-URL
else 200 OK
Http-->>App: 200 + body + headers
App->>Cache: put(uri, Response.clone())
App-->>Browser: serve fetched blob / object-URL
end
else cache miss
App->>Http: GET uri
Http->>API: GET request
API-->>Http: Response (+ Last-Modified?)
Http-->>App: Response
App->>Cache: put(uri, Response.clone())
App-->>Browser: serve fetched blob / object-URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/service/book/BookService.java (1)
31-31: Consider using explicit imports instead of wildcard.Wildcard imports can obscure dependencies and potentially cause naming collisions. The explicit imports would be:
CacheControl,MediaType,MediaTypeFactory,ResponseEntity.Suggested explicit imports
-import org.springframework.http.*; +import org.springframework.http.CacheControl; +import org.springframework.http.MediaType; +import org.springframework.http.MediaTypeFactory; +import org.springframework.http.ResponseEntity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java` at line 31, Replace the wildcard import "import org.springframework.http.*;" in BookService.java with explicit imports for the symbols used so imports are clear and avoid collisions; specifically add explicit imports for CacheControl, MediaType, MediaTypeFactory, and ResponseEntity (remove the wildcard and organize/import only the classes referenced by methods in BookService).booklore-ui/src/app/shared/service/cache-storage.service.ts (1)
43-43: Handle cache write failures fromput()to avoid unhandled async errors.
this.put(uri, res.clone())is fire-and-forget; ifcaches.open()/cache.put()fails, the rejection is unobserved.Proposed fix
- this.put(uri, res.clone()); + await this.put(uri, res.clone()).catch((error) => { + console.error("Error writing cache:", error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-ui/src/app/shared/service/cache-storage.service.ts` at line 43, The call this.put(uri, res.clone()) is currently fire-and-forget and can produce unobserved promise rejections if caches.open()/cache.put() fails; update the caller (or put method) to await the promise or attach a .catch handler to it and log or handle errors to prevent unhandled async errors. Specifically, locate the invocation this.put(uri, res.clone()) in CacheStorageService and change it to either await this.put(uri, res.clone()) inside an async function or append .catch(err => {/* log via your logger or console.error with context */}) so failures from put() (and internals like caches.open()/cache.put()) are observed and handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-ui/src/app/features/book/service/book-file.service.ts`:
- Around line 34-36: The cache path in BookFileService (when
localSettingsService.get().cacheStorageEnabled is true) must handle rejections
and falsy responses from cacheStorageService.getCache(url) and fall back to
this.http.get; update the logic around cacheStorageService.getCache(url) (the
Promise wrapped with from(...)) to catch Promise rejections and check for a
falsy response/blob, and on error or missing data return the HTTP Observable
(this.http.get<Blob>(url, { responseType: 'blob' as 'json' })); implement this
using RxJS operators (e.g., from(...).pipe(catchError(...)) or
switchMap/mergeMap to choose the HTTP fallback) so cache failures do not
propagate to callers.
In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 209-211: The constructed URI in pdf-reader.component.ts uses
direct concatenation of fileType into the query string (const uri = ... ?
`${API_CONFIG.BASE_URL}/api/v1/books/${bookId}/content?bookType=${fileType}` :
...), which can break for reserved characters; fix it by encoding the bookType
value with encodeURIComponent(fileType) when building the query parameter (keep
bookId and base URL unchanged) so the resulting uri is safe for HTTP requests.
- Around line 205-217: getBookData currently creates blob URLs via
URL.createObjectURL(blob) and assigns them to this.bookData without revoking
previous ones, and ngOnDestroy doesn't revoke the active blob URL; to fix,
introduce a property (e.g., currentBlobUrl) to track the active object URL,
ensure before assigning a new URL in getBookData you revoke the existing one via
URL.revokeObjectURL(currentBlobUrl) (and set currentBlobUrl to the new URL after
assignment), and revoke currentBlobUrl inside ngOnDestroy to release memory;
update any places that read/write this.bookData to use the tracked URL
accordingly (functions: getBookData, ngOnDestroy, and where this.bookData is
set/used).
In
`@booklore-ui/src/app/features/settings/local-settings/local-settings.component.html`:
- Around line 4-57: The template in local-settings.component.html is using
*transloco="let t; prefix: 'settingsNaming'" but still contains hardcoded
strings; replace each literal (header title/description, section
title/description, labels/descriptions for "Enable Cache Storage", "Cache
Storage Usage (MB)", "Clear Cache", and button labels "Refresh"/"Clear Cache")
with the appropriate transloco lookup using the t function (e.g. {{
t('localSettings.title') }}, {{ t('localSettings.description') }}, {{
t('cacheStorage.title') }}, etc.) while keeping bindings and event handlers
intact (settings.cacheStorageEnabled, cacheStorageUsageMb,
isLoadingCacheStorageUsage, isClearingCacheStorage, loadCacheStorageUsage(),
clearCacheStorage()); ensure keys use the existing prefix 'settingsNaming' so
strings localize consistently with other settings components.
In
`@booklore-ui/src/app/features/settings/local-settings/local-settings.component.scss`:
- Around line 9-45: Stylelint is flagging declaration-empty-line-before
violations in the SCSS blocks; open the .scss and for each selector shown in the
diff (.settings-title, .settings-description, .settings-card, .section-header,
.section-title, .section-description, .settings-content, .setting-item) adjust
blank lines so they follow the project's declaration-empty-line-before rule
(remove unexpected empty lines before declaration lists or add the single
required blank line where configured), ensure there are no extra blank lines
between declarations inside each block, and re-run stylelint (or apply stylelint
--fix) to verify the violations are resolved.
In
`@booklore-ui/src/app/features/settings/local-settings/local-settings.component.ts`:
- Around line 59-69: Wrap the body of the accept async handler in a
try/catch/finally: call await this.cacheStorageService.clear() and await
this.loadCacheStorageUsage() inside try, catch errors to call
this.messageService.add with a severity "error" and useful detail, and ensure
this.isClearingCacheStorage is always reset to false in finally; keep the
existing success message in the try path and reference the accept method,
isClearingCacheStorage flag, cacheStorageService.clear, loadCacheStorageUsage,
and messageService.add when making the changes.
In `@booklore-ui/src/app/features/settings/settings.component.html`:
- Around line 13-15: The "Local" tab label is hardcoded; update the p-tab
content that uses SettingsTab.LocalSettings to use the translation helper (e.g.,
{{ t('settings.tabs.local') }}) instead of the literal "Local", and add the
corresponding "settings.tabs.local" key with value "Local" to the translation
files (e.g., en.json) so the tab is internationalized; ensure you reference the
same key pattern used by other tabs like {{ t('reader') }}.
In `@booklore-ui/src/app/shared/service/cache-storage.service.ts`:
- Around line 97-101: The clear() method currently maps cache.keys() to an array
of this.delete(...) promises but doesn't wait for them, so fix clear() to await
all deletions before resolving: after obtaining keys from cache() and mapping to
this.delete(key.url), return or await Promise.all(...) of those delete promises
(or convert clear() to async and await the deletions). Ensure you still resolve
to void after all this.delete(...) calls complete, keeping references to the
cache() and delete(...) methods.
- Line 70: The catch clause in CacheStorageService uses an invalid union
annotation `catch (error: HttpErrorResponse | any)` which violates TS catch
typing rules; change the catch parameter to `catch (error: unknown)` in the
method where this appears (look for the catch in cache-storage.service.ts, e.g.,
inside methods of class CacheStorageService), then inside the catch block narrow
the error with runtime checks (for example use `if (error instanceof
HttpErrorResponse)` or type guard helpers) before accessing
HttpErrorResponse-specific properties, otherwise treat it as a generic error.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java`:
- Line 31: Replace the wildcard import "import org.springframework.http.*;" in
BookService.java with explicit imports for the symbols used so imports are clear
and avoid collisions; specifically add explicit imports for CacheControl,
MediaType, MediaTypeFactory, and ResponseEntity (remove the wildcard and
organize/import only the classes referenced by methods in BookService).
In `@booklore-ui/src/app/shared/service/cache-storage.service.ts`:
- Line 43: The call this.put(uri, res.clone()) is currently fire-and-forget and
can produce unobserved promise rejections if caches.open()/cache.put() fails;
update the caller (or put method) to await the promise or attach a .catch
handler to it and log or handle errors to prevent unhandled async errors.
Specifically, locate the invocation this.put(uri, res.clone()) in
CacheStorageService and change it to either await this.put(uri, res.clone())
inside an async function or append .catch(err => {/* log via your logger or
console.error with context */}) so failures from put() (and internals like
caches.open()/cache.put()) are observed and handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 129b7314-91e8-4f5b-8d52-9964d60c56da
📒 Files selected for processing (14)
booklore-api/src/main/java/org/booklore/config/security/SecurityConfig.javabooklore-api/src/main/java/org/booklore/controller/AudiobookReaderController.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-ui/src/app/features/book/service/book-file.service.tsbooklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.tsbooklore-ui/src/app/features/settings/local-settings/local-settings.component.htmlbooklore-ui/src/app/features/settings/local-settings/local-settings.component.scssbooklore-ui/src/app/features/settings/local-settings/local-settings.component.tsbooklore-ui/src/app/features/settings/settings.component.htmlbooklore-ui/src/app/features/settings/settings.component.tsbooklore-ui/src/app/shared/service/cache-storage.service.tsbooklore-ui/src/app/shared/service/local-settings.service.ts
booklore-ui/src/app/features/settings/local-settings/local-settings.component.html
Show resolved
Hide resolved
booklore-ui/src/app/features/settings/local-settings/local-settings.component.scss
Show resolved
Hide resolved
booklore-ui/src/app/features/settings/local-settings/local-settings.component.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-ui/src/app/shared/service/cache-storage.service.ts`:
- Around line 100-106: There is an extra closing brace that prematurely ends the
CacheStorageService class (after the clear() method) causing
getCacheSizeInBytes() and cache() to be parsed as top-level statements; remove
the spurious '}' so the class remains open and those methods
(getCacheSizeInBytes, cache, clear, etc.) are defined as members of
CacheStorageService.
- Around line 38-45: The call to this.put(uri, res.clone()) is not awaited so
getCache() can return before the entry is persisted and write failures are
ignored; update the code to await the cache write (await this.put(uri,
res.clone())) and wrap it in a try/catch so write errors are logged/handled (or
rethrown) rather than silently dropped; ensure put(uri, Response) returns a
Promise if it doesn't already and that callers of getCache()/put() can properly
observe failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d01da3b-d2e2-48ea-af14-560ed6801367
📒 Files selected for processing (2)
booklore-ui/src/app/features/settings/local-settings/local-settings.component.tsbooklore-ui/src/app/shared/service/cache-storage.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-ui/src/app/features/settings/local-settings/local-settings.component.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
101-117:⚠️ Potential issue | 🟠 MajorRevoke prior blob URL before replacing
bookDataduring in-component navigation.Line 196 cleanup only runs on destroy; Line 117 can overwrite an existing
blob:URL with a new one while the component is still alive (route param changes), leaving the old URL unreleased.Proposed fix
next: ({book, bookSetting, myself, bookData}) => { + if (this.bookData?.startsWith('blob:') && this.bookData !== bookData) { + URL.revokeObjectURL(this.bookData); + } const pdfMeta = book; const pdfPrefs = bookSetting; @@ this.page = pdfMeta.pdfProgress?.page || 1; this.bookData = bookData;Also applies to: 196-198, 217-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 101 - 117, Before overwriting this.bookData with a new blob URL in the next handler, revoke the previous blob URL to avoid leaking object URLs: if this.bookData is a non-empty string that starts with "blob:" call URL.revokeObjectURL(this.bookData) (guarding for null/undefined), then assign the new bookData; apply the same revoke-before-assign pattern wherever this.bookData is set (including the component's ngOnDestroy cleanup and any other assignments) so previous blob URLs are always released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-ui/src/app/shared/service/cache-storage.service.ts`:
- Around line 70-76: In the catch block inside CacheStorageService
(cache-storage.service.ts) remove the duplicated check for HttpErrorResponse
with status === 304 so the branch appears only once; locate the catch handling
(the function where the try/catch checks "error instanceof HttpErrorResponse &&
error.status === 304") and delete the redundant repeated if statement so the
not-modified return true path is present a single time.
---
Duplicate comments:
In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 101-117: Before overwriting this.bookData with a new blob URL in
the next handler, revoke the previous blob URL to avoid leaking object URLs: if
this.bookData is a non-empty string that starts with "blob:" call
URL.revokeObjectURL(this.bookData) (guarding for null/undefined), then assign
the new bookData; apply the same revoke-before-assign pattern wherever
this.bookData is set (including the component's ngOnDestroy cleanup and any
other assignments) so previous blob URLs are always released.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd4a1d51-c9f8-403d-825c-589f85a0c968
📒 Files selected for processing (3)
booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.tsbooklore-ui/src/app/features/settings/local-settings/local-settings.component.scssbooklore-ui/src/app/shared/service/cache-storage.service.ts
✅ Files skipped from review due to trivial changes (1)
- booklore-ui/src/app/features/settings/local-settings/local-settings.component.scss
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
booklore-ui/src/app/shared/service/cache-storage.service.ts (2)
104-111: Cache size calculation may be inaccurate whencontent-lengthheader is missing.The method relies on the
content-lengthheader which may not always be present (e.g., chunked responses, server omission). For responses without this header, size defaults to 0, under-reporting actual cache usage. Since this is displayed to users in the UI, consider usingblob.sizefor accurate measurement.♻️ Proposed fix using blob size
getCacheSizeInBytes(): Promise<number> { return this.cache() .then((cache) => cache.keys()) .then((keys) => keys.map((key) => this.match(key.url))) .then((promises) => Promise.all(promises)) - .then((responses) => responses.map(response => parseInt(response?.headers.get("content-length") || "0"))) + .then((responses) => Promise.all( + responses.map(response => response?.blob().then(blob => blob.size) ?? Promise.resolve(0)) + )) .then((sizes) => sizes.reduce((total, size) => total + size, 0)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-ui/src/app/shared/service/cache-storage.service.ts` around lines 104 - 111, getCacheSizeInBytes currently sums Content-Length headers which can be missing; update it to fall back to the actual response body size by calling response.blob() and using blob.size when headers lack content-length (and handle null/undefined responses and non-OK responses). In the promise chain starting from cache() -> cache.keys() -> this.match(key.url) (and inside the responses array), for each response check response?.headers.get("content-length") first, parse it if present, otherwise await response.blob() and use blob.size (treat missing response as size 0), then sum those sizes and return the total; keep Promise.all for the blob async calls so behavior remains asynchronous.
26-28: Replace@ts-ignorewith a safer type narrowing approach.The
@ts-ignoredirective is too broad and suppresses all TypeScript errors on the line. The logic guaranteesentryis defined here, but TypeScript cannot infer this. Use a more targeted approach.♻️ Proposed fix
- if (!stale) - //@ts-ignore - return entry; + if (!stale && entry) { + return entry; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-ui/src/app/shared/service/cache-storage.service.ts` around lines 26 - 28, Remove the broad `@ts-ignore` and narrow the type explicitly: instead of suppressing TypeScript, check that entry is not null/undefined (e.g. if (!stale && entry !== undefined) return entry;) or use a non-null assertion only on the return (return entry!) if you are certain entry exists; update the code around the stale and entry variables so TypeScript can infer the defined type without `@ts-ignore`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-ui/src/app/shared/service/cache-storage.service.ts`:
- Around line 104-111: getCacheSizeInBytes currently sums Content-Length headers
which can be missing; update it to fall back to the actual response body size by
calling response.blob() and using blob.size when headers lack content-length
(and handle null/undefined responses and non-OK responses). In the promise chain
starting from cache() -> cache.keys() -> this.match(key.url) (and inside the
responses array), for each response check
response?.headers.get("content-length") first, parse it if present, otherwise
await response.blob() and use blob.size (treat missing response as size 0), then
sum those sizes and return the total; keep Promise.all for the blob async calls
so behavior remains asynchronous.
- Around line 26-28: Remove the broad `@ts-ignore` and narrow the type explicitly:
instead of suppressing TypeScript, check that entry is not null/undefined (e.g.
if (!stale && entry !== undefined) return entry;) or use a non-null assertion
only on the return (return entry!) if you are certain entry exists; update the
code around the stale and entry variables so TypeScript can infer the defined
type without `@ts-ignore`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7831c9f7-4bd3-4c59-b3c4-defbdd6ed117
📒 Files selected for processing (1)
booklore-ui/src/app/shared/service/cache-storage.service.ts
Member
|
@NamLe029 thanks for this -- can you rebase, and also give me a bit of an idea for what this improvement helps with - and what the risks are? usually when adding caching, we need to be thoughtful of areas that might require the latest / non-cached version... so is that afforded throughout? |
Author
|
Oops, apologies for lack of details. Improvements:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
NamLe029
force-pushed
the
web-reader-caching
branch
from
8fac12f to
b551207
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
101-118:⚠️ Potential issue | 🟠 MajorRevoke the previous blob URL before overwriting
bookData.When route params change in the same component instance, Lines 217-219 create a new object URL and Line 117 replaces
this.bookData. Lines 196-198 only revoke the last URL on destroy, so earlier blob URLs stay pinned for the rest of the session.Suggested change
next: ({book, bookSetting, myself, bookData}) => { + if (this.bookData?.startsWith('blob:')) { + URL.revokeObjectURL(this.bookData); + } + const pdfMeta = book; const pdfPrefs = bookSetting; this.pageTitle.setBookPageTitle(pdfMeta); @@ this.canPrint = myself.permissions.canDownload || myself.permissions.admin; this.page = pdfMeta.pdfProgress?.page || 1; this.bookData = bookData;Also applies to: 196-198, 217-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 101 - 118, When assigning a new blob URL to this.bookData (the code path that calls URL.createObjectURL around lines where createObjectURL is used), revoke any previously-created URL first to avoid leaking pinned blobs; e.g., if this.bookData?.blobUrl (or whatever property holds the URL) exists call URL.revokeObjectURL(prevUrl) before overwriting this.bookData, and ensure ngOnDestroy still revokes the final URL (the existing revoke logic in ngOnDestroy should remain but confirm it uses the same property). Update the assignment sites (the createObjectURL block and wherever this.bookData is replaced) to perform the revoke and then set the new blob URL.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
86-91: Treat missing files as a warn-and-null path.This helper logs every missing-path case as an error, while
getFileSizeInKb(Path)in the same class already treats that case as a non-fatal miss. Reusing that pattern here will keep transient cache-validation misses from looking like server faults.♻️ Proposed fix
public Long getFileLastModified(Path filePath) { try { + if (!Files.exists(filePath)) { + log.warn(FILE_NOT_FOUND_MESSAGE + "{}", filePath.toAbsolutePath()); + return null; + } return Files.getLastModifiedTime(filePath).toMillis(); } catch (IOException e) { log.error("Failed to get last modified for path [{}]: {}", filePath, e.getMessage(), e); return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java` around lines 86 - 91, The getFileLastModified(Path) method currently logs all IOExceptions as errors; change it to mirror getFileSizeInKb(Path) by detecting a missing-file condition (e.g., NoSuchFileException or Files.notExists(filePath)) and logging that case at WARN level while returning null, and only log other IOExceptions as errors; update the log call in getFileLastModified to use log.warn(...) for missing-paths and keep log.error(...) for unexpected IOExceptions so transient cache misses aren't treated as server faults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java`:
- Around line 349-350: The response currently sets CacheControl via
CacheControl.noCache().cachePrivate() in BookService (the
builder.cacheControl(...) call) which allows HTTP caching alongside the new
Cache Storage layer; update the backend to make Cache Storage the only persisted
cache by replacing the CacheControl invocation to use
CacheControl.noStore().cachePrivate() (i.e., change CacheControl.noCache() →
CacheControl.noStore() in the builder.cacheControl(...) call), so the server
sends no-store and avoids creating an unmanaged HTTP cache copy.
In
`@booklore-ui/src/app/features/settings/local-settings/local-settings.component.html`:
- Around line 28-33: The label for the "Enable Cache Storage" control is not
associated with the p-checkbox, so clicking the label won't toggle the checkbox
and screen readers lose the relationship; update the label element to include
for="cache-storage-enabled" and set the p-checkbox prop
inputId="cache-storage-enabled" (the p-checkbox bound to
settings.cacheStorageEnabled and using (onChange)="onSettingChange()") so the
visible label is correctly tied to the checkbox input for accessibility.
In `@booklore-ui/src/app/shared/service/local-settings.service.ts`:
- Around line 15-17: The default LocalSettings currently enables persistent
cache by setting defaultSettings.cacheStorageEnabled to true; change this
default to false so cacheStorageEnabled is opt-in via the Local Settings UI
(update the defaultSettings: LocalSettings object and any code that relies on
its initial value such as consumers of LocalSettings to handle false as
default). Ensure references to defaultSettings and the cacheStorageEnabled
property are updated so the app treats persistence as disabled until the user
explicitly enables it.
---
Duplicate comments:
In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 101-118: When assigning a new blob URL to this.bookData (the code
path that calls URL.createObjectURL around lines where createObjectURL is used),
revoke any previously-created URL first to avoid leaking pinned blobs; e.g., if
this.bookData?.blobUrl (or whatever property holds the URL) exists call
URL.revokeObjectURL(prevUrl) before overwriting this.bookData, and ensure
ngOnDestroy still revokes the final URL (the existing revoke logic in
ngOnDestroy should remain but confirm it uses the same property). Update the
assignment sites (the createObjectURL block and wherever this.bookData is
replaced) to perform the revoke and then set the new blob URL.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java`:
- Around line 86-91: The getFileLastModified(Path) method currently logs all
IOExceptions as errors; change it to mirror getFileSizeInKb(Path) by detecting a
missing-file condition (e.g., NoSuchFileException or Files.notExists(filePath))
and logging that case at WARN level while returning null, and only log other
IOExceptions as errors; update the log call in getFileLastModified to use
log.warn(...) for missing-paths and keep log.error(...) for unexpected
IOExceptions so transient cache misses aren't treated as server faults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9450181f-547e-4d95-a1c5-da725e1ab3ca
📒 Files selected for processing (14)
booklore-api/src/main/java/org/booklore/config/security/SecurityConfig.javabooklore-api/src/main/java/org/booklore/controller/AudiobookReaderController.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-ui/src/app/features/book/service/book-file.service.tsbooklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.tsbooklore-ui/src/app/features/settings/local-settings/local-settings.component.htmlbooklore-ui/src/app/features/settings/local-settings/local-settings.component.scssbooklore-ui/src/app/features/settings/local-settings/local-settings.component.tsbooklore-ui/src/app/features/settings/settings.component.htmlbooklore-ui/src/app/features/settings/settings.component.tsbooklore-ui/src/app/shared/service/cache-storage.service.tsbooklore-ui/src/app/shared/service/local-settings.service.ts
✅ Files skipped from review due to trivial changes (2)
- booklore-ui/src/app/features/settings/local-settings/local-settings.component.scss
- booklore-ui/src/app/shared/service/cache-storage.service.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- booklore-api/src/main/java/org/booklore/config/security/SecurityConfig.java
- booklore-ui/src/app/features/settings/settings.component.html
- booklore-ui/src/app/features/settings/settings.component.ts
- booklore-api/src/main/java/org/booklore/controller/BookController.java
- booklore-ui/src/app/features/book/service/book-file.service.ts
- booklore-api/src/main/java/org/booklore/controller/AudiobookReaderController.java
Comment on lines
+28
to
+33
| <label class="setting-label">Enable Cache Storage</label> | ||
| <span class="setting-description">Store data locally in your browser for faster access</span> | ||
| </div> | ||
| <div class="setting-control"> | ||
| <p-checkbox [(ngModel)]="settings.cacheStorageEnabled" [binary]="true" (onChange)="onSettingChange()"> | ||
| </p-checkbox> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "local-settings.component.html" | head -5Repository: grimmory-tools/grimmory
Length of output: 151
🌐 Web query:
PrimeNG p-checkbox inputId label association documentation
💡 Result:
To associate an HTML <label> with a PrimeNG p-checkbox, set an inputId on the checkbox and point the label’s for attribute to the same value (this links to the hidden native input PrimeNG renders). [1]
<p-checkbox inputId="ny" [(ngModel)]="checked" [binary]="true"></p-checkbox>
<label for="ny">New York</label>PrimeNG also documents accessibility alternatives if you can’t/won’t use a <label>: ariaLabelledBy (reference another element’s id) or ariaLabel (string label). [1]
Source: PrimeNG Checkbox docs + Accessibility section. [1]
🏁 Script executed:
cat -n ./booklore-ui/src/app/features/settings/local-settings/local-settings.component.html | sed -n '20,40p'Repository: grimmory-tools/grimmory
Length of output: 1119
🏁 Script executed:
# Also check if there are other label-checkbox patterns in the same file
grep -n "p-checkbox\|<label" ./booklore-ui/src/app/features/settings/local-settings/local-settings.component.htmlRepository: grimmory-tools/grimmory
Length of output: 427
Bind the visible label to the checkbox input for accessibility.
Line 28 renders a <label>, but the p-checkbox at Line 32 has no input association. Clicking the label text won't toggle the control, and assistive technology loses the label relationship. Add inputId="cache-storage-enabled" to the checkbox and for="cache-storage-enabled" to the label.
Suggested change
- <label class="setting-label">Enable Cache Storage</label>
+ <label class="setting-label" for="cache-storage-enabled">Enable Cache Storage</label>
<span class="setting-description">Store data locally in your browser for faster access</span>
@@
- <p-checkbox [(ngModel)]="settings.cacheStorageEnabled" [binary]="true" (onChange)="onSettingChange()">
+ <p-checkbox inputId="cache-storage-enabled" [(ngModel)]="settings.cacheStorageEnabled" [binary]="true" (onChange)="onSettingChange()">
</p-checkbox>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <label class="setting-label">Enable Cache Storage</label> | |
| <span class="setting-description">Store data locally in your browser for faster access</span> | |
| </div> | |
| <div class="setting-control"> | |
| <p-checkbox [(ngModel)]="settings.cacheStorageEnabled" [binary]="true" (onChange)="onSettingChange()"> | |
| </p-checkbox> | |
| <label class="setting-label" for="cache-storage-enabled">Enable Cache Storage</label> | |
| <span class="setting-description">Store data locally in your browser for faster access</span> | |
| </div> | |
| <div class="setting-control"> | |
| <p-checkbox inputId="cache-storage-enabled" [(ngModel)]="settings.cacheStorageEnabled" [binary]="true" (onChange)="onSettingChange()"> | |
| </p-checkbox> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@booklore-ui/src/app/features/settings/local-settings/local-settings.component.html`
around lines 28 - 33, The label for the "Enable Cache Storage" control is not
associated with the p-checkbox, so clicking the label won't toggle the checkbox
and screen readers lose the relationship; update the label element to include
for="cache-storage-enabled" and set the p-checkbox prop
inputId="cache-storage-enabled" (the p-checkbox bound to
settings.cacheStorageEnabled and using (onChange)="onSettingChange()") so the
visible label is correctly tied to the checkbox input for accessibility.
Comment on lines
+15
to
+17
| public readonly defaultSettings: LocalSettings = { | ||
| cacheStorageEnabled: true, | ||
| }; |
Contributor
There was a problem hiding this comment.
Make persistent content caching opt-in.
Line 16 enables Cache Storage for every device by default, and the downstream readers/services use that flag to persist full EPUB/PDF responses locally. That makes retained content and disk usage opt-out, which is risky on shared browsers. Please default this to false and let the new Local Settings UI enable it explicitly.
Suggested change
public readonly defaultSettings: LocalSettings = {
- cacheStorageEnabled: true,
+ cacheStorageEnabled: false,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public readonly defaultSettings: LocalSettings = { | |
| cacheStorageEnabled: true, | |
| }; | |
| public readonly defaultSettings: LocalSettings = { | |
| cacheStorageEnabled: false, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@booklore-ui/src/app/shared/service/local-settings.service.ts` around lines 15
- 17, The default LocalSettings currently enables persistent cache by setting
defaultSettings.cacheStorageEnabled to true; change this default to false so
cacheStorageEnabled is opt-in via the Local Settings UI (update the
defaultSettings: LocalSettings object and any code that relies on its initial
value such as consumers of LocalSettings to handle false as default). Ensure
references to defaultSettings and the cacheStorageEnabled property are updated
so the app treats persistence as disabled until the user explicitly enables it.
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
2 participants
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.
Description
This PR introduces:
Improvements:
This feature helps reducing traffic to the server, especially for large book files.
How does Cache Storage work?
When enabled, all EPUB, PDF content responses will be cached. Subsequent requests will reuse that cached response as long as the response is deemed valid (via a HEAD request with the If-Modified-Since header to the server).
Additional changes:
LocalSettingsfor device-scoped settings (to control Cache Storage behaviors)Testing
./gradlew testpassesng testpassesScreenshots
Local Settings UI
Summary by CodeRabbit
New Features
Bug Fixes & Improvements