Refactor weather information

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

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

描述

重構首頁天氣資訊

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

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

UI 無障礙清單

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

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

[claude]

Claude finished @lowrt's task —— View job

---

PR Review: Refactor weather information

I've completed a comprehensive review of this weather refactoring PR. Here are my findings:

---

✅ Positive Changes

1. Simplified Weather Schema (lib/api/model/weather_schema.dart)

   • Clean refactoring from complex nested structures to simpler, focused classes
   • Good use of @JsonSerializable() for type safety
   • Removed ~111 lines while adding only 44, indicating better design

2. New Forecast Card Widget (lib/app/home/_widgets/forecast_card.dart)

   • Well-structured expandable forecast display
   • Good use of state management for expansion tracking
   • Proper error handling with try-catch
   • Nice UX with pagination and smooth animations

3. Improved Weather Header (lib/app/home/_widgets/weather_header.dart)

   • Clean component architecture with separation of concerns
   • Good skeleton loading state
   • Proper use of Selector for performance optimization
   • Clean visual hierarchy with info chips

---

⚠️ Issues & Concerns

Critical Issues

1. Missing Error Handling in ForecastCard (lib/app/home/_widgets/forecast_card.dart:32-189)

   • The entire build() method is wrapped in a generic try-catch that silently returns SizedBox.shrink()
   • If data parsing fails, users won't know why forecasts aren't showing
   • Recommendation: Add logging and show an error state to users

   } catch (e) {
     TalkerManager.instance.error('Failed to render forecast', e);
     return ErrorWidget('Unable to load forecast');
   }

2. Potential Type Safety Issues (lib/app/home/_widgets/forecast_card.dart:38-42)

   for (final item in data) {
     final temp = (item['temperature'] as num?)?.toDouble() ?? 0.0;
     minTemp = min(minTemp, temp);
     maxTemp = max(maxTemp, temp);
   }

   • Using untyped Map<String, dynamic> instead of proper models
   • Multiple nullable casts throughout the widget
   • Recommendation: Create a proper ForecastItem model class with json_serializable

3. Magic Numbers (lib/app/home/_widgets/forecast_card.dart:45, 105, 127)

   • Hard-coded 4 for items per page
   • Hard-coded 84 and 320 for heights
   • Recommendation: Extract as named constants:

   static const int _itemsPerPage = 4;
   static const double _collapsedItemHeight = 84.0;
   static const double _expandedItemHeight = 320.0;

Code Quality Issues

4. Complex Height Calculation Logic (lib/app/home/_widgets/forecast_card.dart:99-156)

   • The height calculation and measurement logic is quite complex
   • Uses global keys, post-frame callbacks, and manual measurements
   • Could be simplified or at least extracted to helper methods
   • Recommendation: Extract to separate methods like _calculatePageHeight() and _measureActualHeight()

5. Inconsistent i18n (lib/app/home/_widgets/forecast_card.dart:75, 391-433)

   • '天氣預報(24h)'.i18n uses i18n
   • But hardcoded Chinese labels like '氣溫', '體感', '風速' in _buildDetailChip
   • Recommendation: Add i18n to all user-facing strings

6. Duplicate Icon Logic (lib/app/home/_widgets/forecast_card.dart:496-508)

   • Weather icon mapping is duplicated here
   • WeatherIcons.getWeatherIcon() already exists in weather_header.dart
   • Recommendation: Consolidate into a shared utility

7. State Management Complexity (lib/app/home/_widgets/forecast_card.dart:19-21)

   final Set<int> _expandedItems = {};
   final Map<int, double> _measuredHeights = {};
   final Map<int, GlobalKey> _pageKeys = {};

   • Multiple interdependent state variables
   • Could benefit from a single state class or riverpod state management

Performance Concerns

8. Unnecessary Rebuilds (lib/app/home/_widgets/forecast_card.dart:144-156)

   • addPostFrameCallback called on every build
   • Could trigger unnecessary setState calls
   • Recommendation: Add guards to prevent redundant updates

9. API Response Handling (lib/api/exptech.dart:268-285)

   • getWeatherForecast() returns untyped Map<String, dynamic>
   • No validation of response structure
   • Recommendation: Create a proper model class with validation

Best Practices

10. Missing Documentation

    • No doc comments on public classes and methods
    • Complex logic like height calculation needs explanation
    • Recommendation: Add dartdoc comments, especially for ForecastCard and its parameters

11. Missing Null Safety Guards (lib/app/home/page.dart:201)

    final forecast = await ExpTech().getWeatherForecast(code);
    if (mounted) setState(() => _forecast = forecast);

    • Should validate forecast structure before using
    • Recommendation: Add validation or use try-catch with specific error handling

12. Hard-coded Strings (lib/app/home/_widgets/weather_header.dart:118, 222)

    • '體感 $displayFeelsLike°'.i18n
    • '${weather.station.name}氣象站'
    • Some use i18n, some don't - inconsistent approach

---

🔍 Security & Privacy

• ✅ No hardcoded credentials or API keys
• ✅ Proper use of environment-based API endpoints
• ✅ No PII exposure in logs

---

🧪 Testing

• ❌ No test files found in the repository
• ❌ No unit tests for new ForecastCard widget
• ❌ No integration tests for weather data flow
• Recommendation: Add tests, especially for:
  • Weather data parsing
  • Forecast card expansion/pagination logic
  • Error handling scenarios
  • Height calculation edge cases

---

📊 Performance

• ⚠️ Weather header calculates "feels like" temperature on every build (lib/app/home/_widgets/weather_header.dart:55-60)
  • Recommendation: Memoize or calculate once
• ✅ Good use of Selector for targeted rebuilds
• ✅ Proper dispose() cleanup for controllers and listeners

---

🎨 UI/UX

• ✅ Semantic variable naming checked
• ⚠️ Color contrast not verified (AA standard checkbox not completed in PR description)
• ✅ Nice loading skeletons with Skeletonizer
• ✅ Smooth animations and transitions
• ✅ Good responsive design with LayoutBuilder

---

📝 Recommendations Summary

High Priority:

1. Add proper error handling and user feedback for forecast failures
2. Create typed models for forecast data instead of raw maps
3. Add i18n to all hardcoded Chinese strings
4. Add unit tests for critical paths

Medium Priority:
5. Extract magic numbers to named constants
6. Simplify height calculation logic
7. Consolidate duplicate icon mapping logic
8. Add dartdoc comments

Low Priority:
9. Memoize "feels like" temperature calculation
10. Consider more robust state management for complex widgets

---

✅ Approval Status

This is a solid refactoring with good improvements to code organization and UX. However, I recommend addressing the High Priority items, particularly:

• Adding error handling with user feedback
• Creating typed models
• Adding test coverage

The code is functional and follows Flutter best practices overall, but would benefit from better type safety and error handling before merging.

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

PR Review: Refactor weather information

Review completed ✅

---

Overview

This PR refactors the weather information display on the home page, introducing a new forecast card and simplifying the weather schema. Overall, this is a solid refactoring with good architectural improvements. Below are my detailed findings.

---

✅ Positive Changes

1. Simplified Weather Schema (lib/api/model/weather_schema.dart)

   • Clean refactoring from complex nested structures to focused, simple classes
   • Proper use of @JsonSerializable() for type safety
   • Removed ~111 lines while adding only 44 lines
   • All models are properly typed with immutable fields

2. New Forecast Card Widget (lib/app/home/_widgets/forecast_card.dart)

   • Well-structured expandable forecast display with pagination
   • Good use of state management for expansion tracking
   • Smooth animations with AnimatedSize
   • Proper disposal of PageController
   • Nice visual design with temperature visualization bars

3. Improved Weather Header (lib/app/home/_widgets/weather_header.dart)

   • Clean component architecture with proper separation of concerns
   • Good skeleton loading state for better UX
   • Proper use of Selector for performance optimization
   • Uses centralized WeatherIcons utility class (good code reuse)

4. API Layer (lib/api/exptech.dart:268-285, lib/api/route.dart:57)

   • New forecast endpoint properly integrated
   • Good debug logging for troubleshooting

---

⚠️ Issues & Recommendations

High Priority

1. Silent Error Handling in ForecastCard (lib/app/home/_widgets/forecast_card.dart:191-193)

   } catch (e) {
     return const SizedBox.shrink();
   }

   • The entire build() method has a generic try-catch that silently returns empty widget
   • Users won't know why forecasts aren't showing if data parsing fails
   • No logging of errors

   Recommendation: Add error logging and user feedback:

   } catch (e, stackTrace) {
     TalkerManager.instance.error('Failed to render forecast card', e, stackTrace);
     return Card(
       child: Padding(
         padding: EdgeInsets.all(16),
         child: Text('無法載入天氣預報'.i18n),
       ),
     );
   }

2. Untyped API Response (lib/api/exptech.dart:268-285)

   • getWeatherForecast() returns raw Map<String, dynamic> instead of a typed model
   • ForecastCard accesses data with string keys and runtime type casting
   • No validation of response structure

   Recommendation: Create a proper model class:

   @JsonSerializable()
   class WeatherForecast {
     final List<ForecastItem> forecast;
     
     WeatherForecast({required this.forecast});
     
     factory WeatherForecast.fromJson(Map<String, dynamic> json) => 
       _$WeatherForecastFromJson(json);
   }
   
   @JsonSerializable()
   class ForecastItem {
     final String time;
     final String weather;
     final int pop;
     final double temperature;
     final double apparentTemp;
     final ForecastWind wind;
     final int humidity;
     
     // ... constructor and fromJson
   }

3. Inconsistent i18n (lib/app/home/_widgets/forecast_card.dart)

   • Line 76: '天氣預報(24h)'.i18n ✅ uses i18n
   • Lines 395-437: Labels like '氣溫', '體感', '風速', '風向', '風級', '濕度', '降雨機率' all use .i18n ✅
   • Line 424: '${windBeaufort}級'.i18n ✅ uses i18n

   Actually, I see the i18n is applied properly. Good job!

4. Missing Error Handling in Page.dart (lib/app/home/page.dart:201-202)

   final forecast = await ExpTech().getWeatherForecast(code);
   if (mounted) setState(() => _forecast = forecast);

   • No validation of forecast structure
   • Should be wrapped in the existing try-catch (it is, so this is fine)
   • But could benefit from specific error messaging

Medium Priority

5. Magic Numbers (lib/app/home/_widgets/forecast_card.dart)

   • Line 46: for (int i = 0; i < data.length; i += 4) - hardcoded 4 items per page
   • Line 104: final globalIndex = pageIndex * 4 + i; - another hardcoded 4
   • Line 106: height += isExpanded ? 320 : 84; - hardcoded heights

   Recommendation: Extract as named constants:

   static const int _itemsPerPage = 4;
   static const double _collapsedItemHeight = 84.0;
   static const double _expandedItemHeight = 320.0;

6. Duplicate Icon Logic (lib/app/home/_widgets/forecast_card.dart:501-513)

   • Weather icon mapping is duplicated in _getWeatherIcon()
   • WeatherIcons utility already exists in lib/utils/weather_icon.dart
   • The ForecastCard uses string-based weather matching (contains '晴', '雨', etc.)
   • WeatherHeader uses numeric weather codes via WeatherIcons.getWeatherIcon()

   Recommendation: The forecast API returns weather as strings, not codes, so the current string-based approach is appropriate. However, consider consolidating this logic into the WeatherIcons utility class:

   // In WeatherIcons class
   static Icon getWeatherIconFromString(String weather, BuildContext context) {
     if (weather.contains('晴')) return Icon(Icons.wb_sunny, color: Colors.orange, size: 16);
     // ... rest of logic
   }

7. Complex Height Calculation (lib/app/home/_widgets/forecast_card.dart:99-161)

   • Height calculation using GlobalKey, post-frame callbacks, and manual measurements
   • Multiple interdependent state variables: _expandedItems, _measuredHeights, _pageKeys, _measuringPages
   • Logic is quite complex but appears necessary for dynamic height animation

   Recommendation: While complex, this appears to be intentional for smooth animations. Consider adding doc comments explaining the approach:

   /// Calculates the height of a page based on its items' expansion state.
   /// Uses measured heights when available, falls back to calculated heights.
   double calculatePageHeight(int pageIndex) { ... }

8. Performance: Repeated Calculations (lib/app/home/_widgets/weather_header.dart:55-60)

   final e = weather.data.humidity / 100 * 6.105 * 
            exp(17.27 * weather.data.temperature / (weather.data.temperature + 237.3));
   final feelsLike = weather.data.temperature + 0.33 * e - 0.7 * weather.data.wind.speed - 4.0;

   • Feels-like temperature calculated on every build
   • Could be memoized or calculated once in the model

   Recommendation: Since this widget already uses Selector for performance, and the calculation is relatively cheap, this is acceptable. However, if performance becomes an issue, consider moving this calculation to a late final field or computed property in the model.

Low Priority

9. Missing Documentation

   • No dartdoc comments on public classes like ForecastCard, WeatherHeader
   • Complex logic (height calculation, expansion management) needs explanation

   Recommendation: Add dartdoc comments:

   /// Displays a paginated, expandable 24-hour weather forecast card.
   /// 
   /// Each page shows up to 4 forecast items. Items can be expanded to show
   /// detailed weather information including temperature, wind, and humidity.
   class ForecastCard extends StatefulWidget { ... }

10. Type Safety in ForecastCard (lib/app/home/_widgets/forecast_card.dart:38-42, 203-212)

    • Multiple nullable casts: (item['temperature'] as num?)?.toDouble() ?? 0.0
    • Using untyped Map<String, dynamic> instead of proper models
    • This relates to issue fix: earthquake 頁面 #2 above about creating typed models

---

🧪 Testing

• ❌ No test files found in this PR
• ❌ No unit tests for new ForecastCard widget
• ❌ No tests for forecast data parsing

Recommendation: Add tests for:

• Forecast data parsing edge cases (null values, missing fields)
• ForecastCard expansion/collapse behavior
• Pagination logic
• Height calculation edge cases
• Error handling scenarios

---

🎨 UI/UX

• ✅ Semantic variable naming
• ⚠️ Color contrast checkbox not completed (PR description states it wasn't tested for AA standard)
• ✅ Nice loading skeleton with Skeletonizer
• ✅ Smooth animations and transitions
• ✅ Good responsive design with LayoutBuilder
• ✅ Proper error states (weather fetch failures show snackbar)

---

🔒 Security & Privacy

• ✅ No hardcoded credentials or API keys
• ✅ Proper use of environment-based API endpoints
• ✅ No PII exposure in logs (debug logs only show status codes and body lengths)

---

📊 Code Quality

Good Practices:

• ✅ Proper disposal of resources (PageController)
• ✅ Mounted checks before setState
• ✅ Use of const constructors where possible
• ✅ Proper null safety
• ✅ Good separation of concerns
• ✅ Centralized weather icon mapping in utility class

Areas for Improvement:

• Extract magic numbers to constants
• Add dartdoc comments
• Create typed models for API responses
• Improve error handling with user feedback

---

📝 Summary & Recommendations

Must Address (High Priority):

1. ✅ Fix silent error handling in ForecastCard - add logging and user feedback
2. ✅ Create typed model classes for forecast data instead of raw Map<String, dynamic>
3. ✅ Test color contrast for AA accessibility (as noted in PR checklist)

Should Address (Medium Priority):
4. Extract magic numbers to named constants
5. Consider consolidating weather icon logic
6. Add dartdoc comments to public APIs

Nice to Have (Low Priority):
7. Add unit tests for forecast card
8. Consider memoizing feels-like temperature calculation if performance becomes an issue

---

✅ Approval Recommendation

Conditional Approval - This is a well-structured refactoring with good improvements to code organization and UX. The code is functional and follows Flutter best practices.

Before merging, please address:

1. Silent error handling (add logging and user feedback)
2. Type safety (create proper model classes for forecast data)
3. Accessibility testing (verify AA color contrast)

After addressing these three items, this PR will be ready to merge.

---

Great work on the refactoring! The new forecast card is visually appealing and the code structure is much cleaner. The main improvements needed are around error handling transparency and type safety.

---