Cached fetched lyrics on disk so repeat lookups load instantly - #169
Conversation
Added a persistent LyricsCache (JSON in the Caches dir, LRU-capped) keyed by normalized title+artist+album. Successful lookups are served from the cache on a re-open or after relaunch without hitting the providers; misses stay retryable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011q78vsxxcRkYYvN61cCjhK
|
Warning Review limit reached
Next review available in: 52 minutes Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable usage-based reviews in Billing to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). 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 see our Fair Usage Limits Policy for further information, and refer to the rate limits docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new ChangesPersistent LRU Lyrics Cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
IntelliNestTests/LyricsApiServiceTests.swift (1)
117-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the cached lookup makes zero provider requests.
Changing stubs to misses proves the result survives, but a network-first implementation that falls back to cache could still pass. Add an inverted request expectation around the second fetch.
Suggested test tightening
// The provider now misses; a cached track must still resolve from the cache // rather than fall through to the (now empty) network result. stub(lrclibGetURL(duration: 233), json: "{}", statusCode: 404) stub(lrclibSearchURL(), json: "[]") stub(lyricsOvhURL(), json: "{}", statusCode: 404) + let unexpectedRequest = expectation(description: "cached lookup should not hit providers") + unexpectedRequest.isInverted = true + let providerURLs = Set([lrclibGetURL(duration: 233), lrclibSearchURL(), lyricsOvhURL()]) + URLProtocolStub.observerRequests { request in + if let url = request.url, providerURLs.contains(url) { + unexpectedRequest.fulfill() + } + } let second = await service.fetchLyrics(title: title, artist: artist, album: nil, durationSeconds: 233) XCTAssertEqual(second, first, "a cached hit is served without re-fetching") + await fulfillment(of: [unexpectedRequest], timeout: 0.2)🤖 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 `@IntelliNestTests/LyricsApiServiceTests.swift` around lines 117 - 123, The second-fetch cache test in LyricsApiServiceTests should verify there are zero provider requests, not just that the cached result matches. Tighten the test around fetchLyrics(title:artist:album:durationSeconds:) by adding an inverted request expectation for the lrclibGetURL, lrclibSearchURL, and lyricsOvhURL stubs before calling the second fetch, so a network-first fallback cannot still pass. Keep the existing cached equality assertion to confirm the returned value still comes from the cache.
🤖 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 `@IntelliNest/Services/LyricsCache.swift`:
- Around line 104-112: The injected cache path in LyricsCache.persist() can fail
silently because the parent directory may not exist before writing the encoded
data. Update the persist flow in LyricsCache, ideally right after resolving
fileURL and before data.write, to create the target directory (using the
injected directory URL) if needed, then proceed with encoding and atomic write.
Keep the fix localized to persist() so the injected-directory path behaves the
same as the default cache path.
In `@IntelliNestTests/LyricsApiServiceTests.swift`:
- Around line 152-155: The test setup in setUpWithError for
LyricsApiServiceTests uses a fixed lyrics-cache-tests temp path, which can
collide across overlapping runs. Change the directory creation logic to generate
a unique temporary directory per test run (for example by incorporating a UUID
or similar unique suffix) and keep the cleanup in the same setup path so the
fixture remains isolated and stable.
---
Nitpick comments:
In `@IntelliNestTests/LyricsApiServiceTests.swift`:
- Around line 117-123: The second-fetch cache test in LyricsApiServiceTests
should verify there are zero provider requests, not just that the cached result
matches. Tighten the test around
fetchLyrics(title:artist:album:durationSeconds:) by adding an inverted request
expectation for the lrclibGetURL, lrclibSearchURL, and lyricsOvhURL stubs before
calling the second fetch, so a network-first fallback cannot still pass. Keep
the existing cached equality assertion to confirm the returned value still comes
from the cache.
🪄 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: 9d36b7eb-f233-43fb-804c-3e4ecd386e86
📒 Files selected for processing (5)
IntelliNest.xcodeproj/project.pbxprojIntelliNest/Model/LyricsTimeline.swiftIntelliNest/Services/LyricsApiService.swiftIntelliNest/Services/LyricsCache.swiftIntelliNestTests/LyricsApiServiceTests.swift
Created the injected cache directory before persisting so an injected path can't silently drop the cache. Gave each persistence test its own temp subdirectory (keyed by the test name, not a runtime UUID) so fixtures can't collide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011q78vsxxcRkYYvN61cCjhK
1 participant
What
Follow-up to the lyrics work (#168): lyrics now persist in a cache so a re-opened or
replayed track loads instantly — no network round-trip, even across app launches.
Before this, only the currently playing track was latched (
lyricsTrackKey); flippingto a song viewed earlier refetched from the providers.
How
LyricsCache(Services/LyricsCache.swift): anactorholding an in-memory mapbacked by a single JSON file in the Caches directory.
title + artist + album(lowercased/trimmed), so trivialcase/spacing differences don't fragment the cache.
.notFoundstays retryable, matching theexisting fetch-side latch.
try?); a missing/corrupt file just starts empty.LyricsApiServiceconsults the cache before the provider chain and stores the resultafter; injectable so tests/previews use a memory-only cache (
directory: nil).LyricsResult/LyricLineCodablefor persistence.Tests
LyricsCacheTests— store/return,.notFoundnot cached, key normalization, LRUeviction, and persistence across instances (relaunch).
LyricsApiServiceTests— a second lookup is served from the cache even after theproviders start missing; existing tests use a memory-only cache.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes