fix: handle missing QR code #65
Conversation
If the QR code is not successfully generated, send the user a notification. A notification is chosen for persistence, otherwise text on the screen may be lost on boot. Closes #49
WalkthroughThe pull request introduces changes to the Home Assistant plugin for OVOS, focusing on enhancing error handling and WebSocket connection capabilities. Key modifications include the addition of a Changes
Sequence DiagramsequenceDiagram
participant User
participant Plugin
participant HomeAssistant
participant WebSocket
User->>Plugin: Initiate OAuth
Plugin->>HomeAssistant: Request QR Code
alt QR Code Retrieved Successfully
HomeAssistant-->>Plugin: QR Code URL
Plugin->>User: Display QR Code
else QR Code Retrieval Fails
HomeAssistant-->>Plugin: No QR Code
Plugin->>User: Notify Failure
end
Plugin->>WebSocket: Connect (Max Size: 5MB)
WebSocket-->>Plugin: Connection Established
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🪛 Ruff (0.8.2)ovos_PHAL_plugin_homeassistant/__init__.py257-257: SyntaxError: Expected ',', found name 264-264: SyntaxError: Expected ',', found name ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
ovos_PHAL_plugin_homeassistant/__init__.py (1)
886-888: LGTM! Consider adding error details to the notification.Good addition of error handling for missing QR codes. The notification helps users understand when the login process fails.
Consider including more specific error details in the notification message:
- self.gui.show_notification("Failed to get QR code for Home Assistant login!") + self.gui.show_notification(f"Failed to get QR code for Home Assistant login: {message.data.get('error', 'Unknown error')}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ovos_PHAL_plugin_homeassistant/__init__.py(1 hunks)ovos_PHAL_plugin_homeassistant/logic/socketclient.py(1 hunks)requirements.txt(1 hunks)
🔇 Additional comments (2)
ovos_PHAL_plugin_homeassistant/logic/socketclient.py (1)
49-49: LGTM! Consider monitoring message sizes.The increase in WebSocket message size to 5MB is reasonable and aligns with the requirements updates. This should help prevent potential message truncation issues.
Consider monitoring actual message sizes in production to ensure this limit is appropriate:
requirements.txt (1)
8-8: LGTM! Well-documented version constraints.The version constraints are appropriate and the comments clearly explain the reasoning behind them:
- Avoiding websockets 14.0 due to breaking changes
- Adding websocket-client with Python 3 compatible versions
Also applies to: 11-11
|
Approved too soon. Found these errors in logs after authentication finished: |
|
@NeonDaniel I made the max websocket message size configurable with |
chore: allow newer versions of websockets
fix: increase websocket max size to 5MB from 1MB
Closes numerous issues ahead of merging the plugin and skill.
Summary by CodeRabbit
New Features
Dependency Updates
websocketspackage version constraint.websocket-clientpackage.Bug Fixes