refactor: DRY flake metadata extraction and ref-statuses loading#232
refactor: DRY flake metadata extraction and ref-statuses loading#232Malix-Labs wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThree public accessor methods ( ChangesNode Accessor Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| ]; | ||
|
|
There was a problem hiding this comment.
this one was basically duplicated, i assume by mistake?
| static CEL_MARKDOWN_TEMPLATE: &str = include_str!(concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/templates/summary.cel.md.hbs" | ||
| )); | ||
| static CEL_MARKDOWN_TEMPLATE: &str = include_str!("templates/summary.cel.md.hbs"); | ||
|
|
||
| static CEL_TEXT_TEMPLATE: &str = include_str!(concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/templates/summary.cel.txt.hbs" | ||
| )); | ||
| static CEL_TEXT_TEMPLATE: &str = include_str!("templates/summary.cel.txt.hbs"); | ||
|
|
||
| static STANDARD_MARKDOWN_TEMPLATE: &str = include_str!(concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/templates/summary.standard.md.hbs" | ||
| )); | ||
| static STANDARD_MARKDOWN_TEMPLATE: &str = include_str!("templates/summary.standard.md.hbs"); | ||
|
|
||
| static STANDARD_TEXT_TEMPLATE: &str = include_str!(concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/templates/summary.standard.txt.hbs" | ||
| )); | ||
| static STANDARD_TEXT_TEMPLATE: &str = include_str!("templates/summary.standard.txt.hbs"); |
There was a problem hiding this comment.
i assume that's what you meant, otherwise i don't understand why it was how it was
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ref_statuses.rs (1)
37-44:⚠️ Potential issue | 🟠 MajorAdd an explicit timeout for the blocking ref-statuses HTTP call.
Line 38 performs a blocking network request without a timeout, which could hang indefinitely on network stalls or slow responses, impacting availability.
Use
reqwest::blocking::Client::builder()with atimeout()duration:Suggested fix
use std::collections::BTreeMap; +use std::time::Duration; pub(crate) fn fetch_ref_statuses() -> Result<BTreeMap<String, String>, FlakeCheckerError> { + let client = reqwest::blocking::Client::builder() + .timeout(Duration::from_secs(15)) + .build()?; + - let officially_supported: BTreeMap<String, String> = - reqwest::blocking::get(ALLOWED_REFS_URL)? + let officially_supported: BTreeMap<String, String> = client + .get(ALLOWED_REFS_URL) + .send()? + .error_for_status()? .json::<Response>()? .data .result .iter() .map(|res| (res.metric.channel.clone(), res.metric.status.clone())) .collect();🤖 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 `@src/ref_statuses.rs` around lines 37 - 44, The reqwest::blocking::get() call fetching from ALLOWED_REFS_URL has no timeout and could hang indefinitely on network issues. Replace the direct reqwest::blocking::get() call with a reqwest::blocking::Client created using Client::builder() with an explicit timeout duration set via the timeout() method, then use that client's get() method to make the request. This ensures the HTTP call will fail gracefully after the specified timeout rather than hanging indefinitely.
🤖 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.
Outside diff comments:
In `@src/ref_statuses.rs`:
- Around line 37-44: The reqwest::blocking::get() call fetching from
ALLOWED_REFS_URL has no timeout and could hang indefinitely on network issues.
Replace the direct reqwest::blocking::get() call with a
reqwest::blocking::Client created using Client::builder() with an explicit
timeout duration set via the timeout() method, then use that client's get()
method to make the request. This ensures the HTTP call will fail gracefully
after the specified timeout rather than hanging indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ec27130-49c8-4de0-ad9b-76715202cc35
📒 Files selected for processing (6)
parse-flake-lock/src/lib.rssrc/condition.rssrc/flake.rssrc/main.rssrc/ref_statuses.rssrc/summary.rs
Summary by CodeRabbit
Node:git_ref(),last_modified(), andowner()for easier metadata retrieval.