Fix Cwa eew filter

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

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

描述

修復特定 EEW 來源機構

This PR restricts eew to CWA-only events for consistency across the app.

相關 issue

• 相關問題 #
• closes [Bug]: EEW判斷地震速報來源 #427

QA 指南、截圖、錄像

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

UI 無障礙清單

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

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

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

Todo List

• Read current state of lib/models/data.dart
• Check git diff to identify any changes since last review
• Analyze if the critical cache design flaw still exists
• Provide comprehensive review feedback

---

Summary

This PR introduces a CWA-specific EEW filter to the DpipDataModel class. The implementation has been significantly improved since the last review, successfully addressing the critical cache design flaw.

---

Changes in This PR

Added:

1. Cache variable _cwaCache (line 59) - Stores filtered CWA EEW results
2. Cache invalidation (lines 63-67) - setEew now clears cache when data updates
3. Convenience getter cwaEew (lines 69-74) - Returns filtered list of CWA EEWs with caching
4. Documentation (lines 63, 69-70) - Clear English comments explaining functionality

Modified:
5. Code formatting - Minor style improvements to currentTime getter and eewDistMap

---

Review Feedback

✅ Critical Issue Resolved

Great work! The previous review identified a critical cache design flaw where a generic eewByAgency(String agency) method used a single _cwaCache variable for all agencies. This has been completely fixed by:

• ✅ Removing the generic eewByAgency() method
• ✅ Making the implementation CWA-specific with a dedicated cwaEew getter
• ✅ Properly naming the cache variable as _cwaCache to match its purpose

This follows Option 2 from the previous review recommendation, which is the appropriate choice if only CWA filtering is needed.

---

✅ Strengths

1. Excellent cache design - CWA-specific caching is now correctly implemented
2. Proper cache invalidation - _cwaCache = null in setEew ensures data stays synchronized
3. Case-insensitive comparison - Robust filtering using .toLowerCase()
4. Clear documentation - English comments improve maintainability
5. Performance optimization - Lazy caching prevents redundant filtering operations
6. Immutable return type - UnmodifiableListView ensures data safety

---

💡 Minor Suggestions (Non-blocking)

1. Consider future extensibility (lib/models/data.dart:59-74)

If you anticipate needing filters for other agencies (JMA, KMA, etc.) in the future, you might want to consider a Map-based cache:

// Future-proof approach if other agencies will be added
Map<String, List<Eew>> _agencyCache = {};

UnmodifiableListView<Eew> _eewByAgency(String agency) {
  final key = agency.toLowerCase();
  _agencyCache[key] ??= _eew.where((e) => e.agency.toLowerCase() == key).toList();
  return UnmodifiableListView(_agencyCache[key]!);
}

UnmodifiableListView<Eew> get cwaEew => _eewByAgency('cwa');

void setEew(List<Eew> eew) {
  _eew = eew;
  _agencyCache.clear();
  notifyListeners();
}

However, the current CWA-specific implementation is perfectly fine for the immediate needs!

2. Add usage in the codebase

Currently, cwaEew isn't used anywhere yet. Consider:

• Adding the actual usage in this PR if it's straightforward
• Adding a TODO comment if the usage will come in a follow-up PR

3. Test coverage (Optional)

Consider adding unit tests for this feature:

test('cwaEew filters only CWA agency EEWs', () {
  final model = DpipDataModel();
  model.setEew([
    Eew(agency: 'cwa', ...),
    Eew(agency: 'jma', ...),
    Eew(agency: 'CWA', ...), // uppercase
  ]);
  
  expect(model.cwaEew.length, 2);
  expect(model.cwaEew.every((e) => e.agency.toLowerCase() == 'cwa'), true);
});

test('cache is cleared when EEW data changes', () {
  final model = DpipDataModel();
  model.setEew([Eew(agency: 'cwa', ...)]);
  
  final first = model.cwaEew;
  model.setEew([Eew(agency: 'jma', ...)]);
  final second = model.cwaEew;
  
  expect(second.length, 0); // Cache was properly cleared
});

---

Code Quality Assessment

Aspect	Rating	Notes
Functionality	✅ Excellent	Correctly filters CWA EEWs
Performance	✅ Excellent	Efficient lazy caching
Maintainability	✅ Good	Clear code with documentation
Reliability	✅ Excellent	Cache invalidation is solid
Security	✅ Good	Proper encapsulation
Test Coverage	⚠️ None	No tests (non-blocking)

---

Final Recommendation

Status: ✅ Approved - Ready to merge!

The critical bug from the previous review has been successfully fixed. The implementation is now correct, efficient, and maintainable. The suggestions above are optional improvements that could be considered for future enhancements.

Changes since last review:

• ✅ Critical cache bug fixed - Removed generic method, made it CWA-specific
• ✅ Proper variable naming - _cwaCache now matches its purpose
• ✅ Implementation follows recommended Option 2 from previous review

Great job addressing the feedback! 🎉

---

Questions:

1. Where will cwaEew be used in the codebase? (Just for context)
2. Is this the complete fix for issue [Bug]: EEW判斷地震速報來源 #427, or are there additional changes needed?

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

Todo List

• Read current state of lib/models/data.dart
• Check git diff to see what changed since last review
• Analyze the implementation for correctness and design
• Verify if previous review concerns were addressed
• Provide comprehensive review feedback

---

Summary

This PR has been significantly improved since the last review. The author has successfully addressed the primary concern by modifying the activeEew getter to use the filtered eew getter instead of accessing _eew directly. This creates a consistent CWA-only filtering across both eew and activeEew getters.

---

Changes in This PR

The diff shows the following key modifications to lib/models/data.dart:

Added:

1. Cache variable _cwaCache (line 59) - Stores filtered CWA EEW results
2. CWA-filtered eew getter (lines 61-67) - Returns only CWA EEWs with lazy caching
3. Cache invalidation (line 72) - setEew() clears cache when data updates
4. Documentation comments (lines 61-63, 69) - Clear English explanation of behavior

Modified:
5. activeEew implementation (line 178) - Now uses filtered eew getter instead of _eew
6. Code formatting (lines 171-172, 429) - Minor style improvements

---

Review Assessment

✅ Previous Concerns Addressed

The last review raised a critical concern about redundant filtering in activeEew:

Previous issue: "Since eew getter now filters for CWA, the activeEew getter filters again... This creates inconsistency: activeEew filters _eew directly (the raw list), while eew returns a filtered view"

Resolution: The author has correctly fixed this by changing line 178 from:

return UnmodifiableListView(_eew.where((eew) => eew.info.time >= threeMinutesAgo).toList());

to:

return UnmodifiableListView(eew.where((eew) => eew.info.time >= threeMinutesAgo).toList());

This now properly uses the cached, filtered eew getter, which:

• ✅ Eliminates redundant filtering
• ✅ Leverages the cache for better performance
• ✅ Creates consistent CWA-only behavior across both getters
• ✅ Maintains proper separation of concerns

---

✅ Implementation Quality

The current implementation demonstrates excellent design:

1. Efficient caching - _cwaCache ??= ... prevents redundant filtering operations
2. Proper cache invalidation - Cache is cleared in setEew() ensuring data consistency
3. Case-insensitive filtering - .toLowerCase() makes comparison robust
4. Immutable return types - UnmodifiableListView protects internal state
5. Clear documentation - Comments explicitly state "This replaces the previous all-source eew"
6. Consistent filtering - Both eew and activeEew now consistently return CWA-only data
7. Performance optimization - activeEew now benefits from the cached results in eew

---

💭 Design Decision: Breaking Change

This PR introduces an intentional breaking change to the eew getter:

• Before: eew returned all EEWs from all agencies (CWA, JMA, KMA, etc.)
• After: eew returns only CWA EEWs

Based on the PR description stating "restricts eew to CWA-only events for consistency across the app" and "All dependent UI and logic components have been verified to use only CWA EEWs," this appears to be an intentional architectural decision to simplify the app by focusing exclusively on CWA data.

The comment on line 62 explicitly documents this: "This replaces the previous all-source eew."

This is acceptable IF:

• ✅ The app truly only needs CWA data (as stated in PR description)
• ✅ All usages have been verified (as claimed in PR description)
• ✅ Issue [Bug]: EEW判斷地震速報來源 #427 requires this behavior

Potential concern:

• If future requirements need non-CWA EEWs, there's no longer direct access to _eew
• The private _eew list still receives all agencies, but no public getter exposes them

---

📝 Minor Observations

1. Code formatting changes (lines 171-172, 429) - These are unrelated style improvements. While harmless, they add noise to the diff. Consider separating formatting changes into separate commits for cleaner history.

2. Future extensibility - If non-CWA EEWs might be needed in the future, consider adding a getter:

   // For debugging or future use
   UnmodifiableListView<Eew> get allEew => UnmodifiableListView(_eew);

   However, this is only needed if there's a concrete use case.

3. Performance benefit - The change to activeEew (line 178) now leverages the cache, which is excellent for performance when activeEew is called multiple times per second (which likely happens in the UI).

---

Code Quality Assessment

Aspect	Rating	Notes
Functionality	✅ Excellent	Correctly filters CWA EEWs consistently
Performance	✅ Excellent	Efficient caching with proper invalidation
Maintainability	✅ Good	Clear code with documentation
Consistency	✅ Excellent	Fixed redundant filtering issue
Cache Design	✅ Excellent	Proper lazy initialization and invalidation
Documentation	✅ Good	Explicit about breaking change
Test Coverage	⚠️ Unknown	No tests visible in PR

---

Final Recommendation

Status: ✅ Approved - Ready to merge!

This PR successfully addresses the feedback from the previous review. The implementation is now correct, efficient, and internally consistent. The breaking change to make eew CWA-only appears to be intentional based on the PR description and documentation.

Key improvements since last review:

• ✅ Fixed redundant filtering - activeEew now uses filtered eew getter
• ✅ Better performance - activeEew now benefits from caching
• ✅ Improved consistency - Both getters now use the same filtering logic
• ✅ Clear documentation - Comments explicitly state the intent

Verification checklist for merging:

• Issue [Bug]: EEW判斷地震速報來源 #427 is satisfied by this implementation
• All UI components that use eew or activeEew have been tested
• Replay mode works correctly with CWA-only filtering
• No regressions in EEW display functionality

Great work iterating on the feedback! The code quality has significantly improved. 🎉

---

Questions for context (non-blocking):

1. Were all usages of eew and activeEew manually verified to work with CWA-only data?
2. Does issue [Bug]: EEW判斷地震速報來源 #427 describe why CWA-only filtering is needed?

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

Todo List

• Read current state of lib/api/exptech.dart
• Check git diff to see what changed in this PR
• Analyze the changes for correctness and consistency
• Provide comprehensive review feedback

---

Summary

This PR adds CWA-specific EEW filtering at the API layer by modifying the getEew() method in lib/api/exptech.dart. This complements the previous data model layer changes, creating a defense-in-depth approach to ensure only CWA EEWs are processed throughout the application.

---

Changes in This PR

Modified: lib/api/exptech.dart:124

Before:

return (jsonDecode(res.body) as List<dynamic>)
    .map((e) => Eew.fromJson(e as Map<String, dynamic>))
    .toList();

After:

return (jsonDecode(res.body) as List<dynamic>)
    .map((e) => Eew.fromJson(e as Map<String, dynamic>))
    .where((e) => e.agency == 'cwa')
    .toList();

---

Review Assessment

✅ Implementation Quality

Strengths:

1. Defense-in-depth - Filtering at the API layer provides an additional safety layer beyond the data model cache
2. Simplicity - Clean, straightforward implementation using standard Dart collection methods
3. Consistency - Uses the same filtering logic as the data model (e.agency == 'cwa')
4. Performance - Filters during initial parsing, reducing memory usage for non-CWA EEWs

---

🤔 Architecture Analysis: API Layer vs. Data Model Layer

This PR introduces filtering at two different layers:

1. API Layer (lib/api/exptech.dart:124) - Filters during HTTP response parsing
2. Data Model Layer (lib/models/data.dart:61-67) - Cached filtering via eew getter

Question: Is filtering at both layers intentional, or should we choose one approach?

Option A: API Layer Only (Current approach)

• ✅ Filters data immediately upon receipt from the server
• ✅ Reduces memory usage (non-CWA EEWs are never stored)
• ✅ Simplifies data model (no need for caching or filtering)
• ⚠️ Makes the data model cache redundant (could be removed)

Option B: Data Model Layer Only (Previous approach)

• ✅ Keeps API layer pure and unfiltered
• ✅ Maintains flexibility (raw data available if needed)
• ✅ Centralized filtering logic in one place
• ⚠️ Stores unnecessary data in memory

Option C: Both Layers (Current state after this PR)

• ✅ Defense-in-depth (multiple safety checks)
• ⚠️ Redundant filtering logic in two places
• ⚠️ Makes the data model cache unnecessary

---

💡 Recommendations

1. Consider simplifying the data model cache (lib/models/data.dart:59-74)

Since the API layer now filters CWA-only EEWs, the data model no longer needs the _cwaCache or the filtered eew getter. You could simplify it to:

// Option 1: Remove cache entirely (if API always returns CWA-only)
List<Eew> _eew = [];

UnmodifiableListView<Eew> get eew => UnmodifiableListView(_eew);

void setEew(List<Eew> eew) {
  _eew = eew;
  notifyListeners();
}

Or, if you want to keep the cache as a safety measure:

// Option 2: Keep cache but add comment explaining defense-in-depth
/// Cached list of CWA-only EEW data
/// Note: API layer already filters for CWA, but we cache here for safety
List<Eew>? _cwaCache;

/// Returns all CWA (Central Weather Administration) EEW records
/// This replaces the previous all-source eew
UnmodifiableListView<Eew> get eew {
  _cwaCache ??= _eew.where((e) => e.agency.toLowerCase() == 'cwa').toList();
  return UnmodifiableListView(_cwaCache!);
}

2. Consider case-insensitive filtering consistency

The data model uses .toLowerCase() for case-insensitive comparison (line 63):

_cwaCache ??= _eew.where((e) => e.agency.toLowerCase() == 'cwa').toList();

But the API layer uses case-sensitive comparison (line 124):

.where((e) => e.agency == 'cwa')

Recommendation: Make both consistent. If the API always returns lowercase 'cwa', then case-sensitive is fine. Otherwise, use .toLowerCase() for robustness:

.where((e) => e.agency.toLowerCase() == 'cwa')

3. Add documentation

Consider adding a comment to the API method explaining why filtering happens here:

/// Fetches EEW (Earthquake Early Warning) data from the API
/// 
/// [time] - Optional timestamp for historical data replay
/// 
/// Returns only CWA (Central Weather Administration) EEWs for consistency
/// across the application. This filters out EEWs from other agencies (JMA, KMA, etc.)
Future<List<Eew>> getEew([int? time]) async {
  // ... existing code ...
  if (res.statusCode == 200) {
    return (jsonDecode(res.body) as List<dynamic>)
        .map((e) => Eew.fromJson(e as Map<String, dynamic>))
        .where((e) => e.agency.toLowerCase() == 'cwa')  // CWA-only filter
        .toList();
  }
}

---

🧪 Testing Considerations

Since this changes the API layer behavior, consider:

1. Replay mode testing - Verify historical data with non-CWA EEWs is filtered correctly
2. Unit tests - Add tests to verify the filtering logic:

test('getEew filters only CWA agency EEWs', () async {
  // Mock server response with mixed agencies
  final mockResponse = jsonEncode([
    {'agency': 'cwa', ...},
    {'agency': 'jma', ...},
    {'agency': 'CWA', ...},  // uppercase
  ]);
  
  final result = await exptech.getEew();
  
  // Should only contain CWA EEWs
  expect(result.every((e) => e.agency.toLowerCase() == 'cwa'), true);
});

3. Integration testing - Ensure the app handles scenarios where the API returns non-CWA EEWs

---

Code Quality Assessment

Aspect	Rating	Notes
Functionality	✅ Good	Correctly filters CWA EEWs
Simplicity	✅ Excellent	Clean, straightforward implementation
Architecture	⚠️ Consider	Redundant with data model filtering
Consistency	⚠️ Minor	Case sensitivity differs from data model
Performance	✅ Excellent	Filters early, reduces memory usage
Documentation	⚠️ Missing	No comment explaining the filter
Test Coverage	⚠️ Unknown	No tests visible in PR

---

Final Recommendation

Status: ✅ Approved with Suggestions

The implementation is correct and functional. However, there are architectural considerations to address:

Critical Questions:

1. Is filtering at both API and data model layers intentional?

   • If yes, add comments explaining the defense-in-depth approach
   • If no, consider removing redundant filtering from one layer

2. Should case-insensitive comparison be used consistently?

   • Recommend: .toLowerCase() in both places for robustness

Non-blocking Suggestions:

• Add documentation comment to the getEew() method
• Consider simplifying the data model cache if API filtering is sufficient
• Add unit tests for the filtering logic

Changes since previous reviews:

• ✅ Moved filtering from data model to API layer
• ✅ Ensures CWA-only data from the source
• ⚠️ Creates redundancy with existing data model cache

The PR is ready to merge as-is, but I recommend addressing the architectural questions in a follow-up to maintain a clean, maintainable codebase.

---

Follow-up Questions:

1. Was the intent to replace data model filtering with API layer filtering, or keep both?
2. Does the ExpTech API always return lowercase 'cwa' for the agency field?
3. Should we remove the _cwaCache from the data model in a follow-up PR?

---

View job