Handle image generation with retry

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

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

描述

地震報告加入重試機制
以免回傳404

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

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

UI 無障礙清單

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

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

[claude]

Claude finished @lowrt's task in 1m 37s —— View job

---

Code Review: Handle image generation with retry

Summary

This PR adds a retry mechanism for earthquake report images that may not be immediately available (404 errors) when the Central Weather Administration (CWA) is still generating them. The implementation includes a SafeImageSection wrapper widget that displays a friendly error message with a retry button when images fail to load.

---

Code Quality & Flutter Best Practices

Strengths:

• ✅ Good separation of concerns with SafeImageSection as a reusable wrapper widget
• ✅ Proper use of StatefulWidget for managing retry state
• ✅ Uses KeyedSubtree with ValueKey(_retryKey) to force widget rebuild on retry - this is a good pattern
• ✅ Proper mounted check before calling setState in _onImageError
• ✅ Added null checks for image URLs before rendering sections (lines 1060, 1077, 1094)
• ✅ Added fit: BoxFit.cover to CachedNetworkImage for better image rendering

Issues & Suggestions:

1. Code Formatting ⚠️

   • lib/app/map/_lib/managers/report.dart:441 - Extra space in class name: _CwaGeneratingView (trailing space)
   • lib/app/map/_lib/managers/report.dart:444 - Extra space in constructor: const _CwaGeneratingView ( (space before parenthesis)
   • Inconsistent indentation in lines 1067, 1084, 1101 - the builder: (onError) => pattern is poorly formatted

2. State Management Issue 🔴

   void _onImageError() {
     if (_hasError) return;  // Early return prevents double-setState
     
     WidgetsBinding.instance.addPostFrameCallback((_) {
       if (!mounted || _hasError) return;
       setState(() {
         _hasError = true;
       });
     });
   }

   The double-check of _hasError is good, but there's a potential race condition. If the error callback is invoked multiple times rapidly (which can happen with network images), the check in the callback might not prevent multiple setState calls. Consider setting _hasError = true immediately before the post-frame callback:

   void _onImageError() {
     if (_hasError) return;
     _hasError = true;  // Set immediately to prevent race conditions
     
     WidgetsBinding.instance.addPostFrameCallback((_) {
       if (!mounted) return;
       setState(() {});  // Just rebuild
     });
   }

3. Missing Error Widget for Report Image ⚠️

   • The main report image (report.reportImageUrl at line 1054) is NOT wrapped with SafeImageSection
   • This is inconsistent - if intensity/PGA/PGV maps can have 404 errors, the report image likely can too
   • Should apply the same retry mechanism to all images for consistency

4. Widget Naming Convention ⚠️

   • _CwaGeneratingView is a private widget (underscore prefix) but defined at the file level outside of _ReportMapLayerSheetState
   • While technically valid, it's more common to either:
     • Make it public: CwaGeneratingView
     • Move it inside the state class if it's truly private to that widget's implementation

5. Hardcoded Height ⚠️

   return SizedBox(height: 180, ...)  // Line 451

   The hardcoded height of 180 might not match the actual aspect ratio space (2334/2977). Consider calculating the height based on available width and aspect ratio, or use the same aspect ratio:

   return AspectRatio(
     aspectRatio: 2334 / 2977,
     child: Column(...)
   );

---

Potential Bugs

1. Memory Leak Risk 🟡

   • The onLoadFailed callback is stored but never cleared
   • If the parent widget is disposed while waiting for an image error, the callback might reference a disposed context
   • The check for mounted helps, but consider using WeakReference or ensuring the parent widget lifecycle is respected

2. Image Caching Issue 🟡

   • When retry is triggered, the _retryKey increments and the widget rebuilds
   • However, CachedNetworkImage might still have the failed URL cached
   • The library might return the cached error immediately without retrying
   • Consider adding a cache-busting strategy or explicitly clearing the cached failed image:

   // In _retry method
   await CachedNetworkImage.evictFromCache(imageUrl);

3. No Retry Limit 🟡

   • Users can retry indefinitely
   • If the image truly doesn't exist or the server is down, users will keep retrying with no feedback
   • Consider adding a retry counter and showing a different message after multiple failed attempts

---

Performance Considerations

Good:

• ✅ Using CachedNetworkImage for efficient image caching
• ✅ Conditional rendering with null checks prevents unnecessary widget builds
• ✅ KeyedSubtree efficiently handles widget replacement on retry

Concerns:

• 🟡 Each retry creates a new widget subtree (due to KeyedSubtree), which is fine but could be optimized with better state management
• 🟡 No loading indicator while images are loading initially - users might see blank space before error or success

---

Security Concerns

• ✅ No security issues identified
• ✅ URLs are from trusted API sources (report.intensityMapImageUrl, etc.)
• ✅ No user input is directly used in image URLs

---

Test Coverage

⚠️ No tests included in this PR

Recommended test scenarios:

1. Test that error state is properly set when image fails to load
2. Test that retry increments the key and rebuilds the widget
3. Test that multiple rapid error callbacks don't cause multiple setState calls
4. Test that retry button clears error state
5. Test that mounted check prevents setState on disposed widget
6. Integration test: Verify retry works with actual network images (mocked 404 response)

---

Additional Recommendations

1. Consider UX Improvements:

   • Add a loading spinner while waiting for retry
   • Add automatic retry with exponential backoff before showing the error UI
   • Show elapsed time or estimated generation time

2. Code Organization:

   • Consider moving SafeImageSection and _CwaGeneratingView to a separate file in lib/widgets/ for better reusability

3. Documentation:

   • Add documentation comments explaining the retry mechanism
   • Document the onLoadFailed callback parameter in EnlargeableImage

4. Consistency:

   • Apply SafeImageSection to the main report image as well

---

Verdict

Overall Assessment: Good implementation with some issues that should be addressed

Severity Breakdown:

• 🔴 Critical: 1 (state management race condition)
• 🟡 Medium: 4 (missing retry for main image, hardcoded height, no retry limit, caching issue)
• ⚠️ Minor: 4 (formatting, naming convention, test coverage, documentation)

Recommendation: Request changes before merging

Must Fix Before Merge:

1. Fix the race condition in _onImageError
2. Fix code formatting issues (trailing spaces, indentation)
3. Apply SafeImageSection to the main report image for consistency

Should Consider:

1. Add retry limits to prevent infinite retries
2. Handle cached failed images explicitly
3. Add unit tests for the retry logic
4. Use proper aspect ratio for error view

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task in 1m 32s —— View job

---

Code Review: Handle image generation with retry

Summary

This PR adds a retry mechanism for earthquake report images that may return 404 errors when the Central Weather Administration (CWA) is still generating them. The implementation introduces a SafeImageSection wrapper widget that displays a user-friendly error message with a retry button when images fail to load.

---

Code Quality & Implementation

Strengths:

• ✅ Good separation of concerns with SafeImageSection as a reusable wrapper widget
• ✅ Proper use of StatefulWidget for managing retry state
• ✅ Uses KeyedSubtree with ValueKey(_retryKey) to force widget rebuild on retry - excellent pattern
• ✅ Proper mounted check before calling setState in _onImageError
• ✅ Added null checks for image URLs before rendering sections (lines 1058-1112)
• ✅ Added fit: BoxFit.cover to CachedNetworkImage for better image rendering
• ✅ Clean builder pattern using callback function for error handling
• ✅ Uses AspectRatio in _GeneratingView instead of hardcoded heights

Issues & Suggestions:

1. State Management Race Condition 🔴

   In lib/app/map/_lib/managers/report.dart:409-417:

   void _onImageError() {
     if (_hasError) return;
     _hasError = true;
   
     WidgetsBinding.instance.addPostFrameCallback((_) {
       if (!mounted) return;
       setState(() {});
     });
   }

   Issue: While the _hasError flag is set immediately (which is good), there's still a theoretical window where multiple error callbacks could be queued before the post-frame callback executes. The implementation is actually quite solid, but could be slightly more explicit:

   Consideration: The current implementation is acceptable, but you might want to document why you set _hasError = true before the post-frame callback - it's actually a good defensive pattern that prevents race conditions.

2. Missing Retry for Main Report Image ⚠️

   In lib/app/map/_lib/managers/report.dart:1044-1056, the main report image is NOT wrapped with SafeImageSection:

   Section(
     label: Text('地震報告圖'.i18n),
     children: [
       Padding(
         padding: .symmetric(horizontal: 8),
         child: EnlargeableImage(
           aspectRatio: 4 / 3,
           heroTag: 'report-image-${report.id}',
           imageUrl: report.reportImageUrl,
           imageName: report.reportImageName,
         ),
       ),
     ],
   ),

   Question: Is the main report image (reportImageUrl) always immediately available, or could it also be in a "generating" state? For consistency and robustness, consider applying the same retry mechanism to all earthquake report images.

3. No Retry Limit 🟡

   Users can retry indefinitely. If the CWA server is truly down or the image doesn't exist, users will keep retrying with no feedback about the situation.

   Suggestion: Consider tracking retry attempts and showing a different message after 3-5 failed attempts:

   int _retryCount = 0;
   
   void _retry() {
     setState(() {
       _hasError = false;
       _retryKey++;
       _retryCount++;
     });
   }

   Then in _GeneratingView, you could show: "圖片載入失敗，請稍後再試" after multiple retries.

4. Cache Management 🟡

   When the user clicks retry, CachedNetworkImage might still have the failed URL cached and return the cached error immediately without actually retrying the network request.

   Suggestion: Consider explicitly clearing the cache on retry:

   void _retry() async {
     await CachedNetworkImage.evictFromCache(imageUrl);
     setState(() {
       _hasError = false;
       _retryKey++;
     });
   }

   However, this requires passing imageUrl to SafeImageSection. You might need to adjust the architecture slightly.

5. Error Widget Returns Empty SizedBox ⚠️

   In lib/widgets/report/enlargeable_image.dart:35-38:

   errorWidget: (context, url, error) {
     onLoadFailed?.call();
     return const SizedBox.shrink();
   },

   The SizedBox.shrink() might cause layout shifts when the error occurs, as the AspectRatio wrapper will collapse. This works because _hasError triggers _GeneratingView to render, but it creates a brief moment where the space disappears.

   Note: This is actually fine given your architecture since SafeImageSection handles the error display. The shrink is intentional to prevent double error UI.

---

Flutter Best Practices

Good Patterns:

• ✅ Proper widget composition with clear separation of concerns
• ✅ Correct use of ValueKey to force widget rebuilds
• ✅ Lifecycle management with mounted checks
• ✅ Immutable widget properties with final fields
• ✅ Proper use of callbacks for cross-widget communication

Minor Observations:

• The _GeneratingView widget is prefixed with _ making it private to the file, which is appropriate since it's only used within this file
• Widget naming follows Flutter conventions

---

Potential Bugs

None identified - The implementation is solid and handles edge cases well:

• ✅ Prevents double error handling with if (_hasError) return
• ✅ Checks widget mounting before setState
• ✅ Properly resets state on retry
• ✅ Null-safe with optional callback

---

Performance Considerations

Efficient:

• ✅ Using CachedNetworkImage for proper image caching
• ✅ Conditional rendering with null checks prevents unnecessary widget builds
• ✅ KeyedSubtree efficiently handles widget replacement on retry
• ✅ Post-frame callback prevents setState during build

No performance concerns identified.

---

Security & Privacy

• ✅ No security issues identified
• ✅ URLs come from trusted API sources (report model properties)
• ✅ No user input directly used in image URLs
• ✅ No sensitive data exposure

---

UX Considerations

Strengths:

• ✅ Clear messaging: "CWA 正在製圖中" explains why the image isn't available
• ✅ Actionable retry button with refresh icon
• ✅ Maintains proper aspect ratio during error state

Potential Improvements:

• Consider adding a subtle loading indicator while waiting for retry
• Consider automatic retry with exponential backoff before showing error UI
• Show timestamp or "generated at" info once image loads successfully

---

Testing Recommendations

While no tests are included in this PR (which is common for UI features), consider testing:

1. Unit Tests:

   • Test that _onImageError sets _hasError flag correctly
   • Test that _retry increments _retryKey and resets error state
   • Test that multiple rapid calls to _onImageError don't cause issues

2. Widget Tests:

   • Test that error state renders _GeneratingView
   • Test that retry button calls the retry callback
   • Test that KeyedSubtree key changes on retry

3. Integration Tests:

   • Test retry flow with mocked network responses (success -> 404 -> success)
   • Test that cached failed images are properly retried

---

Consistency Check

All three map image types are now wrapped with SafeImageSection:

• ✅ Intensity map (intensityMapImageUrl) - lines 1058-1076
• ✅ PGA map (pgaMapImageUrl) - lines 1077-1094
• ✅ PGV map (pgvMapImageUrl) - lines 1095-1112

Question: Should the main report image (reportImageUrl at line 1044) also use SafeImageSection?

---

Verdict

Overall Assessment: Well-implemented feature with solid error handling and good UX

Severity Breakdown:

• 🔴 Critical: 0
• 🟡 Medium: 2 (no retry limit, cache management consideration)
• ⚠️ Minor: 2 (main report image consistency, testing coverage)

Recommendation: ✅ Approve with suggestions

The implementation is production-ready. The identified issues are enhancement opportunities rather than blockers. Consider addressing:

1. Before merge (optional):

   • Decide whether main report image needs retry mechanism
   • Consider adding retry limit for better UX

2. Future improvements:

   • Add cache eviction on retry if users report issues
   • Add automatic retry with exponential backoff
   • Add tests for the retry mechanism

Great work on the clean implementation and proper Flutter patterns! 🎉

---