Skip to content

Address pr review comments #166

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.

Sign up for GitHub

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

Merged
takenagain merged 1 commit into from
Aug 4, 2025
Merged

Address pr review comments #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ class TrezorConnectionMonitor {
/// [onConnectionRestored] will be called when the device becomes connected
/// after being disconnected/unreachable.
/// [onStatusChanged] will be called for any status change.
/// [maxDuration] sets the maximum time to monitor before timing out.
void startMonitoring({
String? devicePubkey,
Duration pollInterval = const Duration(seconds: 1),
Duration maxDuration = const Duration(minutes: 30),
VoidCallback? onConnectionLost,
VoidCallback? onConnectionRestored,
void Function(TrezorConnectionStatus)? onStatusChanged,
Expand All @@ -38,6 +40,7 @@ class TrezorConnectionMonitor {
.watchConnectionStatus(
devicePubkey: devicePubkey,
pollInterval: pollInterval,
maxDuration: maxDuration,
)
.listen(
(status) {
Expand All @@ -60,9 +63,12 @@ class TrezorConnectionMonitor {
onConnectionRestored?.call();
}
},
onError: (Object error) {
_log.severe('Error monitoring Trezor connection: $error');
onConnectionLost?.call();
onError: (Object error, StackTrace stackTrace) {
_log.severe('Error monitoring Trezor connection: $error', error, stackTrace);
// Only call onConnectionLost if this is a real connection error, not a disposal
if (_connectionSubscription != null) {
onConnectionLost?.call();
}
},
onDone: () {
_log.info('Trezor connection monitoring stopped');
Expand Down
3 changes: 2 additions & 1 deletion packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ enum TrezorConnectionStatus {
/// Check if the status indicates the device is not available
bool get isUnavailable =>
this == TrezorConnectionStatus.disconnected ||
this == TrezorConnectionStatus.unreachable;
this == TrezorConnectionStatus.unreachable ||
this == TrezorConnectionStatus.busy;

/// Check if the device should continue being monitored
bool get shouldContinueMonitoring =>
Expand Down
11 changes: 9 additions & 2 deletions packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,15 @@ class TrezorRepository {
///
/// The stream immediately yields the current status, then continues to poll
/// using [pollInterval]. If the status changes, a new value is emitted. The
/// stream closes once a `Disconnected` status is observed.
/// stream closes once a `Disconnected` status is observed or [maxDuration] is reached.
Stream<TrezorConnectionStatus> watchConnectionStatus({
String? devicePubkey,
Duration pollInterval = const Duration(seconds: 1),
Duration maxDuration = const Duration(minutes: 30),
}) async* {
TrezorConnectionStatus? last;
final stopwatch = Stopwatch()..start();
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

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();

Copilot uses AI. Check for mistakes.

try {
last = await getConnectionStatus(devicePubkey: devicePubkey);
yield last;
Expand All @@ -226,7 +229,7 @@ class TrezorRepository {
);
}

while (last!.shouldContinueMonitoring) {
while (last!.shouldContinueMonitoring && stopwatch.elapsed < maxDuration) {
await Future<void>.delayed(pollInterval);
try {
final current = await getConnectionStatus(devicePubkey: devicePubkey);
Expand All @@ -239,6 +242,10 @@ class TrezorRepository {
return;
}
}

if (stopwatch.elapsed >= maxDuration) {
yield TrezorConnectionStatus.unreachable;
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
}

/// Cancel all active initializations and clean up resources
Expand Down
Loading