Fix app provider validation, add firmware battery check, and Fix WAL #3927
Conversation
…race condition - Use capability.id instead of title for validation checks in add_app_provider - Improve external_integration validation to handle actions without webhook - Trim chat and memory prompts before submission - Add low battery warning (<10%) on firmware update page to prevent bricking - Disable firmware update button when battery is too low - Fix race condition in WAL sync where _device could become null during async operations
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes and improvements. It correctly changes app provider validation to use capability.id and improves logic for external integrations. It also adds a crucial safety feature for firmware updates by checking for low battery, displaying a warning, and disabling the update button. Additionally, it fixes a race condition in the WAL sync service by correctly handling a potentially nullable device object during async operations. My review focuses on adherence to the project's coding conventions, specifically regarding color opacity modification, ensuring the use of withValues({double? alpha}) instead of withOpacity() as per established rules.
| color: Colors.red.withOpacity(0.1), | ||
| borderRadius: BorderRadius.circular(12), | ||
| border: Border.all(color: Colors.red.withOpacity(0.3)), |
There was a problem hiding this comment.
The general rules suggest using the withValues({double? alpha}) extension method instead of the deprecated withOpacity() for modifying a color's opacity. This should be applied here for consistency and to adhere to best practices.
| color: Colors.red.withOpacity(0.1), | |
| borderRadius: BorderRadius.circular(12), | |
| border: Border.all(color: Colors.red.withOpacity(0.3)), | |
| color: Colors.red.withValues(alpha: 0.1), | |
| borderRadius: BorderRadius.circular(12), | |
| border: Border.all(color: Colors.red.withValues(alpha: 0.3)), |
References
- The project's general rules recommend using the
withValues({double? alpha})extension method for changing a color's opacity, aswithOpacity()is considered deprecated.
| child: Text( | ||
| otaUpdateSteps.isEmpty ? "Start Update" : "Update", | ||
| style: TextStyle( | ||
| color: isBatteryTooLow ? Colors.white.withOpacity(0.4) : Colors.white, |
There was a problem hiding this comment.
According to the general rules, withValues({double? alpha}) should be used instead of withOpacity(). Please update this line to follow the established convention.
| color: isBatteryTooLow ? Colors.white.withOpacity(0.4) : Colors.white, | |
| color: isBatteryTooLow ? Colors.white.withValues(alpha: 0.4) : Colors.white, |
References
- The project's general rules recommend using the
withValues({double? alpha})extension method for changing a color's opacity, aswithOpacity()is considered deprecated.
adithya-234
changed the title
adithya-234
changed the title
|
too many changes in a single PR, also conflicts, pls feel free to reopen when its ready for review and also does not have multiple changes/fixes in a single PR |
|
Hey @adithya-234 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
2 participants
…race condition