Skip to content

Conversation

@ibigbug
Copy link
Member

@ibigbug ibigbug commented May 24, 2025

🤔 This is a ...

  • New feature
  • Bug fix
  • Performance optimization
  • Enhancement feature
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Workflow
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

📝 Changelog

returns proxies in providers for API

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Changelog is provided or not needed

@ibigbug ibigbug requested a review from Copilot May 24, 2025 18:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the API to return all proxies, including those from providers, and refactors several initialization calls to remove panic paths.

  • Merges provider proxies into get_proxies in OutboundManager.
  • Simplifies HealthCheck::new to return Self instead of Result<Self>.
  • Removes .unwrap() and .map_err() chains on provider creation, but leaves error handling gaps.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
clash_lib/src/app/remote_content_manager/providers/proxy_provider/proxy_set_provider.rs Dropped the .unwrap() on ProxySetProvider::new.
clash_lib/src/app/remote_content_manager/healthcheck.rs Changed HealthCheck::new to be infallible (-> Self).
clash_lib/src/app/outbound/manager.rs Added get_proxies to include provider proxies; removed several .map_err() handlers; added doc comments; introduced provider_handlers.
Comments suppressed due to low confidence (5)

clash_lib/src/app/outbound/manager.rs:131

  • [nitpick] The variable provider_handlers actually stores proxies from providers, not handlers. Renaming it to provider_proxies or similar would improve clarity.
let mut provider_handlers = HashMap::new();

clash_lib/src/app/remote_content_manager/providers/proxy_provider/proxy_set_provider.rs:316

  • The call to ProxySetProvider::new returns a Result, but the previous .unwrap() was removed without handling errors. You should either propagate the error with ? or handle it explicitly to avoid compilation failures or silent drops.
);

clash_lib/src/app/outbound/manager.rs:139

  • Chaining self.handlers.iter() (yielding AnyOutboundHandler) with provider_handlers.iter() (yielding provider proxy types) may cause a type mismatch. Consider unifying both collections under a common trait or mapping provider proxies into handler adapters before chaining.
for (k, v) in self.handlers.iter().chain(provider_handlers.iter()) {

clash_lib/src/app/outbound/manager.rs:375

  • Error mapping for PlainProvider::new was removed; failures in health check config will now be ignored. Reintroduce proper error handling (e.g., .map_err(...) or ?) to surface misconfigurations.
);

clash_lib/src/app/outbound/manager.rs:760

  • This removal of .map_err(...) after PlainProvider::new silently drops configuration errors. You should propagate or log these errors to prevent hidden failures.
);

@codecov
Copy link

codecov bot commented May 24, 2025

Codecov Report

Attention: Patch coverage is 35.29412% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
clash_lib/src/app/outbound/manager.rs 15.38% 11 Missing ⚠️

📢 Thoughts on this report? Let us know!

@ibigbug ibigbug merged commit 993dda8 into master May 24, 2025
29 of 30 checks passed
@ibigbug ibigbug deleted the fix-api-proxies branch May 24, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants