Address pr review comments#166
Merged
takenagain merged 1 commit intofeat/trezor-connection-statusfrom Aug 4, 2025
Merged
Conversation
Co-authored-by: francoisvw <francoisvw@protonmail.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
takenagain
merged commit eae55fa
into
feat/trezor-connection-status
2 of 4 checks passed
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances Trezor connection monitoring by adding timeout functionality and improving error handling. The changes prevent infinite polling, provide better resource management, and make busy devices be considered unavailable.
- Added timeout functionality to prevent infinite monitoring with configurable maximum duration
- Enhanced error handling in the connection monitor with improved logging and stack traces
- Updated connection status logic to consider busy devices as unavailable
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| trezor_repository.dart | Added timeout functionality with stopwatch and maxDuration parameter to prevent infinite polling |
| trezor_connection_status.dart | Modified isUnavailable getter to include busy status alongside disconnected and unreachable |
| trezor_connection_monitor.dart | Enhanced error handling with stack traces and added maxDuration parameter support |
| Duration maxDuration = const Duration(minutes: 30), | ||
| }) async* { | ||
| TrezorConnectionStatus? last; | ||
| final stopwatch = Stopwatch()..start(); |
There was a problem hiding this comment.
The stopwatch is started immediately but the first status check happens before the while loop. Consider starting the stopwatch after the initial status check to ensure the timeout applies only to the polling phase, not the initial connection attempt.
Suggested change
| final stopwatch = Stopwatch()..start(); |
| } | ||
|
|
||
| if (stopwatch.elapsed >= maxDuration) { | ||
| yield TrezorConnectionStatus.unreachable; |
There was a problem hiding this comment.
Yielding 'unreachable' status on timeout may be misleading as the device might still be connected but monitoring has timed out. Consider introducing a new status like 'timeout' or 'monitoring_expired' to distinguish between actual unreachability and monitoring timeout.
CharlVS
added a commit
that referenced
this pull request
Aug 13, 2025
…ted (#126) * feat(auth): add polling stream for Trezor connection (#125) * Implement connection status stream * fix(types): use abstract classes for freezed models * feat(trezor,auth): sign user out if device is disconnected * fix(trezor): use snake case for keys and pascal case for values * fix(trezor): improve connection status stream error handling * refactor(trezor): improve Trezor connection monitoring with timeout and error handling (#166) Co-authored-by: Cursor Agent <cursoragent@cursor.com> * refactor(review): address PR nitpicks * refactor(review): add convenience methods and constructors * test: add unit tests for the new poll utility * refactor(trezor): add logging statements to auth mixin * test(trezor): add unit test cases for newly added interfaces * refactor(trezor): await close and stop functions and yield error * test(trezor): expand connection error handling test * fix(trezor): mitigate Trezor pin/passphrase exposure with converter freezed by default adds all fields to toString overrides, which could result in user pin/passphrases being printed to logs * fix(trezor): assume initial state as available and extract local variable * chore: rename dragon charts folder Rename the dragon charts folder to be consistent with the package name. NO FUNCTIONAL CHANGES * fix: fix broken package reference --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: CharlVS <77973576+CharlVS@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhance Trezor connection monitoring by adding a timeout, considering busy devices unavailable, and improving error handling.
These changes improve the reliability and resource management of the Trezor connection monitoring, preventing infinite polling and providing clearer status and error feedback.
Learn more about Cursor Agents