refactor: use struct and impl method for link check#241
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a public LinkChecker struct encapsulating a reqwest::Client (5s timeout, no redirects) and moves the former module-level async check_link into an async method LinkChecker::check_link(&self, url). Module exports and service call sites are updated to use LinkChecker. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant Service as link_checker::service
participant LC as LinkChecker
participant HTTP as reqwest::Client
Caller->>Service: run(...)
Service->>LC: LinkChecker::new() / default()
loop For each link
Service->>LC: check_link(url)
activate LC
LC->>HTTP: HTTP request (no redirects, 5s timeout)
HTTP-->>LC: response / error
LC-->>Service: LinkCheckResult
deactivate LC
end
Service-->>Caller: Summary/Results
note over LC,HTTP: Retries, redirect handling, and status classification are internal to LinkChecker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 0
🧹 Nitpick comments (1)
src/link_checker/checker.rs (1)
10-17:LinkChecker::new에서 안전한 초기화 경로 추가 제안현재
build().unwrap()때문에 TLS 설정 문제 등으로 클라이언트 생성이 실패하면 전체 실행이 패닉으로 종료됩니다. 구조체를 도입해 재사용하기로 한 만큼, 호출 측이 오류를 처리할 수 있도록 fallible 생성자를 제공하는 편이 안정적입니다.new()는 유지하되 내부에서try_new()를 호출하도록 하면 기존 호출부 변경도 최소화할 수 있습니다.- pub fn new() -> Self { - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(5)) - .redirect(reqwest::redirect::Policy::none()) - .build() - .unwrap(); - - LinkChecker { client } - } + pub fn try_new() -> Result<Self, reqwest::Error> { + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(5)) + .redirect(reqwest::redirect::Policy::none()) + .build()?; + + Ok(LinkChecker { client }) + } + + pub fn new() -> Self { + Self::try_new().expect("failed to build reqwest::Client") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/link_checker/checker.rs(3 hunks)src/link_checker/mod.rs(1 hunks)src/link_checker/service.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: reddevilmidzy
PR: reddevilmidzy/queensac#226
File: src/main.rs:21-32
Timestamp: 2025-09-22T15:50:19.531Z
Learning: User reddevilmidzy prefers to communicate in Korean and likes to keep PR scope focused and well-defined.
🧬 Code graph analysis (1)
src/link_checker/service.rs (1)
src/link_checker/checker.rs (1)
new(9-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Pull Request Test Coverage Report for Build 18257090077Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the link checking functionality by introducing a struct-based approach for better client reuse and improved architecture.
- Renamed module from
linktocheckerand introducedLinkCheckerstruct - Replaced function-based link checking with a struct that reuses HTTP client instances
- Added
Defaultimplementation forLinkCheckeras suggested by Clippy
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/link_checker/mod.rs | Updated module and public API exports to use new LinkChecker struct |
| src/link_checker/service.rs | Modified to use instance-based link checking with LinkChecker::new() |
| src/link_checker/checker.rs | Complete refactor introducing LinkChecker struct with client reuse and updated tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/link_checker/checker.rs (2)
9-20: Consider capturing the underlying error.The error message "failed to create Client" is generic and doesn't include the actual error from
reqwest::Client::builder().build(), which would help with debugging if client creation fails.Apply this diff to capture and propagate the underlying error:
pub fn new() -> Result<Self, String> { let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs(5)) .redirect(reqwest::redirect::Policy::none()) - .build(); - - if let Ok(client) = client { - Ok(LinkChecker { client }) - } else { - Err("failed to create Client".to_string()) - } + .build() + .map_err(|e| format!("failed to create Client: {e}"))?; + + Ok(LinkChecker { client }) }
59-63: Consider whetherDefaultshould panic.The
Defaulttrait implementation callsexpect(), which will panic if client creation fails. Whilereqwest::Client::builder().build()rarely fails in practice, panicking in aDefaultimplementation can be surprising for callers who expectDefaultto be infallible.Consider one of these alternatives:
- Document that
Default::default()may panic.- Use
ClientBuilder::build()without custom configuration (which is infallible) for theDefaultimpl, and reserve the configured client for explicitnew()calls.- Remove the
Defaultimpl and require explicit construction.Since your use case consistently needs the timeout and redirect policy, and clippy suggested adding
Default, the current approach is acceptable if you document the panic possibility or are confident client creation won't fail in your environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/link_checker/checker.rs(3 hunks)src/link_checker/service.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: reddevilmidzy
PR: reddevilmidzy/queensac#226
File: src/main.rs:21-32
Timestamp: 2025-09-22T15:50:19.531Z
Learning: User reddevilmidzy prefers to communicate in Korean and likes to keep PR scope focused and well-defined.
🧬 Code graph analysis (1)
src/link_checker/service.rs (1)
src/link_checker/checker.rs (2)
default(60-62)new(9-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/link_checker/service.rs (1)
103-108: LGTM! Efficient refactor to reuse HTTP client.Creating the
LinkCheckerinstance once before the loop ensures the underlyingreqwest::Clientis reused across all link checks, which is more efficient than creating a new client for each check. The method call syntax is correct.src/link_checker/checker.rs (3)
4-6: LGTM! Clean struct definition.The struct encapsulates the
reqwest::Clientto enable efficient reuse across multiple link checks.
22-56: LGTM! Retry logic is sound.The retry mechanism with 3 attempts, 1-second sleep between retries, and proper error handling ensures robustness when checking links. The method correctly uses
&selfto share the client across calls.
131-188: LGTM! Tests updated correctly.All tests now instantiate
LinkChecker::default()and call the method appropriately. The test coverage remains intact.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/link_checker/checker.rs (2)
22-56: Consider distinguishing retryable from non-retryable errors.The retry logic currently retries all request errors uniformly. However, certain failures like DNS resolution errors, certificate validation failures, or 4xx client errors (except 429 rate limits) are unlikely to succeed on retry and waste time.
You could check error types or status codes to skip retries for non-transient failures:
pub async fn check_link(&self, url: &str) -> LinkCheckResult { let mut attempts = 3; while attempts > 0 { match self.client.get(url).send().await { Ok(res) => { // ... existing logic ... } Err(e) => { // Don't retry non-transient errors if e.is_connect() || e.is_timeout() || e.is_status() { if attempts == 1 { return LinkCheckResult::Invalid(format!("Request error: {e}")); } } else { // DNS, TLS, redirect errors - fail immediately return LinkCheckResult::Invalid(format!("Request error: {e}")); } } } attempts -= 1; tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; } LinkCheckResult::Invalid("Max retries exceeded".to_string()) }
30-39: Redirect without Location header returns Valid.Line 39 returns
LinkCheckResult::Validwhen a redirection status is received but noLocationheader is present. A redirect response without a location header is unusual and might indicate a misconfigured server.Consider treating this as an error or at least logging it:
if let Some(redirect_url) = res.headers().get("location") && let Ok(redirect_str) = redirect_url.to_str() { if is_trivial_redirect(url, redirect_str) { return LinkCheckResult::Valid; } return LinkCheckResult::Redirect(redirect_str.to_string()); } - return LinkCheckResult::Valid; + // Redirect without Location header is unusual + return LinkCheckResult::Invalid( + format!("Redirect status {} without Location header", status) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/link_checker/checker.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: reddevilmidzy
PR: reddevilmidzy/queensac#226
File: src/main.rs:21-32
Timestamp: 2025-09-22T15:50:19.531Z
Learning: User reddevilmidzy prefers to communicate in Korean and likes to keep PR scope focused and well-defined.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| } else if status.as_u16() == 404 && url.contains("github.com") { | ||
| return handle_github_404(url); |
There was a problem hiding this comment.
URL host check is fragile.
Using url.contains("github.com") can match unintended URLs like https://notgithub.com or paths containing the substring. Consider parsing the URL and checking the host properly.
-} else if status.as_u16() == 404 && url.contains("github.com") {
+} else if status.as_u16() == 404 && is_github_url(url) {
return handle_github_404(url);Add a helper function:
fn is_github_url(url: &str) -> bool {
Url::parse(url)
.ok()
.and_then(|u| u.host_str())
.map(|h| h == "github.com" || h.ends_with(".github.com"))
.unwrap_or(false)
}🤖 Prompt for AI Agents
In src/link_checker/checker.rs around lines 40-41, the check using
url.contains("github.com") is fragile and can match unintended strings; replace
it by parsing the URL and checking the host properly (use
Url::parse(url).ok().and_then(|u| u.host_str()).map(|h| h == "github.com" ||
h.ends_with(".github.com")).unwrap_or(false))—add a small helper fn
is_github_url(url: &str) -> bool that implements this logic, use that helper in
place of the contains() call, and ensure the url crate is in scope and parsing
failures are handled by returning false.
Docstrings generation was requested by @reddevilmidzy. * #241 (comment) The following files were modified: * `src/link_checker/checker.rs` * `src/link_checker/service.rs`
|
Note Generated docstrings for this pull request at #243 |
Docstrings generation was requested by @reddevilmidzy. * #241 (comment) The following files were modified: * `src/link_checker/checker.rs` * `src/link_checker/service.rs` Co-Authored-by: Redddy <areyou@redddy.com>
♟️ What’s this PR about?
(default 구현은 안했었는데 clippy가 구현하라고 친절하게 알려줘서 구현해주었다 )
Summary by CodeRabbit
Refactor
Bug Fixes
Tests