feat(cache): add onCacheEvent lifecycle hook#30
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a cache lifecycle observability feature: a new ChangesCache Events Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Cache as cache.get
participant Storage
participant Hook as onCacheEvent
Caller->>Cache: get(key)
Cache->>Storage: read entry
Storage-->>Cache: entry / null
Cache->>Cache: evaluate expired/stale/validate
alt cache hit
Cache->>Hook: emit(hit)
else stale (SWR)
Cache->>Hook: emit(stale)
Cache->>Storage: refresh in background
Cache->>Hook: emit(set, reason)
else miss
Cache->>Hook: emit(miss)
Cache->>Storage: resolve and store
Cache->>Hook: emit(set, reason=initial)
end
Cache-->>Caller: value
sequenceDiagram
participant Caller
participant Invalidate as invalidateCache
participant Storage
participant Hook as onCacheEvent
Caller->>Invalidate: invalidateCache(key)
alt onCacheEvent provided
Invalidate->>Storage: read oldValue
end
Invalidate->>Storage: null out matching entries
alt entry removed
Invalidate->>Hook: emit(evict, reason=manual)
end
Invalidate-->>Caller: done
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (4)
src/http.ts (1)
205-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_integrityOptsstripping logic vs cache.ts.This helper is structurally identical to
_integrityOptsinsrc/cache.ts(stripsbase/group/name/onCacheEvent). SinceCachedEventHandlerOptionsstructurally extendsCacheOptions, consider exporting the cache.ts version and reusing it here instead of maintaining two copies of the same exclusion list — reduces risk of the lists drifting if another field needs excluding later.As per coding guidelines, "HTTP layer utilities (defineCachedHandler) must be implemented in http.ts and depend on cache.ts for core functionality."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http.ts` around lines 205 - 211, The _integrityOpts helper in src/http.ts duplicates the same field-stripping logic already present in cache.ts, so update defineCachedHandler to reuse the cache.ts implementation instead of maintaining a second copy. Export the shared _integrityOpts (or an equivalent cache utility) from cache.ts and call it from http.ts, keeping the HTTP-specific wiring in http.ts while depending on cache.ts for the core logic. Make sure the reused helper still strips base, group, name, and onCacheEvent from CachedEventHandlerOptions.Source: Coding guidelines
README.md (1)
289-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate heading: two
### \CacheEventType`` sections.Static analysis flags this as MD024 (no-duplicate-heading) since both the
CacheEventTypeconstant and its derived type share the same heading text. If this section is autogenerated (per the<!-- automd -->convention), regenerate viapnpm fmt/check the generator config to disambiguate (e.g., "CacheEventType (const)" vs "CacheEventType (type)") rather than hand-editing.As per coding guidelines, "Never touch contents inside
<!-- automd -->in README.md. They are auto-generated (usepnpm fmtto update)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 289 - 303, The README has duplicate `### CacheEventType` headings, so update the auto-generated documentation rather than editing the automd block by hand. Regenerate the README using the generator/format workflow so the `CacheEventType` constant and its derived type get distinct headings (for example, “CacheEventType (const)” and “CacheEventType (type)”), which will satisfy MD024.Sources: Coding guidelines, Linters/SAST tools
src/types.ts (1)
96-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider tightening
evict'soldValueto required.Every current call site (
cache.tsevict emits,invalidateCache) only fires anevictevent whenoldValue !== undefined, so consumers always receive a defined value in practice. Marking it optional (oldValue?: T) forces unnecessaryundefinedchecks downstream.✏️ Optional tightening
| { type: typeof CacheEventType.Evict; key: string; name: string; - oldValue?: T; + oldValue: T; reason: CacheEvictReason; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types.ts` around lines 96 - 115, Tighten the CacheEvent union so the Evict variant requires oldValue instead of making it optional, since cache.ts evict emissions and invalidateCache only emit this event when a value exists. Update the CacheEvent type definition in src/types.ts for the CacheEventType.Evict branch to make oldValue mandatory, and ensure any affected event creation sites or consumers referenced by CacheEvent and CacheEvictReason still align with the new required field.src/cache.ts (1)
114-127: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEager
validate()call when a hook is set changes call semantics.Without a hook,
validate(entry)is only invoked as the last operand of the||chain (short-circuited if any earlier condition is already true). With a hook enabled,_validateFailedcallsvalidate(entry)unconditionally, regardless ofshouldInvalidateCache/staleness/TTL/integrity already being true. This is called out in the inline comment, but ifvalidateis expensive or can throw on malformed entries, this now runs in more cases than before purely because a hook was attached. Consider wrapping this call so a throwingvalidatedoesn't newly break requests onceonCacheEventis added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.ts` around lines 114 - 127, Update the cache expiration logic in src/cache.ts so `validate(entry)` does not run eagerly just because `_hasHook` is true. In the `expired` decision path, keep the original short-circuit behavior by only invoking `validate` after all earlier invalidation checks fail, while still preserving hook-driven event handling. Use the existing symbols `_hasHook`, `_validateFailed`, and `expired` to locate the logic, and wrap the validate call so a throwing `validate` cannot newly affect requests when `onCacheEvent` is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cache.ts`:
- Around line 354-388: The `evict` path in `src/cache.ts` lets a failed
`storage.get()` during the `oldValue` lookup abort the whole invalidation when
`onCacheEvent` is set. Update the logic in this eviction function to make the
read for `oldValue` non-fatal by catching/ignoring read failures (or otherwise
isolating them) so `storage.set(k, null)` still runs. Keep the hook behavior in
`onCacheEvent`/`onError` unchanged, but ensure cache removal always proceeds
even if one tier read rejects.
---
Nitpick comments:
In `@README.md`:
- Around line 289-303: The README has duplicate `### CacheEventType` headings,
so update the auto-generated documentation rather than editing the automd block
by hand. Regenerate the README using the generator/format workflow so the
`CacheEventType` constant and its derived type get distinct headings (for
example, “CacheEventType (const)” and “CacheEventType (type)”), which will
satisfy MD024.
In `@src/cache.ts`:
- Around line 114-127: Update the cache expiration logic in src/cache.ts so
`validate(entry)` does not run eagerly just because `_hasHook` is true. In the
`expired` decision path, keep the original short-circuit behavior by only
invoking `validate` after all earlier invalidation checks fail, while still
preserving hook-driven event handling. Use the existing symbols `_hasHook`,
`_validateFailed`, and `expired` to locate the logic, and wrap the validate call
so a throwing `validate` cannot newly affect requests when `onCacheEvent` is
enabled.
In `@src/http.ts`:
- Around line 205-211: The _integrityOpts helper in src/http.ts duplicates the
same field-stripping logic already present in cache.ts, so update
defineCachedHandler to reuse the cache.ts implementation instead of maintaining
a second copy. Export the shared _integrityOpts (or an equivalent cache utility)
from cache.ts and call it from http.ts, keeping the HTTP-specific wiring in
http.ts while depending on cache.ts for the core logic. Make sure the reused
helper still strips base, group, name, and onCacheEvent from
CachedEventHandlerOptions.
In `@src/types.ts`:
- Around line 96-115: Tighten the CacheEvent union so the Evict variant requires
oldValue instead of making it optional, since cache.ts evict emissions and
invalidateCache only emit this event when a value exists. Update the CacheEvent
type definition in src/types.ts for the CacheEventType.Evict branch to make
oldValue mandatory, and ensure any affected event creation sites or consumers
referenced by CacheEvent and CacheEvictReason still align with the new required
field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 077f7e1d-6952-4b75-bbe1-bee5f58d2c07
📒 Files selected for processing (6)
README.mdsrc/cache.tssrc/http.tssrc/index.tssrc/types.tstest/index.test.ts
Emits hit/miss/stale/set/evict events with old/new value and a reason for observability (metrics, audit logging, cascading invalidation). Opt-in and side-effect free: no behavior change for callers that don't set the hook.
raminjafary
force-pushed
the
feat/cache-lifecycle-events
branch
from
19aaba5 to
0ad925e
Compare
1 participant
Adds an opt-in onCacheEvent hook for observing the cache lifecycle. Closes #19 and #13.
What it does
Tests: new cases covering every event type and reason, HTTP route naming, CacheEventType constants, hook-error isolation on both the get and invalidate paths, and regression guards (no-op invalidate emits nothing, fully-expired repopulation reports initial, no eager validate on the no-hook path, integrity unchanged when the hook is added). Full suite 115 passing; lint, typecheck, and build clean.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation