Repository navigation
fix(infra): include the error source chain in soloist download failures - #958
Conversation
The download warning only showed reqwest's top-level text like 'error decoding response body', hiding whether the cause was a connection reset, the read timeout, or a cut stream. failed() now walks error.source() and joins every level into the message, which also reaches the settings status line. Part of LargeModGames#950 Signed-off-by: niukanen1 <57656076+niukanen1@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Format and verify download errors src/infra/soloist/download.rs, CHANGELOG.md |
failed appends each source error’s display text, joined with : . Static failure cases construct Failure::Failed directly. Tests cover chained and source-free errors. The changelog records the warning change. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
- Soloist download failure logs no underlying cause #950: The issue reports that Soloist logs omit the underlying cause. This change includes source errors in download warnings.
Possibly related PRs
- LargeModGames/spotatui#935: This PR introduced the download failure helper and paths that this change updates.
Merge Risk: 
ec2f9
-
The change adds error-source details to download failures. No merge-blocking issue was identified.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the conventional-commit prefix "fix" with the scope "infra", which matches the allowed prefixes. It is concise and imperative. It names the main change: download failure messages now in… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title uses the conventional-commit prefix "fix" with the scope "infra", which matches the allowed prefixes. It is concise and imperative. It names the main change: download failure messages now include the error source chain. The raw summary and PR objectives confirm that the change does exactly this in src/infra/soloist/download.rs.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
✨ Simplify code
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A failed Soloist download only logged
error decoding response body: reqwest'sDisplaykeeps the top-level text and hideserror.source(), so the warning did not say whether the cause was a connection reset, the 30s read timeout, or a cut stream.failed()insrc/infra/soloist/download.rsnow takesstd::error::Errorand walks the source chain, joining every level with": ". The full chain reaches both the download warning and the settings status line, which interpolates the message as before. The three call sites that built the message from plain strings now constructFailure::Faileddirectly.The automatic retry for body read errors suggested in the issue is left as a follow-up; this change is the diagnostic half.
Tests: two unit tests in the download module cover the chain walk with a fake two-level error (the three-level string
error decoding response body: error reading a body from connection: connection reset by peer, which fails on the currentto_string()conversion) and the no-source case that must not gain a trailing separator.Part of #950
Summary by CodeRabbit