feat(jef-117): extend backend caching to User, Skill, Education, and WorkExperience - #289
Conversation
…WorkExperience Wraps four previously-uncached repositories in the same cache-aside pattern already used for notes/interview rounds/applications: - CachedUserRepository caches findById and findByEmail — findById backs UserResolver.getMe(), which the web app calls on effectively every authenticated page load. Since a user has two natural cache keys (id and email) and email can change via update(), invalidation tracks an id→email map so update() can clear both the old and new cached email entries, not just whichever is convenient. create() also defensively invalidates the target email's cache, since RegisterUseCase/OAuth linking call findByEmail() first to check for duplicates and may have cached a stale "not found" result. findAll() stays uncached — it's only a once-a-week batch read in the digest cron, not a hot path. - CachedSkillRepository, CachedEducationRepository, CachedWorkExperienceRepository cache findAllByUserId/findById, direct ports of CachedNoteRepository — same per-owner child-entity shape as Notes/Contacts, just keyed by userId. Raw Drizzle repos for these four are re-registered under drizzleUserRepository/drizzleSkillRepository/drizzleEducationRepository/drizzleWorkExperienceRepository in the DI container so the cached decorators can wrap them, while the outward-facing Cradle keys (userRepository/skillRepository/educationRepository/workExperienceRepository) are unchanged — no consumer/resolver files needed edits.
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdded cache-aside decorators for four repositories. Added cache-key builders, mutation invalidation, ownership tracking, container wiring, and comprehensive Vitest coverage. ChangesRepository caching
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant CachedUserRepository
participant Cache
participant DrizzleUserRepository
Client->>CachedUserRepository: findById or findByEmail
CachedUserRepository->>Cache: read lookup key
CachedUserRepository->>DrizzleUserRepository: fetch on cache miss
DrizzleUserRepository-->>CachedUserRepository: return user or null
CachedUserRepository->>Cache: store lookup result
CachedUserRepository-->>Client: return user or null
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 (2)
apps/api/src/__tests__/infrastructure/cache/CachedUserRepository.test.ts (1)
146-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test to match its assertion.
The name states "does not redundantly invalidate the email cache", but the assertion confirms the opposite: the email entry is invalidated and the next read reaches the inner repository. The inline comment already notes this. Rename the test to describe the checked behaviour, for example "invalidates the email cache even when the email is unchanged".
🤖 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 `@apps/api/src/__tests__/infrastructure/cache/CachedUserRepository.test.ts` around lines 146 - 160, Rename the test case around repo.update and the subsequent findByEmail call to describe that the email cache is invalidated even when the email is unchanged, matching the existing assertion and inline comment.apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts (1)
12-14: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBound the ownership-tracking maps in the four cached decorators.
Each new decorator keeps an auxiliary
Mapthat records ownership sodelete()can invalidate the correct list or email key. TheMemoryCacheentries expire after the TTL, but these maps have no TTL. Entries are added on every cache miss and removed only bydelete(). The container registers all four decorators asLifetime.SINGLETONatapps/api/src/http/container.tslines 503-508, so the maps live for the whole process and grow with the number of distinct records the API has served.The same pattern already exists in
CachedApplicationRepositoryandCachedNoteRepository, so this is not a regression introduced here. Consider a shared bounded structure, for example an LRU map with a size cap, or delete the map entry when the corresponding cache key expires. A size cap is safe because a missing map entry only skips a list invalidation that the TTL will resolve.
apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts#L12-L14: boundemailByUserId.apps/api/src/infrastructure/db/repositories/CachedSkillRepository.ts#L16-L17: bounduserIdBySkillId.apps/api/src/infrastructure/db/repositories/CachedEducationRepository.ts#L16-L17: bounduserIdByEducationId.apps/api/src/infrastructure/db/repositories/CachedWorkExperienceRepository.ts#L16-L17: bounduserIdByWorkExperienceId.🤖 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 `@apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts` around lines 12 - 14, Bound the ownership-tracking maps used by the four cached decorators so they cannot grow without limit, using a shared or equivalent capped/LRU structure while preserving existing invalidation behavior. Update emailByUserId in apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts (lines 12-14), userIdBySkillId in apps/api/src/infrastructure/db/repositories/CachedSkillRepository.ts (lines 16-17), userIdByEducationId in apps/api/src/infrastructure/db/repositories/CachedEducationRepository.ts (lines 16-17), and userIdByWorkExperienceId in apps/api/src/infrastructure/db/repositories/CachedWorkExperienceRepository.ts (lines 16-17); missing ownership entries must safely skip the corresponding list invalidation.
🤖 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 `@apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts`:
- Around line 106-109: Update CachedUserRepository.updateLastDigestSentAt to
also invalidate the cached userByEmail entry using the email available through
emailByUserId, matching the cache invalidation performed by delete() and
update().
---
Nitpick comments:
In `@apps/api/src/__tests__/infrastructure/cache/CachedUserRepository.test.ts`:
- Around line 146-160: Rename the test case around repo.update and the
subsequent findByEmail call to describe that the email cache is invalidated even
when the email is unchanged, matching the existing assertion and inline comment.
In `@apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts`:
- Around line 12-14: Bound the ownership-tracking maps used by the four cached
decorators so they cannot grow without limit, using a shared or equivalent
capped/LRU structure while preserving existing invalidation behavior. Update
emailByUserId in
apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts (lines
12-14), userIdBySkillId in
apps/api/src/infrastructure/db/repositories/CachedSkillRepository.ts (lines
16-17), userIdByEducationId in
apps/api/src/infrastructure/db/repositories/CachedEducationRepository.ts (lines
16-17), and userIdByWorkExperienceId in
apps/api/src/infrastructure/db/repositories/CachedWorkExperienceRepository.ts
(lines 16-17); missing ownership entries must safely skip the corresponding list
invalidation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dce6770-0a8c-453d-9ec6-4d1bb1e8c78c
📒 Files selected for processing (10)
apps/api/src/__tests__/infrastructure/cache/CachedEducationRepository.test.tsapps/api/src/__tests__/infrastructure/cache/CachedSkillRepository.test.tsapps/api/src/__tests__/infrastructure/cache/CachedUserRepository.test.tsapps/api/src/__tests__/infrastructure/cache/CachedWorkExperienceRepository.test.tsapps/api/src/constants.tsapps/api/src/http/container.tsapps/api/src/infrastructure/db/repositories/CachedEducationRepository.tsapps/api/src/infrastructure/db/repositories/CachedSkillRepository.tsapps/api/src/infrastructure/db/repositories/CachedUserRepository.tsapps/api/src/infrastructure/db/repositories/CachedWorkExperienceRepository.ts
| async updateLastDigestSentAt(id: string, sentAt: Date): Promise<void> { | ||
| await this.inner.updateLastDigestSentAt(id, sentAt); | ||
| this.cache.delete(CACHE_KEYS.userById(id)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Invalidate the byEmail entry in updateLastDigestSentAt.
update() clears both userById and userByEmail. updateLastDigestSentAt() clears only userById. A cached userByEmail entry therefore keeps a stale lastDigestSentAt until the TTL expires. The stored email is available in emailByUserId, so the fix is symmetric with delete().
🐛 Proposed fix
async updateLastDigestSentAt(id: string, sentAt: Date): Promise<void> {
await this.inner.updateLastDigestSentAt(id, sentAt);
this.cache.delete(CACHE_KEYS.userById(id));
+ const email = this.emailByUserId.get(id);
+ if (email) this.cache.delete(CACHE_KEYS.userByEmail(email));
}📝 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.
| async updateLastDigestSentAt(id: string, sentAt: Date): Promise<void> { | |
| await this.inner.updateLastDigestSentAt(id, sentAt); | |
| this.cache.delete(CACHE_KEYS.userById(id)); | |
| } | |
| async updateLastDigestSentAt(id: string, sentAt: Date): Promise<void> { | |
| await this.inner.updateLastDigestSentAt(id, sentAt); | |
| this.cache.delete(CACHE_KEYS.userById(id)); | |
| const email = this.emailByUserId.get(id); | |
| if (email) this.cache.delete(CACHE_KEYS.userByEmail(email)); | |
| } |
🤖 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 `@apps/api/src/infrastructure/db/repositories/CachedUserRepository.ts` around
lines 106 - 109, Update CachedUserRepository.updateLastDigestSentAt to also
invalidate the cached userByEmail entry using the email available through
emailByUserId, matching the cache invalidation performed by delete() and
update().
…ackend-caching # Conflicts: # apps/api/src/constants.ts # apps/api/src/http/container.ts
|
Merged `main` in to resolve the conflict caused by JEF-116/#287 landing after this branch was cut — both touched `constants.ts` (additive, kept both sets of `CACHE_KEYS`) and `container.ts` (both renamed/added Cradle registrations; merged so all 7 cached repos from #287 plus the 4 from this PR coexist under their final `drizzleXxxRepository`/`CachedXxxRepository` names). Also picked up `IUserRepository.findByBackupEmail` from JEF-36/#283 (also merged to main in the interim) — added it to `CachedUserRepository` as an uncached passthrough, since its only two call sites (backup-email recovery request, add-backup-email verification) are already rate-limited and low-frequency, not worth a third cache key. Re-verified clean typecheck/lint, and the full test suite (1122/1124 — same 2 pre-existing flaky tests as before, both pass individually in isolation). |
1 participant
Summary
User,Skill,Education,WorkExperience.CachedUserRepositorycachesfindByIdandfindByEmail.findByIdbacksUserResolver.getMe(), which the web app calls on effectively every authenticated page load — the single most-read row in the system after the ApiToken hash lookup from JEF-116.idandemail), andemailcan change viaupdate()(email-change flow), so invalidation tracks anid → emailmap —update()clears both the old and new cached email entries, not just whichever is convenient.create()also defensively invalidates the target email's cache entry, sinceRegisterUseCase/OAuth account linking callfindByEmail()first to check for a duplicate and may have cached a stale "not found" result that would otherwise hide the freshly-created user.findAll()stays uncached — it's a once-a-week batch read from the digest cron, not a hot path.updateLastDigestSentAt()invalidatesuserByIdafter delegating, since it changes a cached field (unlikeApiToken.updateLastUsedfrom JEF-116, this only fires once a week per user, so invalidating costs nothing).CachedSkillRepository,CachedEducationRepository,CachedWorkExperienceRepositorycachefindAllByUserId/findById— direct ports ofCachedNoteRepository, same per-owner child-entity shape as Notes/Contacts, just keyed byuserId.drizzleUserRepository/drizzleSkillRepository/drizzleEducationRepository/drizzleWorkExperienceRepository. The outward-facing Cradle keys (userRepository/skillRepository/educationRepository/workExperienceRepository) are unchanged, so no consumer/resolver/use-case files needed any edits.CACHE_KEYSentries:userById,userByEmail,skillById,skillList,educationById,educationList,workExperienceById,workExperienceList.Implements the plan from JEF-117. Independent of JEF-116/#287 (this branch is based on
main, not stacked on that PR).Test plan
pnpm --filter @job-finder/api typecheck— cleanpnpm --filter @job-finder/api test— 1063/1063 passpnpm --filter @job-finder/api lint— clean (one pre-existing unrelated warning inschema.ts)CachedUserRepository.update()invalidates the old cached email, not just the new one🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests