fix(cache): improve cache handling by checking availability before operations by balazs-szucs · Pull Request #397 · grimmory-tools/grimmory

balazs-szucs · 2026-04-05T20:59:38Z

Description

Linked Issue: Fixes #

Changes

This pull request improves the robustness of the CacheStorageService by adding checks to ensure the browser's Cache API is available before performing any cache operations. This prevents errors in environments where the Cache API is not supported, such as non-secure contexts or certain browsers.

Cache API availability checks:

Introduced a cacheAvailable getter to consistently determine if the Cache API is available (typeof caches !== 'undefined').
Updated all cache-related methods (getCache, match, has, put, delete, clear, getCacheSizeInBytes) to check for cacheAvailable and return safe fallback values when the Cache API is unavailable.
Modified the private cache() method to reject with a clear error if the Cache API is not available, preventing further cache operations in unsupported environments.
Ensured that cache writes (in getCache) only occur when the Cache API is available.

Summary by CodeRabbit

Bug Fixes
- Made caching more resilient when the browser Cache API is unavailable or throws errors; cache operations now fail safely without affecting app functionality.
- Cache reads return predictable fallback values when missing or invalid, reducing unexpected errors.
Refactor
- Simplified cache access and validation flow and removed verbose error logging for quieter, more stable runtime behavior.

…erations

coderabbitai · 2026-04-05T20:59:54Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb27728f-ff93-41f3-a68a-b38f997911fd

📥 Commits

Reviewing files that changed from the base of the PR and between 71b33c2 and 06bb812.

📒 Files selected for processing (1)

frontend/src/app/shared/service/cache-storage.service.ts

🚧 Files skipped from review as they are similar to previous changes (1)

frontend/src/app/shared/service/cache-storage.service.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Test Suite / Frontend Tests
GitHub Check: Test Suite / Backend Tests
GitHub Check: Analyze (java-kotlin)

📝 Walkthrough

Walkthrough

CacheStorageService now checks runtime availability of the Cache API and opens caches via openCache(): Promise<Cache | null>. Cache operations (match, has, put, delete, clear, getCacheSizeInBytes, attemptToGetAndValidateCache) return safe defaults and fail silently when the Cache API is missing or errors occur.

Changes

Cohort / File(s)	Summary
Cache Storage Service `frontend/src/app/shared/service/cache-storage.service.ts`	Replaced `cache(): Promise<Cache>` with `openCache(): Promise<Cache

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I peek if caches live or hide,
If not, I let the network guide.
Silent failures, safe default tune,
A little hop beneath the moon,
Robust and steady—cache is kind.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title follows the conventional commit format with 'fix' type and descriptive subject, clearly summarizing the cache API availability check improvements.
Description check	✅ Passed	The PR description includes the required template sections and provides comprehensive details about cache API availability checks, fallback handling, and implementation changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

✨ Simplify code

Create PR with simplified code

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (2)

frontend/src/app/shared/service/cache-storage.service.ts (2)
13-15: Prefer a single availability predicate.

cacheAvailable and cache() currently duplicate the same guard. Reusing the getter inside cache() would make it harder for the unsupported-environment behavior to drift later.

Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/app/shared/service/cache-storage.service.ts` around lines 13 -
15, The cache availability check is duplicated: reuse the private getter
cacheAvailable inside the cache() method (and any other places with the same
guard) instead of re-checking typeof caches !== 'undefined'; update cache() to
return null or throw consistently when cacheAvailable is false, and remove the
duplicated typeof caches check so all code paths rely on the single
cacheAvailable predicate (affecting the cache() method and the similar guard
around the other occurrence that currently mirrors lines 145-149).
17-44: Add coverage for the no-caches fallback.

This branch is now what keeps getCache() compatible with frontend/src/app/features/book/service/book-file.service.ts:30-38 and frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:610-622, both of which immediately consume the returned Response via .blob(). A focused test that stubs caches away and asserts the response is still readable would lock that in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/app/shared/service/cache-storage.service.ts` around lines 17 -
44, Add a unit/integration test covering the "no caches" fallback in getCache so
we don't regress the behavior used by BookFileService and PdfReader components:
stub global caches to undefined (e.g. (globalThis as any).caches = undefined),
call cacheStorageService.getCache(uri, /*noValidate*/ false) and immediately
consume the returned Response via .blob() (or .arrayBuffer()) to assert the body
is readable and not locked; also assert that cacheStorageService.put(...) is not
invoked when cacheAvailable is false/when caches is absent and that fetchFromUri
is used (spy on fetchFromUri and put). Target the getCache, fetchFromUri, put
and attemptToGetAndValidateCache symbols when adding the test to ensure the
fallback path works exactly as callers expect.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/src/app/shared/service/cache-storage.service.ts`:
- Around line 13-15: The cache availability check is duplicated: reuse the
private getter cacheAvailable inside the cache() method (and any other places
with the same guard) instead of re-checking typeof caches !== 'undefined';
update cache() to return null or throw consistently when cacheAvailable is
false, and remove the duplicated typeof caches check so all code paths rely on
the single cacheAvailable predicate (affecting the cache() method and the
similar guard around the other occurrence that currently mirrors lines 145-149).
- Around line 17-44: Add a unit/integration test covering the "no caches"
fallback in getCache so we don't regress the behavior used by BookFileService
and PdfReader components: stub global caches to undefined (e.g. (globalThis as
any).caches = undefined), call cacheStorageService.getCache(uri, /*noValidate*/
false) and immediately consume the returned Response via .blob() (or
.arrayBuffer()) to assert the body is readable and not locked; also assert that
cacheStorageService.put(...) is not invoked when cacheAvailable is false/when
caches is absent and that fetchFromUri is used (spy on fetchFromUri and put).
Target the getCache, fetchFromUri, put and attemptToGetAndValidateCache symbols
when adding the test to ensure the fallback path works exactly as callers
expect.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 38f226f5-8a3c-4c94-aa41-8d208bd9d42c

📥 Commits

Reviewing files that changed from the base of the PR and between 46d0655 and 71b33c2.

📒 Files selected for processing (1)

frontend/src/app/shared/service/cache-storage.service.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Test Suite / Frontend Tests
GitHub Check: Test Suite / Backend Tests
GitHub Check: Analyze (java-kotlin)

🧰 Additional context used

📓 Path-based instructions (2)

frontend/src/**/*.{ts,tsx,html,scss}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation in TypeScript, HTML, and SCSS files

Files:

frontend/src/app/shared/service/cache-storage.service.ts

frontend/src/app/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

frontend/src/app/**/*.{ts,tsx}: Prefer inject() over constructor injection
Follow frontend/eslint.config.js: component selectors use app-*, directive selectors use app*, and any is disallowed

Files:

frontend/src/app/shared/service/cache-storage.service.ts

🧠 Learnings (1)

📚 Learning: 2026-03-20T14:50:04.237Z

Learnt from: NamLe029
Repo: grimmory-tools/grimmory PR: 75
File: booklore-ui/src/app/shared/service/cache-storage.service.ts:38-45
Timestamp: 2026-03-20T14:50:04.237Z
Learning: In `booklore-ui/src/app/shared/service/cache-storage.service.ts`, the call `this.put(uri, res.clone())` inside `getCache()` is intentionally not awaited. This is a fire-and-forget design: the response is returned to the client immediately, and the cache write happens asynchronously in the background to avoid blocking the caller.

Applied to files:

frontend/src/app/shared/service/cache-storage.service.ts

🔇 Additional comments (1)

frontend/src/app/shared/service/cache-storage.service.ts (1)

104-141: Nice safe defaults for unsupported environments.

Returning resolved fallbacks from the public cache operations keeps callers out of capability-detection code and makes the unsupported-browser path much easier to reason about.

zachyale

@balazs-szucs Should we be modifying the frontend within settings to either disable cache settings or surface to the user the cache is unavailable to them in this case?

That way they don't get confused as to why it isn't working when enabled- otherwise, they may think it's a Grimmory bug instead of a client side restriction.

NamLe029 · 2026-04-06T02:32:24Z

Because you have already rejected the promise when cache is unavailable in cache() method, should we use Promise.catch() to handle that case instead of checking cacheAvailable?

balazs-szucs · 2026-04-06T08:25:16Z

@balazs-szucs Should we be modifying the frontend within settings to either disable cache settings or surface to the user the cache is unavailable to them in this case?

That way they don't get confused as to why it isn't working when enabled- otherwise, they may think it's a Grimmory bug instead of a client side restriction.

right now current behavior is one hiccup in the cache can make the reader "unavailable" or blank. Users do have some control over the browser cache, see reader settings, but generally, this is the kind of stuff they do not generally care about on a technical level, they are have some control over the cache on the browser side. I do not i have ever encountered a case where a random user asked "where is my cache" 🤣

i do not think this a big deal, it solves the problem, i can take a closer look later. But i think the browser cache is good, also browser are good managing caches i think, so generally, by and large there shouldn't be problems, and when there is just "fails" silently so there shouldn't be a huge problem on UX side.

Because you have already rejected the promise when cache is unavailable in cache() method, should we use Promise.catch() to handle that case instead of checking cacheAvailable?

I think so yeah, I'll see what i can do.

NamLe029 · 2026-04-06T08:49:21Z

Initially, when i designed this, i have thought of the risk of caching failure and had some protection for it.

The attemptToGetAndValidateCache() method does not throw and returns null on any failures. Inserting (putting) cache is also done asynchronously and have a .catch in case of error.
So the only part that could fail is fetching the server and constructing a Response from angular HttpResponse.

But that's only for the getCache method, there is none for the others.

balazs-szucs · 2026-04-07T09:03:58Z

Initially, when i designed this, i have thought of the risk of caching failure and had some protection for it.

The attemptToGetAndValidateCache() method does not throw and returns null on any failures. Inserting (putting) cache is also done asynchronously and have a .catch in case of error. So the only part that could fail is fetching the server and constructing a Response from angular HttpResponse.

But that's only for the getCache method, there is none for the others.

Thanks, i think with 06bb812
this should be reasonable. Any thought? Or can this go though?

NamLe029 · 2026-04-07T09:12:18Z

looks good to me

…erations (grimmory-tools#397) * fix(cache): improve cache handling by checking availability before operations * fix(cache): improve cache handling and improve error management

…erations (#397) * fix(cache): improve cache handling by checking availability before operations * fix(cache): improve cache handling and improve error management

…erations (grimmory-tools#397) * fix(cache): improve cache handling by checking availability before operations * fix(cache): improve cache handling and improve error management

fix(cache): improve cache handling by checking availability before op…

71b33c2

…erations

balazs-szucs changed the title ~~fix(cache): improve cache handling by checking availability before op…~~ fix(cache): improve cache handling by checking availability before operations Apr 5, 2026

coderabbitai Bot added enhancement frontend labels Apr 5, 2026

coderabbitai Bot reviewed Apr 5, 2026

View reviewed changes

zachyale reviewed Apr 5, 2026

View reviewed changes

fix(cache): improve cache handling and improve error management

06bb812

zachyale approved these changes Apr 7, 2026

View reviewed changes

balazs-szucs merged commit 04c46ba into grimmory-tools:develop Apr 7, 2026
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cache): improve cache handling by checking availability before operations#397

fix(cache): improve cache handling by checking availability before operations#397
balazs-szucs merged 2 commits into
grimmory-tools:developfrom
balazs-szucs:cache-fix

balazs-szucs commented Apr 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 5, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Uh oh!

zachyale left a comment

Uh oh!

NamLe029 commented Apr 6, 2026

Uh oh!

balazs-szucs commented Apr 6, 2026

Uh oh!

NamLe029 commented Apr 6, 2026

Uh oh!

balazs-szucs commented Apr 7, 2026

Uh oh!

NamLe029 commented Apr 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

balazs-szucs commented Apr 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

zachyale left a comment

Choose a reason for hiding this comment

Uh oh!

NamLe029 commented Apr 6, 2026

Uh oh!

balazs-szucs commented Apr 6, 2026

Uh oh!

NamLe029 commented Apr 6, 2026

Uh oh!

balazs-szucs commented Apr 7, 2026

Uh oh!

NamLe029 commented Apr 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

balazs-szucs commented Apr 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 5, 2026 •

edited

Loading