feat: Drag-and-drop-homepage-style

[yayacat]

這是什麼類型的 PR？

選擇所有符合的項目

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

描述

拖曳首頁樣式

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

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

UI 無障礙清單

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

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

[claude]

Claude finished @yayacat's task in 2m 29s —— View job

---

PR Review: Drag-and-drop Homepage Style ✅

I've completed a comprehensive review of this PR implementing drag-and-drop customization for homepage sections.

Todo List

• Read changed files
• Analyze code quality and Flutter best practices
• Check for bugs and security issues
• Review performance considerations
• Assess test coverage
• Provide final review summary

---

Summary

This PR adds a useful drag-and-drop feature allowing users to customize the order and visibility of homepage sections (radar, forecast, history). The implementation is solid overall, with good separation of concerns and proper state management. However, there are 2 critical issues and several moderate improvements needed before merging.

---

⚠️ Critical Issues (Must Fix)

1. Missing Braces on if Statement (lib/app/home/page.dart:218-222)

Severity: High - Violates Effective Dart style guidelines

if (code == null) {
  if (mounted)  // ❌ Missing braces
    setState(() {
      _weather = null;
      _forecast = null;
    });
  return;
}

Fix Required:

if (code == null) {
  if (mounted) {
    setState(() {
      _weather = null;
      _forecast = null;
    });
  }
  return;
}

Impact: Violates flutter_style_todos: error and Effective Dart guidelines. Reduces code clarity and maintainability.

2. Hardcoded Debug Code (lib/app/home/page.dart:84-90)

Severity: High - Affects production UX and performance

bool get _isRaining {
  // TODO: 測試完成後移除強制啟用
  return true;  // ❌ Always returns true
  // if (_weather == null) return false;
  // final code = _weather!.data.weatherCode;
  // // 雨天代碼範圍：15-35(包含雨、大雨、雷雨)
  // return code >= 15 && code <= 35;
}

Impact:

• Rain shader animation always enabled regardless of weather
• Unnecessary battery drain and GPU usage
• Misleading UX (rain effect when not raining)

Fix Required: Either:

1. Uncomment the real logic if testing is complete
2. Remove the feature if not ready for production

---

🔧 Code Quality Issues

3. Deprecated API Usage (lib/app/settings/layout/page.dart:206, 214, 222, 246)

Severity: Medium

Multiple uses of deprecated withValues(alpha:):

context.colors.onSurfaceVariant.withValues(alpha: 0.5)  // Lines 206, 222
iconColor.withValues(alpha: 0.15)  // Line 214
context.colors.onSurfaceVariant.withValues(alpha: value ? 1 : 0.7)  // Line 246

Impact: Deprecation warnings, may break in future Flutter versions.

Recommendation: Use withOpacity() instead or update to Flutter's new color API pattern.

4. Missing Public API Documentation

Severity: Medium - Violates project's analysis_options.yaml (public_member_api_docs: warning)

Missing documentation for:

• SettingsUserInterfaceModel.homeSections (lib/models/settings/ui.dart:19)
• SettingsUserInterfaceModel.toggleSection() (lib/models/settings/ui.dart:78)
• SettingsUserInterfaceModel.reorderSection() (lib/models/settings/ui.dart:92)
• HomeDisplaySection enum values (lib/app/home/home_display_mode.dart:1-5)

Example Fix:

/// List of home page sections currently enabled and their display order.
/// 
/// This list determines which sections appear on the homepage and in what sequence.
/// Users can reorder sections via drag-and-drop in settings.
late List<HomeDisplaySection> homeSections;

5. Potential State Initialization Issue (lib/models/settings/ui.dart:59-74)

Severity: Low-Medium

The constructor initializes homeSections with logic that accesses Preference.homeDisplaySections:

SettingsUserInterfaceModel() {
  if (savedList.isEmpty) {
    homeSections = HomeDisplaySection.values.toList();
  } else {
    final saved = savedList
        .map((s) => HomeDisplaySection.values
            .cast<HomeDisplaySection?>()
            .firstWhere((e) => e?.name == s, orElse: () => null))
        .whereType<HomeDisplaySection>()
        .toList();
    homeSections = saved;
  }
}

Issues:

• savedList is a final field referencing Preference.homeDisplaySections (line 20)
• If preferences aren't initialized when this constructor runs, it could fail
• No error handling for preference load failures
• The cast<HomeDisplaySection?>() pattern is verbose

Recommendation:

SettingsUserInterfaceModel() {
  final saved = Preference.homeDisplaySections;
  if (saved.isEmpty) {
    homeSections = HomeDisplaySection.values.toList();
  } else {
    homeSections = saved
        .map((name) => HomeDisplaySection.values
            .where((e) => e.name == name)
            .firstOrNull)
        .whereType<HomeDisplaySection>()
        .toList();
  }
}

6. Magic Numbers Without Named Constants (lib/app/settings/layout/page.dart)

Severity: Low

Multiple hardcoded values:

• alpha: 0.5, 0.15, 0.7, 1.0 (opacity values)
• Padding/margins: 16, 12, 10, 6, 8, 2 (spacing values)

Recommendation: Extract frequently used values to named constants for consistency and maintainability.

---

🎯 Flutter Best Practices Assessment

✅ Strengths

1. Excellent State Management: Proper use of Provider with ChangeNotifier, Consumer, and Selector
2. Good Widget Key Usage: Correct use of ValueKey(section) for reorderable items
3. Proper Separation of Concerns: Settings UI logic cleanly separated from home page
4. Type Safety: Good use of enums for HomeDisplaySection
5. Responsive Design: Uses ResponsiveContainer appropriately
6. Data Persistence: Proper persistence through Preference system
7. Good Widget Composition: Clean widget tree with appropriate nesting

⚠️ Areas for Improvement

1. ReorderableListView Performance (lib/app/settings/layout/page.dart:43-69)

   • Uses shrinkWrap: true + physics: NeverScrollableScrollPhysics() inside parent ListView
   • This is acceptable for small lists (3 items currently) but document this assumption
   • If the list could grow, consider extracting to separate scrollable

2. Data Type Change: Set → List

   • Changed homeSections from Set<HomeDisplaySection> to List<HomeDisplaySection>
   • This is correct for ordered drag-and-drop, but ensure no other code assumes Set behavior
   • Good: Maintains order for UI display
   • Watch for: Performance if checking .contains() frequently (not an issue with 3 items)

---

🔒 Security & Performance

Security: ✅ No vulnerabilities detected

• No SQL injection, XSS, or OWASP top 10 concerns
• Proper input validation on enum values
• Safe state persistence

Performance:

• ⚠️ Rain shader always enabled - Critical performance issue (see Issue fix: earthquake 頁面 #2)
• ✅ Efficient state management with granular Selector usage
• ✅ Proper use of const constructors where applicable
• ⚠️ Nested scrollables with shrinkWrap (acceptable for small lists)

---

📋 Testing

Current State: ❌ No tests detected for new functionality

Recommended Tests:

1. Unit Tests (lib/models/settings/ui.dart):

   • Test toggleSection() adds/removes sections correctly
   • Test reorderSection() maintains correct order
   • Test preference persistence after state changes
   • Test initialization with empty/populated preferences

2. Widget Tests (lib/app/settings/layout/page.dart):

   • Test drag-and-drop reordering behavior
   • Test switch toggle enables/disables sections
   • Test section icons and colors render correctly

3. Integration Tests:

   • Test homepage sections appear in correct order
   • Test preference changes persist across app restarts

---

🎨 UI Accessibility Checklist

Based on PR template requirements:

變數名稱實現語意化命名？ ✅ Mostly Good

• Most variables are well-named (homeSections, enabledSections, disabledSections)
• Minor: savedList could be savedSectionNames for clarity

測試通過 AA 顏色對比？ ⚠️ Cannot Verify

• Opacity values like alpha: 0.5 on onSurfaceVariant may not meet AA contrast
• alpha: 0.7 for disabled sections - needs testing
• Recommendation: Test with accessibility tools (e.g., Flutter DevTools Accessibility Inspector)

---

📝 Positive Changes from Previous Review

The previous Claude review identified several issues. Here's the current status:

✅ FIXED: Duplicate file content (lib/app/home/page.dart) - file now has correct 756 lines
❌ NOT FIXED: Missing braces on if (mounted) (line 218)
❌ NOT FIXED: Hardcoded _isRaining = true debug code (line 85)
⚠️ PARTIALLY ADDRESSED: Deprecated API usage (still present in layout/page.dart)

---

🚀 Recommendations Priority

Must Fix Before Merge:

1. ⚠️ Fix missing braces on if (mounted) statement (line 218)
2. ⚠️ Remove or implement hardcoded _isRaining debug code (line 85)

Should Fix:
3. Replace deprecated withValues() API with withOpacity() or new API
4. Add public API documentation for new methods and properties
5. Run flutter analyze to catch linting issues
6. Run dart format lib/ to ensure consistent formatting

Consider:
7. Extract magic numbers to named constants
8. Add unit tests for state management logic
9. Test color contrast with accessibility tools
10. Simplify constructor initialization logic

---

📊 Architecture Assessment

Pattern Compliance: ✅ Excellent

• Follows project's Provider-based architecture
• Proper ChangeNotifier pattern with notifyListeners()
• Consistent with existing codebase patterns

Code Changes Summary:

• Changed homeSections from Set to List (correct for ordered display)
• Added reorderSection() method for drag-and-drop
• Modified toggleSection() to work with List instead of Set
• Refactored settings UI to use ReorderableListView
• Updated home page to render sections in user-defined order

---

Final Verdict

Status: ⚠️ Request Changes

This is a well-architected feature with good Flutter practices, but the 2 critical issues (missing braces and hardcoded debug code) must be addressed before merging. The deprecated API usage should also be fixed to avoid future maintenance issues.

Estimated Effort to Fix: ~30 minutes

• Critical fixes: 5-10 minutes
• Deprecated API replacements: 10-15 minutes
• Documentation: 10-15 minutes

Once these issues are resolved, this PR will be ready to merge! 🎉

---