feat: new centralized chat banner controller#5919
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a centralized banner queue/controller for chat so “level up” and “unlocked morph” banners display in a controlled, sequential way (replacing the older snackbar-style implementation).
Changes:
- Added
ChatBannerControllerto queue/dedupe/time out chat banners. - Introduced reusable
ChatBannerBuilderplus newLevelUpBannerandUnlockedMorphBannerwidgets. - Removed legacy unlocked-morph snackbar util and legacy level-up banner util; wired chat page to new controller.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/pangea/common/utils/overlay.dart | Adds helper methods to show new top-positioned chat banners via overlays. |
| lib/pangea/chat/widgets/unlocked_morph_banner.dart | New banner widget for unlocked morph notifications (with details navigation + icon rain). |
| lib/pangea/chat/widgets/level_up_banner.dart | New banner widget for level-up notifications (includes audio playback). |
| lib/pangea/chat/widgets/chat_banner_builder.dart | New shared banner container/animation + auto-close + completer signaling. |
| lib/pangea/chat/utils/unlocked_morphs_snackbar.dart | Removes legacy snackbar-based unlocked morph notification implementation. |
| lib/pangea/chat/chat_banner_controller.dart | New centralized queue controller for banner sequencing and timeout behavior. |
| lib/pangea/analytics_misc/level_up/level_up_banner.dart | Removes legacy level-up util/banner implementation. |
| lib/pages/chat/chat.dart | Switches chat page to drive level-up/unlock notifications through ChatBannerController. |
You can also share your feedback on Copilot code review. Take the survey.
| _animation = CurvedAnimation(parent: _controller!, curve: Curves.easeInOut); | ||
| _controller!.forward().then((_) { | ||
| widget.onAnimatedIn?.call(); | ||
| _autoCloseTimer = Timer(const Duration(seconds: 10), () { | ||
| if (mounted) _close(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
_controller!.forward().then(...) can run its callback after this State has been disposed (e.g., if the overlay is closed quickly). That would still call onAnimatedIn and start _autoCloseTimer. Add a if (!mounted) return; inside the then callback (or use addStatusListener) before invoking callbacks / allocating timers.
| import 'package:fluffychat/pangea/bot/utils/bot_style.dart'; | ||
| import 'package:fluffychat/pangea/chat/widgets/level_up_banner.dart'; | ||
| import 'package:fluffychat/pangea/chat/widgets/unlocked_morph_banner.dart'; | ||
| import 'package:fluffychat/pangea/choreographer/choreo_constants.dart'; |
There was a problem hiding this comment.
overlay.dart now imports unlocked_morph_banner.dart, but UnlockedMorphBanner also imports overlay.dart, creating a circular dependency and coupling a common util (pangea/common/utils) to chat UI widgets. Consider moving showUnlockedConstructBanner/showLevelUpBanner into a chat-specific util/controller file (or invert dependencies by passing a widget builder into OverlayUtil.showOverlay) to break the import cycle.
| _playLevelUpSound(); | ||
| } | ||
|
|
||
| Future<void> _playLevelUpSound() async { | ||
| final player = AudioPlayer(); | ||
| player.setVolume(AppSettings.volume.value); | ||
|
|
||
| await player.play( | ||
| UrlSource( | ||
| "${AppConfig.assetsBaseURL}/${AnalyticsConstants.levelUpAudioFileName}", | ||
| ), | ||
| ); | ||
|
|
||
| await Future.delayed(const Duration(seconds: 2)); | ||
| player.dispose(); |
There was a problem hiding this comment.
_playLevelUpSound() is started from initState() without awaiting, and it doesn't handle errors or ensure the AudioPlayer is disposed in a finally. If player.play(...) throws (network/asset error), this becomes an unhandled async error and the player may leak. Consider wrapping in try/finally, storing the player in state and disposing it in dispose(), and/or using unawaited(...) with internal error handling.
| _playLevelUpSound(); | |
| } | |
| Future<void> _playLevelUpSound() async { | |
| final player = AudioPlayer(); | |
| player.setVolume(AppSettings.volume.value); | |
| await player.play( | |
| UrlSource( | |
| "${AppConfig.assetsBaseURL}/${AnalyticsConstants.levelUpAudioFileName}", | |
| ), | |
| ); | |
| await Future.delayed(const Duration(seconds: 2)); | |
| player.dispose(); | |
| unawaited(_playLevelUpSound()); | |
| } | |
| Future<void> _playLevelUpSound() async { | |
| final player = AudioPlayer(); | |
| try { | |
| player.setVolume(AppSettings.volume.value); | |
| await player.play( | |
| UrlSource( | |
| "${AppConfig.assetsBaseURL}/${AnalyticsConstants.levelUpAudioFileName}", | |
| ), | |
| ); | |
| await Future.delayed(const Duration(seconds: 2)); | |
| } catch (e, stackTrace) { | |
| debugPrint('Error while playing level up sound: $e'); | |
| } finally { | |
| await player.dispose(); | |
| } |
| void addBanner( | ||
| BannerExecutor showBanner, { | ||
| required String overlayKey, | ||
| Duration timeout = const Duration(seconds: 30), | ||
| }) { | ||
| if (_bannerQueue.any((b) => b.overlayKey == overlayKey)) return; | ||
|
|
There was a problem hiding this comment.
addBanner dedupes only against _bannerQueue, so the same overlayKey can be enqueued again while it is currently showing (since the active banner isn't part of the queue). Track the currently showing overlayKey (or maintain a Set of queued+active keys) to prevent duplicate banners from showing back-to-back.
| // ignore_for_file: depend_on_referenced_packages, implementation_imports | ||
|
|
There was a problem hiding this comment.
The file-level // ignore_for_file: depend_on_referenced_packages, implementation_imports suppression looks unnecessary here (imports are all public packages / app libs). Removing it helps keep lint suppressions meaningful and avoids masking real issues later.
| // ignore_for_file: depend_on_referenced_packages, implementation_imports |
| @override | ||
| void dispose() { | ||
| _autoCloseTimer?.cancel(); | ||
| widget.closeCompleter.complete(); |
There was a problem hiding this comment.
dispose() unconditionally calls widget.closeCompleter.complete(). If the banner times out in ChatBannerController (which already completes the completer) or is completed elsewhere, this will throw a StateError (Completer already completed) during widget disposal and can crash the app. Guard the call with if (!widget.closeCompleter.isCompleted) ... (and similarly consider guarding any other completion sites).
| widget.closeCompleter.complete(); | |
| if (!widget.closeCompleter.isCompleted) { | |
| widget.closeCompleter.complete(); | |
| } |
ggurdin
deleted the
5911-if-grammar-improvement-popup-stack-is-more-than-2-3-ignore-others
branch
Thank you so much for your contribution to FluffyChat ❤️❤️❤️
Pull Request has been tested on: