Skip to content

Fix SnackBar toast - #431

Closed
lowrt wants to merge 9 commits into
mainfrom
refactor/toast
Closed

Fix SnackBar toast#431
lowrt wants to merge 9 commits into
mainfrom
refactor/toast

Conversation

@lowrt

@lowrt lowrt commented Nov 5, 2025

Copy link
Copy Markdown
Contributor

這是什麼類型的 PR?

選擇所有符合的項目

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

描述

相關 issue

  • 相關問題 #
  • closes #

QA 指南、截圖、錄像

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

UI 無障礙清單

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

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

@lowrt

lowrt commented Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

@claude review

@ExpTechTW ExpTechTW deleted a comment from claude Bot Nov 5, 2025
@claude

claude Bot commented Nov 5, 2025

Copy link
Copy Markdown

Claude finished @lowrt's task —— View job


Code Review for Toast Refactoring

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Review changed files
  • Analyze toast refactoring implementation
  • Provide comprehensive feedback

Summary

This PR successfully refactors the toast implementation from the external fluttertoast dependency to Flutter's native SnackBar system. The refactoring is well-executed with a custom animation implementation and maintains the same visual design.

Positive Points

Dependency Reduction: Successfully removes the fluttertoast dependency (8.2.12), reducing the app's dependency footprint.

Native Implementation: Uses Flutter's built-in SnackBar which is more maintainable and better integrated with the Material Design system.

Custom Animation: The AnimatedFade widget provides smooth fade-in/fade-out animations matching the previous behavior.

Visual Design: The ToastWidget maintains a consistent design with proper theming, shadows, and responsive layout.

Theme Integration: Text color properly uses Theme.of(context).colorScheme.onSurface for better theme support.

Issues Found

🔴 Critical: Memory Leak in AnimatedFade

Location: lib/utils/toast.dart:36-38

Future.delayed(const Duration(seconds: 3), () {
  if (mounted) _controller.reverse();
});

Problem: The AnimatedController is not disposed properly, and the Future.delayed callback may execute after the widget is disposed.

Impact: This will cause memory leaks when toasts are shown repeatedly, as the controllers are never properly cleaned up.

Recommendation: Override the dispose method:

@override
void dispose() {
  _controller.dispose();
  super.dispose();
}

🟡 Medium: Missing Key in AnimatedFade Constructor

Location: lib/utils/toast.dart:14-16

class AnimatedFade extends StatefulWidget {
  final Widget child;
  const AnimatedFade({required this.child});

Problem: The constructor doesn't accept a Key parameter, which is a Flutter best practice for StatefulWidgets.

Recommendation: Add key parameter:

const AnimatedFade({super.key, required this.child});

🟡 Medium: Unnecessary Function Wrapper

Location: lib/route/image_viewer/image_viewer.dart:108-111

void showImageSavedToast() {
  showToast(context, ToastWidget.text('已儲存圖片'.i18n, icon: const Icon(Symbols.check_rounded)));
}
showImageSavedToast();

Problem: The local function showImageSavedToast() is immediately called and serves no purpose.

Recommendation: Call showToast directly:

showToast(context, ToastWidget.text('已儲存圖片'.i18n, icon: const Icon(Symbols.check_rounded)));

🟢 Minor: Missing SnackBar Duration Control

Location: lib/utils/toast.dart:4-11

Observation: The default SnackBar duration is 4 seconds, but the animation is designed for 3 seconds (fade-out starts at 3 seconds). This creates a 1-second gap where the toast is invisible but still present.

Recommendation: Set explicit duration on the SnackBar:

final snackBar = SnackBar(
  behavior: SnackBarBehavior.floating,
  backgroundColor: Colors.transparent,
  elevation: 0,
  duration: const Duration(seconds: 3, milliseconds: 400), // Match animation timing
  content: AnimatedFade(child: Center(child: toast)),
);

🟢 Minor: Inconsistent Formatting

Location: lib/utils/toast.dart:30-31

late final Animation<double> _opacity =
  CurvedAnimation(parent: _controller, curve: Curves.easeInOut);

Observation: Inconsistent indentation (alignment doesn't match the rest of the codebase).

Recommendation: Standard formatting:

late final Animation<double> _opacity =
    CurvedAnimation(parent: _controller, curve: Curves.easeInOut);

Testing Recommendations

  1. Memory Testing: Run the app with multiple toast invocations and monitor memory usage using DevTools to verify no leaks after fixing the dispose issue.

  2. Animation Testing: Verify the fade-in and fade-out animations work smoothly and the timing matches expectations.

  3. Theme Testing: Test with both light and dark themes to ensure text contrast is appropriate.

  4. Multiple Toasts: Test showing multiple toasts in quick succession to verify the SnackBar queue behavior is acceptable (native SnackBars queue, while the old implementation might have stacked).

Conclusion

The refactoring is architecturally sound and achieves its goal of removing the external dependency. However, the critical memory leak issue must be fixed before merging. The other issues are lower priority but should be addressed for code quality and maintainability.

Recommendation: Request changes to fix the AnimationController disposal issue.


Changed Files Analysis:

  • ios/Podfile.lock - Correctly removes fluttertoast dependency
  • pubspec.yaml & pubspec.lock - Properly removes package reference
  • ⚠️ lib/utils/toast.dart - Good refactoring but needs dispose method
  • ⚠️ lib/route/image_viewer/image_viewer.dart - Works but has unnecessary wrapper function

@lowrt

lowrt commented Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Nov 5, 2025

Copy link
Copy Markdown

Claude finished @lowrt's task —— View job


Code Review for Toast Refactoring

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Review changed files
  • Analyze current implementation
  • Provide comprehensive feedback

Summary

This PR successfully refactors the toast implementation from the external fluttertoast dependency to Flutter's native SnackBar system. The implementation is well-executed and addresses all critical issues from the previous review.


Positive Points

Dependency Reduction: Successfully removes the fluttertoast dependency (8.2.12), reducing the app's dependency footprint and eliminating external maintenance concerns.

Native Implementation: Uses Flutter's built-in SnackBar which is more maintainable and better integrated with the Material Design system.

Custom Animation: The AnimatedFade widget provides smooth fade-in/fade-out animations matching the previous behavior with proper timing.

Memory Management: The AnimationController is now properly disposed in the dispose() method (lines 45-48), preventing memory leaks.

Widget Key Support: The AnimatedFade constructor properly accepts a Key parameter using super.key (line 17).

Visual Design: The ToastWidget maintains a consistent design with proper theming, shadows, and responsive layout.

Theme Integration: Text color properly uses Theme.of(context).colorScheme.onSurface for better dark/light theme support (line 66).

Clean Implementation: The code in image_viewer.dart is now cleaner with direct showToast calls (line 108).


Issues Found

🟡 Minor: SnackBar Duration Mismatch

Location: lib/utils/toast.dart:9

Current Code:

duration: const Duration(seconds: 3),

Observation: The SnackBar duration is set to 3 seconds, but the animation controller's reverse animation starts at 3 seconds (line 36). This means the fade-out animation happens right as the SnackBar is about to dismiss itself. The default SnackBar duration is actually 4 seconds, but here it's been set to 3 seconds, which could cause the widget to be removed before the fade-out animation completes.

Recommendation: Add a small buffer to ensure the fade-out animation completes:

duration: const Duration(seconds: 3, milliseconds: 500),

Impact: Low - The current implementation works, but adding a buffer would ensure smoother dismissal.


🟢 Observation: SnackBar Behavior Change

Location: lib/utils/toast.dart:4-12

Note: The migration from fluttertoast to SnackBar introduces a behavioral change:

  • Old behavior (fluttertoast): Multiple toasts could stack/overlay simultaneously
  • New behavior (SnackBar): SnackBars are queued - showing a new one dismisses the previous one

Impact: This is generally acceptable and follows Material Design guidelines, but users may notice the difference if they trigger multiple toasts rapidly.


Code Quality

Formatting: Code follows Dart formatting conventions with proper indentation.

Error Handling: Async operations are properly handled with mounted checks (line 37).

Widget Composition: Good separation of concerns with AnimatedFade, ToastWidget, and showToast.

Factory Constructor: The ToastWidget.text factory constructor (lines 55-72) provides a clean API for the common use case.


Testing Recommendations

  1. Animation Testing: Verify the fade-in and fade-out animations are smooth across different devices.

  2. Theme Testing: Test with both light and dark themes to ensure text contrast meets AA standards (as noted in the PR checklist).

  3. Multiple Toasts: Test showing multiple toasts in quick succession to verify the SnackBar queue behavior is acceptable for your use case.

  4. Memory Testing: Run the app with multiple toast invocations and verify no memory leaks using Flutter DevTools (the dispose method should handle this properly now).

  5. Context Validity: Test edge cases where context might become invalid (e.g., navigating away immediately after triggering a toast).


Conclusion

The refactoring is well-implemented and all critical issues from the previous review have been addressed. The code quality is high, with proper resource management, theme support, and clean architecture.

Recommendation: ✅ Approve with minor suggestion

The only remaining item is a minor timing consideration for the SnackBar duration, which is optional to address. The code is production-ready as-is.


Changed Files Analysis:

  • ios/Podfile.lock - Correctly removes fluttertoast dependency
  • pubspec.yaml & pubspec.lock - Properly removes package reference
  • lib/utils/toast.dart - Excellent refactoring with proper dispose method
  • lib/route/image_viewer/image_viewer.dart - Clean implementation with direct toast calls

@lowrt

lowrt commented Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Nov 5, 2025

Copy link
Copy Markdown

Claude finished @lowrt's task —— View job


Code Review for Toast Refactoring

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Review changed files
  • Analyze current implementation
  • Provide comprehensive feedback

Summary

This PR successfully refactors the toast implementation from the external fluttertoast dependency (v8.2.12) to Flutter's native SnackBar system. The implementation is production-ready with excellent code quality.


What Changed

The refactoring replaces the fluttertoast package with a custom implementation using:

  • SnackBar with floating behavior for positioning
  • AnimatedFade widget for smooth fade-in/fade-out animations
  • ToastWidget for consistent visual design
  • Proper theming support using Material Design color scheme

Positive Points

Dependency Reduction: Successfully removes the fluttertoast dependency (v8.2.12), reducing the app's dependency footprint and eliminating external maintenance concerns.

Native Implementation: Uses Flutter's built-in SnackBar which is more maintainable and better integrated with the Material Design system.

Custom Animation: The AnimatedFade widget (lib/utils/toast.dart:15-49) provides smooth fade-in/fade-out animations with proper timing:

  • Fade-in duration: Durations.short4 (~600ms)
  • Display time: 3 seconds
  • Fade-out duration: Durations.short4 (~600ms)
  • Total lifecycle: ~4.2 seconds

Memory Management: The AnimationController is properly disposed in the dispose() method (lib/utils/toast.dart:45-48), preventing memory leaks.

Widget Best Practices:

  • Constructor properly accepts Key parameter using super.key (lib/utils/toast.dart:17)
  • Async operations use mounted check to prevent errors after widget disposal (lib/utils/toast.dart:37)

Visual Design: The ToastWidget maintains a consistent design with:

  • Proper theming using context.colors.surfaceContainer and outlineVariant
  • Material elevation shadows (kElevationToShadow[8])
  • Responsive layout with Row and Flexible widgets
  • Pill-shaped border radius (BorderRadius.circular(9999))

Theme Integration: Text color properly uses Theme.of(context).colorScheme.onSurface (lib/utils/toast.dart:66) for better dark/light theme support and AA contrast compliance.

Clean API: The ToastWidget.text factory constructor (lib/utils/toast.dart:55-72) provides a clean, ergonomic API for the common use case.

Code Formatting: The code in image_viewer.dart has been cleaned up with consistent formatting and direct showToast calls.


Areas of Excellence

🌟 Animation Timing: The duration has been carefully tuned in the latest commit (88d384a). The SnackBar duration of 3.5 seconds (Duration(seconds: 3, milliseconds: 500)) provides a 500ms buffer after the fade-out animation starts at 3 seconds, ensuring smooth dismissal without abrupt removal.

🌟 Resource Management: Excellent lifecycle management with proper dispose() implementation preventing memory leaks that are common when using AnimationController.

🌟 Builder Pattern: Smart use of Builder widget in the factory constructor to ensure Theme.of(context) has the correct context (lib/utils/toast.dart:62-68).


Behavioral Changes (By Design)

ℹ️ Toast Queuing: The migration from fluttertoast to SnackBar introduces expected behavioral changes:

  • Old behavior (fluttertoast): Used overlay-based toasts
  • New behavior (SnackBar): SnackBars follow Material Design guidelines where new ones dismiss previous ones

This is the correct Material Design pattern and is an improvement. Users who trigger multiple toasts rapidly will see the queue behavior, which prevents screen clutter.


Minor Observations (No Action Needed)

🟢 Animation Controller Initialization: The cascade notation (..forward()) on line 29 is a nice pattern for immediately starting the animation after controller creation.

🟢 Transparent Background: Using backgroundColor: Colors.transparent and elevation: 0 on the SnackBar allows the custom ToastWidget to fully control its appearance, which is the right approach.

🟢 Context Validity: The implementation properly uses ScaffoldMessenger.of(context) which survives context changes better than direct Scaffold.of(context) calls.


Testing Recommendations

Based on the PR checklist, the following testing should be verified:

  1. ✅ Semantic Naming: Variable names like AnimatedFade, ToastWidget, _controller, _opacity are all semantically named.

  2. ✅ Color Contrast: Text uses Theme.of(context).colorScheme.onSurface ensuring AA contrast compliance when paired with surfaceContainer background.

  3. Animation Testing: Verify the fade-in and fade-out animations are smooth across different devices (especially low-end devices).

  4. Theme Testing: Test with both light and dark themes to ensure the toast is visible and meets AA contrast standards in both modes.

  5. Multiple Toasts: Test showing multiple toasts in quick succession to verify the SnackBar queue behavior is acceptable for your UX requirements.

  6. Context Edge Cases: Test triggering a toast and immediately navigating away to ensure no errors occur.


Code Quality Assessment

Category Rating Notes
Architecture ⭐⭐⭐⭐⭐ Excellent separation of concerns with AnimatedFade, ToastWidget, and showToast
Resource Management ⭐⭐⭐⭐⭐ Perfect disposal of AnimationController
Theming ⭐⭐⭐⭐⭐ Proper use of Material Design color scheme
Animation ⭐⭐⭐⭐⭐ Smooth, well-timed animations with proper duration
API Design ⭐⭐⭐⭐⭐ Clean factory constructor pattern
Code Formatting ⭐⭐⭐⭐⭐ Consistent Dart formatting

Conclusion

This refactoring is exceptionally well-implemented. All critical issues from previous reviews have been addressed, and the code demonstrates:

  • Strong understanding of Flutter widget lifecycle
  • Proper resource management
  • Good Material Design practices
  • Clean, maintainable architecture

The animation timing fix in commit 88d384a shows attention to detail and responsiveness to feedback.

Recommendation: ✅ APPROVE

This code is production-ready and represents a quality improvement over the previous implementation. The removal of the external dependency while maintaining (and improving) functionality is a net positive for the codebase.


Changed Files Analysis:

  • ios/Podfile.lock - Correctly removes fluttertoast dependency from iOS
  • pubspec.yaml & pubspec.lock - Properly removes package reference
  • lib/utils/toast.dart - Excellent refactoring with proper lifecycle management and animations
  • lib/route/image_viewer/image_viewer.dart - Clean implementation with improved formatting

@lowrt
lowrt requested a review from kamiya4047 November 5, 2025 11:07
@lowrt
lowrt marked this pull request as draft November 8, 2025 09:12
@lowrt lowrt changed the title Refactor replace fluttertoast with SnackBar toast Fix SnackBar toast Nov 9, 2025
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Nov 9, 2025
@lowrt

lowrt commented Nov 9, 2025

Copy link
Copy Markdown
Contributor Author

@claude review

@lowrt
lowrt marked this pull request as ready for review November 9, 2025 08:41
@claude

claude Bot commented Nov 9, 2025

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@lowrt lowrt closed this Nov 9, 2025
@lowrt
lowrt deleted the refactor/toast branch November 9, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant