Feat monitor ping

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

• 重構
• 新功能
• 錯誤修復
• 最佳化
• 技術文件更新

描述

新增強震監視器 更新延遲狀態

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

請將這行替換成：如何測試您的 PR 的步驟，已測試的裝置註釋，以及任何相關的 UI 更改圖片。

UI 無障礙清單

如果您的 PR 包含 UI 更改，請使用此清單：

• 變數名稱實現語意化命名？
• 測試通過 AA 顏色對比？

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review: Monitor Ping Feature

Summary

This PR adds a ping/latency monitoring feature to the seismic monitor, displaying connection status and data freshness indicators. The implementation is generally solid but has a few areas that could be improved.

---

Positive Aspects ✅

1. Clear Feature Addition: The ping monitoring provides valuable real-time feedback about data connection status
2. Good Visual Indicators: Color-coded ping display (green/orange/red) makes status immediately clear to users
3. Performance Optimization: Properly uses caching and only updates when data changes
4. Stale Data Detection: The 3-second threshold for marking data as stale is reasonable

---

Issues & Recommendations

1. Private Member Access in Widget ⚠️ (Medium Priority)

Location: lib/app/map/_lib/managers/monitor.dart:1274-1276

final ping = widget.manager._ping;
final isDataOk = widget.manager._dataStatus();

Issue: The widget accesses private members (_ping, _dataStatus()) of the manager class. This breaks encapsulation.

Recommendation: Make these accessible through proper public interfaces:

// In MonitorMapLayerManager class
double get ping => _ping;
bool get dataStatus => _dataStatus();

Then use:

final ping = widget.manager.ping;
final isDataOk = widget.manager.dataStatus;

---

2. Missing Reactive Updates for Ping Display ⚠️ (Medium Priority)

Location: lib/app/map/_lib/managers/monitor.dart:1268-1313

Issue: The ping value is accessed directly from widget.manager._ping but there's no mechanism to trigger rebuilds when the ping value changes. The ValueListenableBuilder only listens to displayTimeNotifier, but ping is calculated and updated separately in _updateDisplayTime().

Current flow:

• _updateDisplayTime() calculates _ping and calls displayTimeNotifier.notifyListeners()
• The ValueListenableBuilder rebuilds
• Widget reads the updated _ping value

Problem: This works by coincidence because displayTimeNotifier.notifyListeners() is called after updating _ping. If the calling order changes, ping updates might not reflect properly.

Recommendation: Either:

1. Create a dedicated ValueNotifier<double> for ping
2. Or create a data class that contains both time and ping information

// Option 1: Separate notifier
final pingNotifier = ValueNotifier<double>(0);

// In _updateDisplayTime()
_ping = (currentTime - t) / 1000;
pingNotifier.value = _ping;

// Option 2: Combined data class
class MonitorStatus {
  final String displayTime;
  final double ping;
  final bool isStale;
  MonitorStatus(this.displayTime, this.ping, this.isStale);
}

final statusNotifier = ValueNotifier<MonitorStatus>(...);

---

3. Redundant Variable Declaration ℹ️ (Low Priority)

Location: lib/app/map/_lib/managers/monitor.dart:733

bool _dataStatus() {
  final bool status = ((GlobalProviders.data.currentTime) - (_lastDataReceivedTime ?? 0)) < 3000;
  return status;
}

Recommendation: Simplify to:

bool _dataStatus() {
  return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 3000;
}

---

4. Magic Number for Container Width ℹ️ (Low Priority)

Location: lib/app/map/_lib/managers/monitor.dart:1287

width: 230,

Issue: The width value 230 is hardcoded and might not adapt well to different screen sizes or when the ping text format changes.

Recommendation: Consider using IntrinsicWidth or calculating based on text size:

// Remove fixed width, or calculate dynamically
decoration: BoxDecoration(
  color: context.colors.surface.withValues(alpha: 0.5),
  borderRadius: BorderRadius.circular(16),
),

---

5. Inconsistent Formatting ℹ️ (Low Priority)

The diff shows many indentation changes that appear to be code formatting adjustments. While the final result is consistent, this makes the PR harder to review as it mixes functional changes with formatting changes.

Recommendation: In future PRs, consider:

1. Running code formatter before making functional changes
2. Or separating formatting changes into a different commit
3. This keeps the functional diff cleaner and easier to review

---

Testing Recommendations

1. Test scenarios to verify:

   • Normal data flow (ping < 1s, 1-5s, > 5s)
   • Data connection loss (stale data display)
   • Rapid connection state changes
   • Screen rotation / different screen sizes

2. Edge cases:

   • What happens when _lastDataReceivedTime is null?
   • Does the UI handle very large ping values gracefully (e.g., 999.99s)?

---

Summary

Overall Assessment: Good implementation with minor improvements needed

Required Changes: None critical, but fixing the encapsulation issue (#1) and reactive updates (#2) would improve code quality and maintainability.

Nice to Have: Address the minor issues (#3, #4, #5) for cleaner code.

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review: Monitor Ping Feature (Follow-up)

Task List

• Check for repository guidelines (CLAUDE.md)
• Read the modified file
• Review git diff to see specific changes
• Analyze code quality and implementation
• Post comprehensive review feedback

---

Summary

I've reviewed the latest changes to the ping monitoring feature. Great work addressing some of the previous feedback! The implementation now uses proper reactive patterns with ValueNotifier for the ping value, and the encapsulation has been improved with public getters. However, there are still a few issues to address.

---

What's Been Fixed ✅

1. Proper Reactive Updates: The ping now uses pingNotifier.value = _ping instead of directly calling displayTimeNotifier.notifyListeners(). This is much cleaner! (lib/app/map/_lib/managers/monitor.dart:718)

2. Better Encapsulation: Added public getters dataStatus and ping for accessing private state (lib/app/map/_lib/managers/monitor.dart:39-40)

3. Nested ValueListenableBuilder: The UI now properly uses nested ValueListenableBuilder widgets for both displayTimeNotifier and pingNotifier (lib/app/map/_lib/managers/monitor.dart:1275-1318)

4. Fixed Container Width: Removed the hardcoded width: 230 and replaced it with mainAxisSize: MainAxisSize.min for better adaptability (lib/app/map/_lib/managers/monitor.dart:1295)

5. Position Adjustment: Fine-tuned the top position from 24 to 26 for better visual alignment (lib/app/map/_lib/managers/monitor.dart:1263)

---

Remaining Issues

1. Inconsistent Stale Data Threshold ⚠️ (Medium Priority)

Location: lib/app/map/_lib/managers/monitor.dart:81, 706

// Line 81 - _dataStatus() method
return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 10000;

// Line 706 - _updateDisplayTime() method  
final isStale = lastDataReceivedTime != null && (currentTime - lastDataReceivedTime) > 3000;

Issue: There's an inconsistency in the stale data thresholds:

• _dataStatus() uses 10 seconds (10000ms)
• _updateDisplayTime() uses 3 seconds (3000ms) for the "STALE" label

This means the ping color can still show green/orange while the time display shows "STALE", creating confusion for users.

Recommendation: Use a consistent threshold. I suggest using 3 seconds for both:

bool _dataStatus() {
  return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 3000;
}

Why 3 seconds? For real-time seismic monitoring, 3 seconds of data staleness is significant and should be immediately visible to users. This aligns with the "STALE" label behavior.

---

2. Missing Disposal of pingNotifier ⚠️ (Medium Priority)

Location: lib/app/map/_lib/managers/monitor.dart:85, 912-918

Issue: The pingNotifier ValueNotifier is created but never disposed, which can cause memory leaks.

Current dispose method:

@override
void dispose() {
  _blinkTimer?.cancel();
  _blinkTimer = null;
  _stopFocusTimer();
  GlobalProviders.data.setReplayMode(false);
  GlobalProviders.data.removeListener(_onDataChanged);
  super.dispose();
}

Recommendation: Add disposal of all ValueNotifiers:

@override
void dispose() {
  _blinkTimer?.cancel();
  _blinkTimer = null;
  _stopFocusTimer();
  GlobalProviders.data.setReplayMode(false);
  GlobalProviders.data.removeListener(_onDataChanged);
  currentRtsTime.dispose();
  displayTimeNotifier.dispose();
  pingNotifier.dispose();
  super.dispose();
}

---

3. Code Organization ℹ️ (Low Priority)

Location: lib/app/map/_lib/managers/monitor.dart:39-85

Issue: The getter methods (dataStatus, ping) and the _dataStatus() method are separated by several field declarations, making the code harder to follow.

Current structure:

bool get dataStatus => _dataStatus();  // Line 39
double get ping => _ping;              // Line 40
double _ping = 0;                       // Line 41
// ... other fields ...
bool _dataStatus() { ... }              // Line 80
final currentRtsTime = ...              // Line 83
final displayTimeNotifier = ...         // Line 84
final pingNotifier = ...                // Line 85

Recommendation: Group related items together for better readability:

// State fields
double _ping = 0;
int? _lastDataReceivedTime;
int _lastDisplayedSecond = 0;

// Notifiers
final currentRtsTime = ValueNotifier<int?>(GlobalProviders.data.rts?.time);
final displayTimeNotifier = ValueNotifier<String>('N/A');
final pingNotifier = ValueNotifier<double>(0);

// Public getters
bool get dataStatus => _dataStatus();
double get ping => _ping;

// Helper methods
bool _dataStatus() {
  return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 3000;
}

---

Testing Recommendations

Please verify the following scenarios:

1. Normal operation: Ping displays correctly with appropriate colors (green < 1s, orange 1-5s, red > 5s)
2. Data staleness: After 3 seconds without data, both the time text and ping color indicate stale status
3. Connection loss: When data stops arriving, "STALE" label appears and ping shows "N/A" with red color
4. Memory management: No memory leaks when navigating away from the monitor view
5. Rapid updates: UI handles frequent data updates without performance issues

---

Overall Assessment

Status: Good progress with minor issues remaining

The changes demonstrate good understanding of reactive patterns in Flutter. The main concerns are:

• Critical: None
• Important: Inconsistent stale thresholds (build: 1.0.13 #1), missing disposals (fix: earthquake 頁面 #2)
• Nice to have: Better code organization (fix: earthquake 頁面 #3)

The feature is functional, but fixing issues #1 and #2 would significantly improve reliability and prevent potential bugs.

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review: Monitor Ping Feature (Latest Changes)

Summary

Great progress on addressing the previous feedback! I've reviewed the latest changes across your 5 commits, and I'm pleased to see that most of the critical issues have been resolved. The code is in much better shape now.

---

✅ Issues Successfully Resolved

1. Proper Reactive Updates ✅

Location: lib/app/map/_lib/managers/monitor.dart:718

The ping now uses pingNotifier.value = _ping with a dedicated ValueNotifier<double> instead of piggybacking on displayTimeNotifier.notifyListeners(). This is exactly the right approach and ensures proper reactive updates.

_ping = (currentTime - t) / 1000;
pingNotifier.value = _ping;  // ✅ Dedicated notifier

2. Better Encapsulation ✅

Location: lib/app/map/_lib/managers/monitor.dart:39-40

You've added public getters for dataStatus and ping, removing the need for widgets to access private members directly:

bool get dataStatus => _dataStatus();
double get ping => _ping;

The widget now properly accesses these through the public interface at lib/app/map/_lib/managers/monitor.dart:1281.

3. Nested ValueListenableBuilder ✅

Location: lib/app/map/_lib/managers/monitor.dart:1278-1322

The UI now uses properly nested ValueListenableBuilder widgets for both displayTimeNotifier and pingNotifier, ensuring efficient rebuilds only when relevant data changes.

4. Fixed Container Width ✅

Location: lib/app/map/_lib/managers/monitor.dart:1297-1298

Removed the hardcoded width: 230 and replaced it with mainAxisSize: MainAxisSize.min for better adaptability:

child: Row(
  mainAxisSize: MainAxisSize.min,  // ✅ Flexible sizing
  mainAxisAlignment: MainAxisAlignment.center,

5. Memory Leak Prevention ✅

Location: lib/app/map/_lib/managers/monitor.dart:918-920

The dispose() method now properly cleans up all ValueNotifier instances:

currentRtsTime.dispose();
displayTimeNotifier.dispose();
pingNotifier.dispose();  // ✅ Fixed in commit 66e5e39

6. UI Improvements ✅

• Position fine-tuned from top: 24 to top: 26 for better alignment
• Ping text width increased from 45 to 55 to prevent wrapping

---

⚠️ Remaining Issue: Inconsistent Stale Data Threshold

Location: lib/app/map/_lib/managers/monitor.dart:81, 706

There's still an inconsistency in the stale data thresholds:

// Line 81 - _dataStatus() method (used for ping color)
return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 12000;  // 12 seconds

// Line 706 - _updateDisplayTime() method (used for "STALE" label)
final isStale = lastDataReceivedTime != null && (currentTime - lastDataReceivedTime) > 3000;  // 3 seconds

The Problem:

This creates a confusing UX where:

• After 3 seconds without data: Time shows "STALE" (red text), but ping might still show green/orange
• After 12 seconds without data: Both time and ping show red

User Experience Impact:

For a seismic monitoring application, this inconsistency could confuse users during critical moments. The ping color is meant to indicate connection health, so it should align with the staleness indicator.

Recommendation:

I see from commit 13093dc that you intentionally extended the threshold to 12s "for user visibility". However, I recommend using consistent thresholds for both indicators. You have two options:

Option A: Use 12s for both (if that's your design choice)

// Line 706
final isStale = lastDataReceivedTime != null && (currentTime - lastDataReceivedTime) > 12000;

Option B: Use 3s for both (for more sensitive monitoring)

// Line 81
return (GlobalProviders.data.currentTime - (_lastDataReceivedTime ?? 0)) < 3000;

My Suggestion: Given this is a seismic monitoring system where timely data is critical, I'd lean toward Option B (3 seconds) or possibly a middle ground like 5-6 seconds for both. But the key is they should match.

---

📝 Minor Code Organization Suggestion (Optional)

Location: lib/app/map/_lib/managers/monitor.dart:39-85

The structure mixes getters, fields, and methods in a way that makes it slightly harder to follow. Consider grouping related items:

// State fields
double _ping = 0;
int? _lastDataReceivedTime;
int _lastDisplayedSecond = 0;

// Notifiers
final currentRtsTime = ValueNotifier<int?>(GlobalProviders.data.rts?.time);
final displayTimeNotifier = ValueNotifier<String>('N/A');
final pingNotifier = ValueNotifier<double>(0);

// Public getters
bool get dataStatus => _dataStatus();
double get ping => _ping;

// Helper methods
bool _dataStatus() { ... }

This is purely a readability improvement and not critical.

---

🎯 Overall Assessment

Status: Nearly complete - one design decision needed

The implementation is solid and demonstrates good understanding of Flutter's reactive patterns. All critical issues from the previous reviews have been addressed:

• ✅ Proper reactive updates with dedicated notifiers
• ✅ Encapsulation through public getters
• ✅ Memory management with proper disposal
• ✅ Flexible UI sizing
• ⚠️ One remaining issue: Inconsistent stale data thresholds (12s vs 3s)

Recommendation: Fix the threshold inconsistency and this PR will be ready to merge! Great work on the iterative improvements.

---

Testing Checklist

Please verify these scenarios work as expected:

1. ✅ Normal operation with varying ping times (< 1s green, 1-5s orange, > 5s red)
2. ⚠️ Data staleness: Verify the time and ping indicators align at the chosen threshold
3. ✅ Connection loss: "STALE" label and "N/A" ping appear consistently
4. ✅ Memory management: No leaks when navigating away from monitor
5. ✅ Different screen sizes: UI adapts properly with flexible sizing

---