Skip to content

Conversation

@hatoo
Copy link
Owner

@hatoo hatoo commented Jun 18, 2025

  • Replace unwrap() calls with proper error handling in certificate generation
  • Convert Ok(..) else patterns to match statements with error context
  • Standardize error logging with consistent messages
  • Handle TLS connection errors gracefully instead of panicking
  • Fix authority parsing to handle invalid values without crashing

🤖 Generated with Claude Code

- Replace unwrap() calls with proper error handling in certificate generation
- Convert Ok(..) else patterns to match statements with error context
- Standardize error logging with consistent messages
- Handle TLS connection errors gracefully instead of panicking
- Fix authority parsing to handle invalid values without crashing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@hatoo hatoo requested a review from Copilot June 18, 2025 12:36

This comment was marked as outdated.

hatoo and others added 2 commits June 18, 2025 21:40
@hatoo hatoo requested a review from Copilot June 18, 2025 12:42
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 improves error handling and removes panic-prone code by replacing unwrap() calls with proper error propagation and logging.

  • Certificate generation now returns a Result with propagated errors instead of panicking.
  • TLS connection accept and upgrade failures are handled gracefully with appropriate logging.
  • The default client now carefully converts host values and parses headers without panicking.

Reviewed Changes

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

File Description
src/tls.rs Updated certificate generation to use error propagation with ? operators.
src/lib.rs Replaced unwrap-based patterns in async connection handling with error logs.
src/default_client.rs Improved error handling in TLS host conversion and header insertion.

src/lib.rs Outdated
Comment on lines 248 to 255
generate_cert(host, root_cert.borrow()).map_err(|err| {
tracing::error!("Failed to generate certificate for host: {}", err);
}).ok()
})
} else {
generate_cert(host, root_cert.borrow())
.map_err(|err| {
tracing::error!("Failed to generate certificate for host: {}", err);
Copy link

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

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

Consider including the actual host value in the error message (e.g., 'Failed to generate certificate for host {}: {}') to provide more context for debugging.

Suggested change
generate_cert(host, root_cert.borrow()).map_err(|err| {
tracing::error!("Failed to generate certificate for host: {}", err);
}).ok()
})
} else {
generate_cert(host, root_cert.borrow())
.map_err(|err| {
tracing::error!("Failed to generate certificate for host: {}", err);
generate_cert(host.clone(), root_cert.borrow()).map_err(|err| {
tracing::error!("Failed to generate certificate for host {}: {}", host, err);
}).ok()
})
} else {
generate_cert(host, root_cert.borrow())
.map_err(|err| {
tracing::error!("Failed to generate certificate for host {}: {}", host, err);

Copilot uses AI. Check for mistakes.
hatoo and others added 2 commits June 18, 2025 21:43
@hatoo hatoo merged commit d529169 into master Jun 18, 2025
3 checks passed
@hatoo hatoo deleted the fix-error-handling branch November 8, 2025 07: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