fix: improve error handling for API failures#22
Conversation
- Add retry logic with exponential backoff (3 retries, 500ms/1s/2s) - Detect and explain private profile errors with guidance - Handle rate limiting (HTTP 429) with clear message and retry - Show partial data when steam level or recently played fails - Add SteamApiError custom error type for structured error handling - Add --timeout flag for configurable request timeout (default 30s) - Add unit tests for error detection and top games extraction Closes #5
📝 WalkthroughWalkthroughRefactors CLI to carry a new Changes
Sequence DiagramsequenceDiagram
participant App as "App (main / CLI)"
participant Client as "SteamClient (HTTP)"
participant Retry as "Retry Handler"
participant API as "Steam API"
participant Detector as "Error Detector"
participant Result as "Result/Parser"
App->>Client: build client (timeout from CLI)
App->>Retry: request_with_retry(endpoint, retries)
Retry->>Client: send HTTP request (with timeout)
Client->>API: HTTP request
alt 200 OK
API-->>Client: 200 + body
Client->>Result: parse body
Result-->>App: SteamStats
else 429 / Rate limit
API-->>Client: 429
Client->>Retry: classify as RateLimited
Retry->>Retry: backoff & retry
else Timeout / Network
Client--xAPI: no response / error
Client->>Retry: classify as Timeout/NetworkError
Retry->>Retry: backoff & retry
else 4xx/5xx
API-->>Client: error
Client->>Detector: detect_api_error / detect_private_profile
Detector-->>App: SteamApiError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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: 3
🤖 Fix all issues with AI agents
In `@src/main.rs`:
- Around line 33-36: The CLI currently allows --timeout 0 for the timeout field
(timeout: u64) which yields a zero-duration and makes requests fail immediately;
update the argument parsing for timeout to enforce a minimum of 1 second (either
by adding clap's value_parser with a range like
clap::value_parser!(u64).range(1..) on the timeout field or by adding a small
runtime check after parsing that rejects values < 1 and exits with a clear
error), and ensure the error message references the timeout option so users know
to supply a positive integer.
In `@src/steam/client.rs`:
- Around line 561-568: The current build_http_client function silently falls
back to Client::new() (losing the configured timeout) when
Client::builder().build() fails; change this to propagate the failure instead of
silently ignoring it by removing the unwrap_or_else fallback and using expect
(or propagate the Result) on Client::builder().build() so the error is surfaced
(e.g., call Client::builder().timeout(timeout)...build().expect("failed to build
reqwest Client with timeout")). This keeps the configured timeout and avoids
silent degradation in build_http_client.
- Around line 525-531: The current verbose logging slices the response body by
bytes using &body[..body.len().min(500)] which can panic on UTF-8 multi-byte
boundaries; change the truncation in the eprintln block (the code guarded by
self.verbose that prints context and body) to compute a character-boundary-safe
end index (e.g., use str::floor_char_boundary(body, 500) on Rust >=1.91 or
compute the last char boundary with body.char_indices().take_while(|(i, _)| *i <
500).last().map_or(0, |(i, c)| i + c.len_utf8())) and then slice with that index
so eprintln prints a valid UTF-8 substring without risking a panic.
🧹 Nitpick comments (5)
src/steam/error.rs (2)
49-55:is_retryable()doesn't cover server errors, butrequest_with_retryretries them anyway.In
client.rs(lines 511–518), server errors (5xx) setlast_errorandcontinue, effectively retrying. However,is_retryable()returnsfalseforApiError. This inconsistency meansis_retryable()doesn't accurately describe retry behavior, which can mislead future callers.Consider either adding
ApiErrorwith a server-error guard tois_retryable, or usingis_retryable()as the single source of truth inrequest_with_retryinstead of hard-coding the retry decision for each status category.
1-47: Consider usingthiserrorto reduce boilerplate.The project already depends on
thiserrorv2.0.0 (perCargo.toml). Using#[derive(thiserror::Error)]with#[error("...")]attributes would eliminate the manualDisplayandErrorimpls while keeping the same behavior.src/steam/client.rs (3)
476-558: Retry loop: theis_retryable()check on network errors (line 542) is always true — theelsebranch at line 550 is dead code.Within the
Err(e)arm (lines 535–551),api_erroris always eitherTimeoutorNetworkError, both of which returntruefromis_retryable(). Thereturn Err(api_error.into())at line 550 is unreachable. This isn't a bug, but it's misleading.If the intent is to guard against future non-retryable network error variants, a comment would suffice. Otherwise, the branch can be removed to simplify the code.
438-474: Achievement endpoints bypassrequest_with_retry— no retry or error classification.
fetch_player_achievementsandfetch_global_percentagesstill callself.client.get().send().awaitdirectly, so they don't benefit from the new retry logic or structured error handling. This is likely acceptable since achievement fetching is already best-effort (wrapped inOption), but it means transient failures on achievements won't be retried.Worth noting for future consideration if achievement fetching becomes more critical.
584-620:detect_api_erroranddetect_private_profilerely on fragile string matching.Both functions use
body.contains(...)to classify errors from raw JSON. This works today but is brittle — whitespace variations, key ordering changes, or localized responses could break detection. Since the body is already being parsed as JSON elsewhere, consider parsing once and inspecting the structured data instead of substring matching.For
detect_api_errorspecifically, the"Forbidden"/"Access is denied"check (line 589) overlaps with the HTTP 403 check already handled inrequest_with_retry(line 507), which makes this path mainly a fallback for non-403 body content.
- Validate --timeout with value_parser range to reject 0 - Fix UTF-8 panic in verbose body truncation (floor_char_boundary) - Use expect() in build_http_client to surface build errors - Use thiserror derive for SteamApiError to reduce boilerplate - Add server errors (5xx) to is_retryable() for consistency - Unify retry logic via is_retryable() as single source of truth - Remove dead code branch in network error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/steam/client.rs`:
- Around line 508-511: The call to str::floor_char_boundary (used in the
verbose-response truncation inside the block checking self.verbose and body)
requires Rust 1.91; either declare rust-version = "1.91" in Cargo.toml to set
the MSRV, or replace floor_char_boundary with a compatibility-safe truncation:
compute the byte index of the 500th character using body.char_indices() (or
fallback to body.len()) and slice with that byte index to avoid splitting UTF-8
characters. Ensure you update the truncation logic where truncated =
&body[..body.floor_char_boundary(500)] is used.
🧹 Nitpick comments (1)
src/steam/client.rs (1)
593-614: Private-profile detection relies on fragile heuristics and can false-positive.A user who genuinely owns zero games will trigger
PrivateProfileif the response body lacks a"games"key (which Steam omits when the list is empty). The parse-error branch (line 607) is similarly ambiguous —"game_count":0without"games"can be a normal empty response, not necessarily a private profile.Consider logging a verbose hint when the heuristic fires so users who actually own zero games aren't stuck:
Suggested improvement
Ok(resp) if resp.response.games.is_empty() && resp.response.game_count == 0 => { - // Could be a private profile or truly no games. - // Check if the response body looks like a minimal/empty response if !body.contains("\"games\"") { + // Note: this also triggers for accounts that genuinely own zero games return Err(SteamApiError::PrivateProfile.into()); } Ok(())This is acceptable for now since zero-game accounts are rare, but worth a comment to flag the known limitation.
| if self.verbose { | ||
| let truncated = &body[..body.floor_char_boundary(500)]; | ||
| eprintln!("[verbose] {} response body: {}", context, truncated); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MSRV or rust-version is specified in Cargo.toml or rust-toolchain
echo "=== Cargo.toml ==="
cat Cargo.toml | grep -i -E 'rust-version|edition|msrv' || echo "No MSRV found in Cargo.toml"
echo ""
echo "=== rust-toolchain* ==="
fd 'rust-toolchain' --max-depth 1 --exec cat {} \; || echo "No rust-toolchain file found"
echo ""
echo "=== Full Cargo.toml [package] section ==="
sed -n '/^\[package\]/,/^\[/p' Cargo.toml | head -30Repository: unhappychoice/steamfetch
Length of output: 521
🌐 Web query:
str::floor_char_boundary Rust stabilized version 1.91
💡 Result:
str::floor_char_boundary was stabilized in Rust 1.91.0 (and it is also const-stable starting in 1.91.0). It shipped with the Rust 1.91.0 release on October 30, 2025. [1] [2]
Sources: Rust 1.91.0 release announcement (lists str::floor_char_boundary as newly stabilized) [1], Rust standard library docs for str::floor_char_boundary (shows 1.91.0 (const: 1.91.0)) [2].
Add rust-version = "1.91" to Cargo.toml or avoid using floor_char_boundary.
The project does not specify an MSRV in Cargo.toml. Since the code uses str::floor_char_boundary (stabilized in Rust 1.91.0), either declare the minimum supported version or replace with a compatibility-friendly alternative.
🤖 Prompt for AI Agents
In `@src/steam/client.rs` around lines 508 - 511, The call to
str::floor_char_boundary (used in the verbose-response truncation inside the
block checking self.verbose and body) requires Rust 1.91; either declare
rust-version = "1.91" in Cargo.toml to set the MSRV, or replace
floor_char_boundary with a compatibility-safe truncation: compute the byte index
of the 500th character using body.char_indices() (or fallback to body.len()) and
slice with that byte index to avoid splitting UTF-8 characters. Ensure you
update the truncation logic where truncated =
&body[..body.floor_char_boundary(500)] is used.
1 participant
Summary
Improve error handling to provide better user experience when API calls fail. Closes #5.
Changes
SteamApiErrorcustom error type for structured error handling (PrivateProfile, RateLimited, Timeout, NetworkError, InvalidApiKey, PlayerNotFound, ApiError)--timeout <SECONDS>CLI flag for configurable request timeout (default: 30s)Checklist from #5
--verboseflag for detailed error infoSummary by CodeRabbit
New Features
Bug Fixes
Tests