-
Notifications
You must be signed in to change notification settings - Fork 0
Add typed errors across services #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@codex please review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is an excellent and comprehensive refactoring of error handling across multiple services. It replaces Box<dyn std::error::Error> with strongly-typed Error enums, which greatly improves code clarity, maintainability, and robustness. The new error types are well-structured, especially in the roborock crate with its nested error enums. The updated retry logic in roborock is now much cleaner and safer thanks to pattern matching on error variants instead of downcasting and string comparison. I have one suggestion for alisa to further improve error handling by preserving error information.
There was a problem hiding this 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 refactors error handling across the codebase by replacing boxed trait object errors (Box<dyn Error + Send + Sync>) with typed error enums. The changes span multiple services including roborock, xiaomi, elisa, isabel, elisheba, and alisa libraries.
- Introduces structured error enums with specific variants for different error types across all affected services
- Refactors roborock protocol errors into
DecodeError,EncodeError, andRpcErrorsub-enums - Updates retry logic in roborock vacuum to work with typed errors instead of trait object downcasting
Reviewed changes
Copilot reviewed 14 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/xiaomi/src/error.rs | New error enum with variants for device, checksum, crypto, IO, JSON, and timeout errors |
| lib/xiaomi/src/vacuum.rs | Removed custom StartError type, now uses Error::UnsupportedBinType; made BinType public |
| lib/xiaomi/src/message.rs | Replaced InvalidChecksum custom error with Error enum; updated crypto error handling |
| lib/xiaomi/src/discover.rs | Removed DevicesNotFound custom error in favor of Error enum variant |
| lib/xiaomi/src/device.rs | Updated error matching to use typed Error enum patterns |
| lib/xiaomi/src/lib.rs | Replaced ErasedError type alias with Error enum export |
| lib/roborock/src/error.rs | New error hierarchy with Error, DecodeError, EncodeError, and RpcError enums |
| lib/roborock/src/protocol.rs | Refactored decode/encode/RPC errors to use typed variants; removed custom error types |
| lib/roborock/src/vacuum.rs | Updated should_retry function to pattern match on typed Error instead of downcasting |
| lib/roborock/src/local.rs | Updated error handling to use typed Error enum |
| lib/roborock/src/lib.rs | Replaced ErasedError type alias with Error enum export |
| bin/isabel/src/error.rs | New error enum for isabel service with bluetooth, MQTT, JSON, and timeout variants |
| bin/isabel/src/lib.rs | Replaced ErasedError with typed Error enum |
| bin/elisheba/src/error.rs | New error enum with sonoff, MQTT, JSON, join, timeout, IO, and unknown device variants |
| bin/elisheba/src/lib.rs | Updated to use typed Error enum; replaced string errors with Error::UnknownDevice |
| bin/elisheba/src/main.rs | Updated function signatures and error handling to use typed Result type |
| bin/elisa/src/error.rs | New error enum with JSON, MQTT, vacuum, queue closed, join, and address parse variants |
| bin/elisa/src/lib.rs | Updated to use typed Error enum; added explicit Ok(()) returns in perform_action |
| bin/alisa/src/error.rs | New error enum with MQTT, JSON, IO, join, and HTTP variants |
| bin/alisa/src/lib.rs | Replaced ErasedError with typed Error enum |
| bin/alisa/src/web_service/mod.rs | Updated ServiceError to use typed Error; added specific From impls for paho_mqtt and serde_json |
| bin/alisa/src/web_service/user/query.rs | Changed expect to proper error propagation using map_err |
| lib/bluetooth/src/lib.rs | Added Error export for consistency with other libraries |
| GEMINI.md | Added note about running validations before completing todo steps |
| AGENTS.md | Added guidelines for PR testing sections and updated workflow instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Summary