fix(api): append provider proxies to global proxies list #802
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the global proxy group setup by including provider proxies, reduces duplication via two new helper functions, and adjusts error propagation and inbound handling.
- Append provider proxies to the global proxy list instead of only using the plain provider.
- Refactor repeated empty‐check and provider‐appending logic into
check_group_emptyandmaybe_append_providers. - Change
unwrap()to?onPlainProvider::newand replace unimplemented redir case with a warning.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| clash_lib/src/app/outbound/manager.rs | Added check_group_empty and maybe_append_providers, refactored duplicate code, improved error handling, and updated global selector to include all providers. |
| clash_lib/src/app/inbound/network_listener.rs | Switched InboundOpts::Redir from unimplemented!() to a logged warning that returns early. |
Comments suppressed due to low confidence (1)
clash_lib/src/app/outbound/manager.rs:389
- New helper functions like
maybe_append_providersandcheck_group_emptyintroduce important logic. Adding unit tests for these helpers would ensure they behave correctly withNone, empty, and non‐existent providers.
fn maybe_append_providers(
| ) { | ||
| if let Some(provider_names) = provider_names { | ||
| for provider_name in provider_names { | ||
| let provider = provider_registry | ||
| .get(provider_name) | ||
| .unwrap_or_else(|| { | ||
| print_and_exit!("provider {} not found", provider_name); | ||
| }) | ||
| .clone(); | ||
|
|
||
| providers.push(provider); | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The helper currently aborts the entire process when a provider is missing. Consider returning a Result with a descriptive error and propagating it, to keep error handling consistent instead of calling print_and_exit!.
| ) { | |
| if let Some(provider_names) = provider_names { | |
| for provider_name in provider_names { | |
| let provider = provider_registry | |
| .get(provider_name) | |
| .unwrap_or_else(|| { | |
| print_and_exit!("provider {} not found", provider_name); | |
| }) | |
| .clone(); | |
| providers.push(provider); | |
| } | |
| } | |
| ) -> Result<(), Error> { | |
| if let Some(provider_names) = provider_names { | |
| for provider_name in provider_names { | |
| let provider = provider_registry | |
| .get(provider_name) | |
| .ok_or_else(|| { | |
| Error::InvalidConfig(format!("provider {} not found", provider_name)) | |
| })? | |
| .clone(); | |
| providers.push(provider); | |
| } | |
| } | |
| Ok(()) |
| InboundOpts::Redir { .. } => unimplemented!(), | ||
| InboundOpts::Redir { .. } => { | ||
| warn!("redir is not implemented yet"); | ||
| return Ok(()); |
There was a problem hiding this comment.
[nitpick] Silently returning Ok(()) for an unimplemented redir option may hide unexpected behavior downstream. Consider returning an explicit Err to signal that this feature is not yet supported.
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
🤔 This is a ...
🔗 Related issue link
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge