feat: add achievement cache and progress bar#19
Conversation
- Cache achievement data locally using last_played as cache key - Show progress bar while fetching achievements - Skip API calls for games that haven't been played since last fetch - Cache stored in ~/.cache/steamfetch/achievements.json
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces achievement data caching to prevent redundant API calls. Adds a new cache module using persistent JSON storage, integrates cache-backed lookups into per-game achievement processing instead of batch aggregation, and displays a progress bar during game iteration. Changes
Sequence DiagramsequenceDiagram
participant Client as Achievement Processor
participant Cache as AchievementCache
participant API as Steam API
Client->>Cache: load()
Cache->>Cache: Read JSON from disk
loop For each game
Client->>Cache: get(appid, last_played)
alt Cache hit
Cache-->>Client: Return CachedAchievement
else Cache miss
Client->>API: Fetch achievements
API-->>Client: Return achievement data
Client->>Cache: set(appid, last_played, data)
end
end
Client->>Cache: save()
Cache->>Cache: Write JSON to disk
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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
🤖 Fix all issues with AI agents
In `@src/cache.rs`:
- Around line 28-35: The save method currently uses
serde_json::to_string(self).unwrap_or_default() which will yield an empty string
on serialization errors and then fs::write will overwrite the existing cache;
change save (the pub fn save) to first attempt serialization via
serde_json::to_string(self) and only call fs::write(path, data) when
serialization returns Ok(data); handle or log the Err variant (do not write
empty data), and keep the existing directory creation logic around cache_path()
and path.parent().
In `@src/steam/client.rs`:
- Around line 330-352: The cache lookup is unsafe for entries with
rtime_last_played == 0 because that value should be treated as "unknown" and can
cause permanent stale hits; update the cache logic to skip cache reads for those
cases by either adding a guard in the loop before calling cache.get (check
game.rtime_last_played == 0 and skip cache handling) or by modifying
AchievementCache::get(appid, last_played) to immediately return None when
last_played == 0 (so the rest of the existing logic is unchanged); ensure
references to CachedAchievement, total_achieved/total_possible accumulation, and
rarest candidate handling remain intact.
🧹 Nitpick comments (1)
src/steam/client.rs (1)
315-315: Unnecessary intermediateVecallocation.
all_gamescollects references just to iterate once. You can loop overgames.games.iter()directly.Proposed simplification
- let all_games: Vec<_> = games.games.iter().collect(); - - let pb = ProgressBar::new(all_games.len() as u64); + let pb = ProgressBar::new(games.games.len() as u64);And at line 330:
- for game in &all_games { + for game in &games.games {
| pub fn save(&self) { | ||
| if let Some(path) = cache_path() { | ||
| if let Some(parent) = path.parent() { | ||
| let _ = fs::create_dir_all(parent); | ||
| } | ||
| let _ = fs::write(path, serde_json::to_string(self).unwrap_or_default()); | ||
| } | ||
| } |
There was a problem hiding this comment.
unwrap_or_default() on serialization failure will silently wipe the cache file.
If serde_json::to_string(self) ever fails, unwrap_or_default() yields "", and fs::write overwrites the cache with an empty string — destroying all previously cached data. Guard the write behind a successful serialization instead.
Proposed fix
pub fn save(&self) {
if let Some(path) = cache_path() {
if let Some(parent) = path.parent() {
let _ = fs::create_dir_all(parent);
}
- let _ = fs::write(path, serde_json::to_string(self).unwrap_or_default());
+ if let Ok(json) = serde_json::to_string(self) {
+ let _ = fs::write(path, json);
+ }
}
}📝 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.
| pub fn save(&self) { | |
| if let Some(path) = cache_path() { | |
| if let Some(parent) = path.parent() { | |
| let _ = fs::create_dir_all(parent); | |
| } | |
| let _ = fs::write(path, serde_json::to_string(self).unwrap_or_default()); | |
| } | |
| } | |
| pub fn save(&self) { | |
| if let Some(path) = cache_path() { | |
| if let Some(parent) = path.parent() { | |
| let _ = fs::create_dir_all(parent); | |
| } | |
| if let Ok(json) = serde_json::to_string(self) { | |
| let _ = fs::write(path, json); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/cache.rs` around lines 28 - 35, The save method currently uses
serde_json::to_string(self).unwrap_or_default() which will yield an empty string
on serialization errors and then fs::write will overwrite the existing cache;
change save (the pub fn save) to first attempt serialization via
serde_json::to_string(self) and only call fs::write(path, data) when
serialization returns Ok(data); handle or log the Err variant (do not write
empty data), and keep the existing directory creation logic around cache_path()
and path.parent().
| for game in &all_games { | ||
| pb.inc(1); | ||
| let game_name = game | ||
| .name | ||
| .clone() | ||
| .unwrap_or_else(|| format!("App {}", game.appid)); | ||
|
|
||
| // Check cache first | ||
| if let Some(cached) = cache.get(game.appid, game.rtime_last_played) { | ||
| total_achieved += cached.achieved; | ||
| total_possible += cached.total; | ||
| if cached.achieved == cached.total && cached.total > 0 { | ||
| perfect_games += 1; | ||
| } | ||
| if let (Some(name), Some(percent)) = (&cached.rarest_name, cached.rarest_percent) { | ||
| rarest_candidates.push(RarestAchievement { | ||
| name: name.clone(), | ||
| game: game_name, | ||
| percent, | ||
| }); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Steam GetOwnedGames API rtime_last_played field documentation
💡 Result:
rtime_last_played is an optional per-game field in the IPlayerService/GetOwnedGames response (i.e., inside each games[] entry) returned by the Steam Web API endpoint. [1]
- Meaning: “last time the game was played”
- Format: Unix epoch timestamp (seconds, UTC) (example:
1677310875). [2] - Common edge case: it may be
0(e.g., for games never played / no last-played time available). [4] - Why it may be missing: in practice, Valve often only includes
rtime_last_played(and other “extra” fields likeplaytime_windows_forever, etc.) when you query your own account using your own Web API key, while other users’ results may omit it even if the library is public. [3]
Sources: [1] [2] [3] [4]
🏁 Script executed:
# Let's examine the cache implementation to understand the matching logic
find . -type f -name "*.rs" | head -20Repository: unhappychoice/steamfetch
Length of output: 225
🏁 Script executed:
# Search for the cache implementation
rg -t rust "AchievementCache" -A 10 -B 2Repository: unhappychoice/steamfetch
Length of output: 2049
🏁 Script executed:
# Look at the cache.get method implementation
rg -t rust "fn get\(" -A 5 | grep -A 5 "cache\|Cache"Repository: unhappychoice/steamfetch
Length of output: 334
🏁 Script executed:
# Check the file structure to find cache-related code
fd -e rs -type f | xargs grep -l "cache\|Cache" 2>/dev/null | head -10Repository: unhappychoice/steamfetch
Length of output: 238
🏁 Script executed:
# Examine the cache.rs file in detail to understand the full implementation
cat -n src/cache.rsRepository: unhappychoice/steamfetch
Length of output: 2285
🏁 Script executed:
# Check the CachedAchievement structure and how it's populated
rg -t rust "struct CachedAchievement" -A 10Repository: unhappychoice/steamfetch
Length of output: 511
🏁 Script executed:
# Look for where cache entries are inserted/stored to understand the populate flow
rg -t rust "cache\.save\|insert" -B 5 -A 5Repository: unhappychoice/steamfetch
Length of output: 50
🏁 Script executed:
# Check steam/models.rs to understand the Game struct and rtime_last_played field
cat -n src/steam/models.rs | head -100Repository: unhappychoice/steamfetch
Length of output: 2888
🏁 Script executed:
# Find where cache.set is called to understand the full cache populate flow
rg -t rust "cache\.set" -B 10 -A 3Repository: unhappychoice/steamfetch
Length of output: 928
🏁 Script executed:
# Look at the complete context around lines 330-352 in client.rs
sed -n '300,380p' src/steam/client.rsRepository: unhappychoice/steamfetch
Length of output: 2888
Bypass cache for games with rtime_last_played == 0 to prevent permanently stale achievement data.
When rtime_last_played is 0 (games never played or API field omitted), using it as a cache key creates an ambiguous state. If Valve releases new achievements for a game the player never plays, the cache will indefinitely serve stale achievement data since rtime_last_played remains 0.
Treat 0 as "unknown last-played time" and skip the cache for these entries:
Proposed fix in cache hit check
// Check cache first
+ if game.rtime_last_played != 0 {
if let Some(cached) = cache.get(game.appid, game.rtime_last_played) {
total_achieved += cached.achieved;
total_possible += cached.total;
if cached.achieved == cached.total && cached.total > 0 {
perfect_games += 1;
}
if let (Some(name), Some(percent)) = (&cached.rarest_name, cached.rarest_percent) {
rarest_candidates.push(RarestAchievement {
name: name.clone(),
game: game_name,
percent,
});
}
continue;
}
+ }Alternatively, centralize the guard in AchievementCache::get():
pub fn get(&self, appid: u32, last_played: u64) -> Option<&CachedAchievement> {
if last_played == 0 {
return None;
}
self.games
.get(&appid)
.filter(|c| c.last_played == last_played)
}🤖 Prompt for AI Agents
In `@src/steam/client.rs` around lines 330 - 352, The cache lookup is unsafe for
entries with rtime_last_played == 0 because that value should be treated as
"unknown" and can cause permanent stale hits; update the cache logic to skip
cache reads for those cases by either adding a guard in the loop before calling
cache.get (check game.rtime_last_played == 0 and skip cache handling) or by
modifying AchievementCache::get(appid, last_played) to immediately return None
when last_played == 0 (so the rest of the existing logic is unchanged); ensure
references to CachedAchievement, total_achieved/total_possible accumulation, and
rarest candidate handling remain intact.
- Show what's being fetched at each step (player, games, achievements) - Display cache vs fetch counts in achievement progress bar - Clear status messages after completion
1 participant
Summary
last_playedtimestamp as cache keyChanges
cache.rsmodule for achievement caching~/.cache/steamfetch/achievements.jsonindicatifcrate for progress bar displayrtime_last_playedfield to Game modelPerformance
Summary by CodeRabbit
New Features
Chores