TW-2796: Implement autoplay for voice messages and enhance audio even#2797
TW-2796: Implement autoplay for voice messages and enhance audio even#2797
Conversation
nqhhdev
requested review from
dab246,
hoangdat and
tddang-linagora
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors audio playback handling across the chat application by introducing two new mixins (AudioMixin and EventFilterMixin) that centralize audio state management, event filtering, and lifecycle operations. The changes migrate audio player initialization and disposal logic from individual widgets to a mixin-based architecture, add platform-specific null-safety guards, and implement caching and auto-play capabilities. Chat pages now use AudioMixin for cross-platform audio management while event filtering logic is extracted into a reusable mixin for media and audio event handling. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai pls help me review |
|
@nqhhdev I'll review the changes in this PR for you. Let me analyze the implementation of autoplay for voice messages and the audio event enhancements. ✅ Actions performedReview triggered.
|
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2797 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
128-135: Remove the unusedhandleOggAudioFileIniOSmethod.This method is not called anywhere in the codebase. Similar audio file handling for iOS exists in
MatrixState.handleOggAudioFileIniOSandChatAudioPlayerState._handleOggAudioFileIniOS, which are actively used.
🧹 Nitpick comments (5)
lib/pages/chat/chat_audio_player_widget.dart (1)
250-260: Consider adding mounted check before showing SnackBar in play error handler.The
onErrorcallback at line 250 shows a SnackBar, but unlike the download error handler (line 210), there's nocontext.mountedcheck. If playback fails after the widget is disposed, this could cause an exception.🔎 Proposed fix
matrix?.audioPlayer?.play().onError((e, s) { Logs().e('Could not play audio file', e, s); + if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( e?.toLocalizedString(context) ?? L10n.of(context)!.couldNotPlayAudioFile, ), ), ); });lib/presentation/mixins/event_filter_mixin.dart (1)
520-534: Clarify the double-reversal logic in getAudioEventsUpToClicked.The method reverses
audioEventstwice (line 524 and 533), which could be confusing. If the input is in chronological order (oldest to newest), the comment states it returns events "from clicked to newest," but the double-reversal might produce unexpected results.Consider simplifying or adding a clarifying comment about the expected input/output ordering.
List<Event> getAudioEventsUpToClicked( List<Event> audioEvents, Event clickedEvent, ) { - final clickedIndex = audioEvents.reversed.toList().indexWhere( + // audioEvents is in chronological order (oldest first) + // We want to find clicked event and return it + all newer events + final reversedEvents = audioEvents.reversed.toList(); + final clickedIndex = reversedEvents.indexWhere( (event) => event.eventId == clickedEvent.eventId, ); if (clickedIndex == -1) { return []; } - // Return from clicked event onwards for sequential playback - return audioEvents.reversed.toList().sublist(clickedIndex); + // Return from clicked event onwards (newest to oldest for playback queue) + return reversedEvents.sublist(clickedIndex); }lib/widgets/matrix.dart (3)
1201-1201: Unnecessary null fallback for MatrixFile.bytes.Based on learnings,
MatrixFile.bytesis non-nullable (Uint8List) in matrix-dart-sdk v4.1.0+. The?? []fallback is defensive but unnecessary.- await file.writeAsBytes(matrixFile.bytes ?? []); + await file.writeAsBytes(matrixFile.bytes);
1237-1247: Add mounted check before showing SnackBar in play error handler.Unlike the download error handler (line 1212), the play error callback doesn't check if
contextis still mounted. This could cause an exception if playback fails after navigation.🔎 Proposed fix
audioPlayer?.play().onError((e, s) { Logs().e('Could not play audio file', e, s); + if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( e?.toLocalizedString(context) ?? L10n.of(context)!.couldNotPlayAudioFile, ), ), ); });
1250-1257: Duplicate helper method across files.
handleOggAudioFileIniOSis now duplicated inMatrixState(here),ChatAudioPlayerWidget, andAudioPlayerState. Consider consolidating this into a shared utility or theAudioMixinto avoid code duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/pages/chat/chat.dartlib/pages/chat/chat_audio_player_widget.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dartlib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-12T07:13:07.752Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2770
File: lib/pages/chat/chat.dart:623-670
Timestamp: 2025-12-12T07:13:07.752Z
Learning: In `lib/pages/chat/chat.dart`, when `_processReadMarker()` encounters a null or empty timeline, the design choice is to drop the pending read marker and wait for the next visibility event to trigger a fresh `setReadMarker()` call, rather than retrying. This is intentional behavior.
Applied to files:
lib/pages/chat/chat.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/pages/chat/chat.dartlib/widgets/matrix.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/pages/chat/chat_audio_player_widget.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/pages/chat/chat.dartlib/widgets/matrix.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dartlib/pages/chat/chat_audio_player_widget.dart
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/pages/chat/chat.dartlib/widgets/matrix.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dartlib/pages/chat/chat_audio_player_widget.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/widgets/matrix.dartlib/pages/chat/events/audio_message/audio_player_widget.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Widget test
🔇 Additional comments (13)
lib/pages/chat/chat.dart (2)
3135-3138: LGTM! Proper null-safety guards for audio player operations.The optional chaining (
?.) onaudioPlayeraligns with the nullableAudioPlayer?type introduced inMatrixState. This prevents null dereference when disposing audio resources.
3141-3157: Review the platform-specific conditional logic.The logic now skips resetting
voiceMessageEventandcurrentAudioStatuson mobile platforms. This appears intentional for the new queue-based autoplay feature, but verify this doesn't leave stale state when users navigate away from chat on mobile.Additionally, the nested conditionals at lines 3154-3156 are good defensive programming—avoiding unnecessary state updates when already in
notDownloadedstatus.lib/pages/chat/chat_audio_player_widget.dart (3)
34-37: LGTM! Null guard for matrix instance.Early return with an empty widget prevents null pointer issues when the
MatrixStateis unavailable.
56-61: LGTM! Null-safe stream access with appropriate fallbacks.Using optional chaining with fallback streams ensures the
StreamGroup.mergedoesn't fail whenaudioPlayeris null.
208-219: Improved error handling with user feedback.Good change from rethrowing to showing a SnackBar and resetting state. The mounted check at line 210 prevents accessing
contextafter widget disposal. The early return at line 218 ensures the method exits cleanly on error.lib/presentation/mixins/event_filter_mixin.dart (2)
1-25: Well-documented mixin with comprehensive event filtering utilities.The mixin provides a clean abstraction for event filtering, decryption, and pagination. The documentation with usage examples is helpful for maintainability.
121-143: LGTM! Robust decryption with graceful error handling.The decryption logic correctly checks for encryption support, handles each event independently, and logs failures without breaking the entire batch. This is resilient design for encrypted room scenarios.
lib/widgets/matrix.dart (2)
97-101: LGTM! Audio player now nullable with queue support.Making
audioPlayernullable and addingvoiceMessageEventssupports the new queue-based autoplay feature. This is a good architectural change for sequential voice message playback.
1141-1181: Queue-based audio playback with proper state management.The
setupAudioPlayerAutoDisposemethod now manages a queue of voice messages, removing completed events and automatically playing the next one. The cleanup logic when the queue is empty (lines 1168-1173) properly disposes resources.lib/pages/chat/events/audio_message/audio_player_widget.dart (4)
50-51: LGTM! EventFilterMixin integration.Adding
EventFilterMixinenables the widget to use the centralized event filtering and audio initialization utilities.
91-114: LGTM! Null-safe audio player operations.The optional chaining on
audioPlayerproperties (isAtEndPosition,playing) and methods (stop,dispose,pause,play) aligns with the nullableAudioPlayer?type and prevents null dereference.
117-125: Clean delegation to EventFilterMixin and MatrixState.The audio initialization now uses
initAudioEventsUpToClickedfrom the mixin to fetch pending audio events, then delegates playback tomatrix.autoPlayAudio. This is a good separation of concerns.
180-198: Improved disposal with async cleanup and post-frame callback.The disposal now:
- Only cleans up if this widget's event is currently playing (line 183)
- Uses async cleanup with error handling (lines 186-190)
- Defers state updates to post-frame callback to avoid widget tree lock issues (lines 194-197)
This is a robust pattern for avoiding common Flutter disposal issues.
nqhhdev
changed the title
nqhhdev
force-pushed
the
TW-2796-Implement-autoplay-for-voice-messages
branch
from
7af62cc to
40b2237
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/pages/chat/chat_audio_player_widget.dart (1)
250-260: Missing mounted check in play error handler.The
onErrorcallback forplay()accessescontextto show a SnackBar, but there's nocontext.mountedcheck. If the widget is disposed before the error occurs, this could throw aFlutterError.🔎 Proposed fix
matrix?.audioPlayer?.play().onError((e, s) { Logs().e('Could not play audio file', e, s); + if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( e?.toLocalizedString(context) ?? L10n.of(context)!.couldNotPlayAudioFile, ), ), ); });lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
180-202: Fix resource disposal race condition in dispose method.The
dispose()method schedules asyncstop()anddispose()operations without awaiting them and without capturing the current audio player reference. If a new audio message is selected before the async operations complete,matrix.audioPlayerwill be reassigned to a new player instance. The old widget's.then()callback will then inadvertently dispose the new player instead of the old one, causing resource conflicts or premature disposal of the active player.Store a reference to the current player before scheduling async operations, or use
await(as done inmatrix.dartline 1169) to ensure completion before the widget fully disposes:if (matrix.voiceMessageEvent.value?.eventId == widget.event.eventId) { final playerToDispose = matrix.audioPlayer; if (playerToDispose != null) { await playerToDispose.stop(); await playerToDispose.dispose(); } WidgetsBinding.instance.addPostFrameCallback((_) { if (matrix.audioPlayer == playerToDispose) { matrix.currentAudioStatus.value = AudioPlayerStatus.notDownloaded; matrix.voiceMessageEvent.value = null; } }); }
🤖 Fix all issues with AI Agents
In @lib/presentation/mixins/event_filter_mixin.dart:
- Around line 533-548: The getAudioEventsUpToClicked function currently reverses
audioEvents (reversedEvents) and returns reversedEvents.sublist(clickedIndex),
producing a backwards list; instead operate on the original audioEvents: find
clickedIndex by searching audioEvents for event.eventId == clickedEvent.eventId
and return audioEvents.sublist(clickedIndex) so the returned sequence is forward
chronological (e.g., [c, d, e]); remove the reversedEvents usage and adjust
variable references accordingly.
In @lib/widgets/matrix.dart:
- Around line 1237-1247: The onError callback for audioPlayer?.play() uses
ScaffoldMessenger.of(context).showSnackBar without verifying the State is still
mounted; update the closure in the play error handler to check the State's
mounted flag before touching context (e.g., if (!mounted) return;) so you only
call ScaffoldMessenger.of(context).showSnackBar when mounted, preserving the
existing Logs().e and the error message construction
(e?.toLocalizedString(context) ?? L10n.of(context)!.couldNotPlayAudioFile)
inside that guarded block.
- Around line 1196-1201: Avoid the force-unwrap and the redundant
null-coalescing: check that currentEvent.attachmentOrThumbnailMxcUrl() is
non-null before using it to build fileName (use a fallback name like
matrixFile.name or a timestamp if null) and remove the unnecessary "?? []" on
matrixFile.bytes since bytes is non-nullable; update the code around fileName
and the File(...) + writeAsBytes(...) call to use the safe fileName and call
writeAsBytes(matrixFile.bytes) directly.
🧹 Nitpick comments (3)
lib/pages/chat/chat_audio_player_widget.dart (2)
34-42: Consider extracting defaults to avoid recreation on every build.The
defaultAudioStatusanddefaultEventValueNotifiers are created on everybuildcall, even whenmatrixis not null. Consider moving these to class-level static constants or restructuring to avoid unnecessary object creation.🔎 Suggested refactor
+class ChatAudioPlayerWidget extends StatelessWidget { + static final _defaultAudioStatus = ValueNotifier<AudioPlayerStatus>( + AudioPlayerStatus.notDownloaded, + ); + static final _defaultEvent = ValueNotifier<Event?>(null); + final MatrixState? matrix; // ... @override Widget build(BuildContext context) { if (matrix == null) { return const SizedBox.shrink(); } - final defaultAudioStatus = ValueNotifier<AudioPlayerStatus>( - AudioPlayerStatus.notDownloaded, - ); - final defaultEvent = ValueNotifier<Event?>(null); return ValueListenableBuilder( - valueListenable: matrix?.currentAudioStatus ?? defaultAudioStatus, + valueListenable: matrix?.currentAudioStatus ?? _defaultAudioStatus,
194-199: Unnecessary null check onmatrixFile.bytes.Based on learnings,
MatrixFile.bytesis non-nullable (Uint8List) in matrix-dart-sdk v4.1.0+. ThedownloadAndDecryptAttachment()method guarantees non-null bytes. The null/empty check onbytesat line 196 is defensive but inconsistent with the SDK contract.🔎 Suggested refactor
- final bytes = matrixFile?.bytes; - - if (bytes == null || bytes.isEmpty) { - throw Exception('Downloaded file has no content'); - } - await file.writeAsBytes(bytes); + await file.writeAsBytes(matrixFile!.bytes);lib/presentation/mixins/event_filter_mixin.dart (1)
369-383: Minor optimization: avoid repeated list reversals.The
reversed.toList()is called on bothbackwardEventsandforwardEventsfor filtering. Consider reversing once and reusing.🔎 Suggested refactor
+ final backwardEventsReversed = backwardEvents.reversed.toList(); + final forwardEventsReversed = forwardEvents.reversed.toList(); + initialFilteredEvents = [ ...(messageTypes != null ? filterEventsByTypes( - backwardEvents.reversed.toList(), + backwardEventsReversed, messageTypes, ) - : filterMediaEvents(backwardEvents.reversed.toList())), + : filterMediaEvents(backwardEventsReversed)), ...initialFilteredEvents, ...(messageTypes != null ? filterEventsByTypes( - forwardEvents.reversed.toList(), + forwardEventsReversed, messageTypes, ) - : filterMediaEvents(forwardEvents.reversed.toList())), + : filterMediaEvents(forwardEventsReversed)), ];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/pages/chat/chat.dartlib/pages/chat/chat_audio_player_widget.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dartlib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/presentation/mixins/event_filter_mixin.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/pages/chat/chat_audio_player_widget.dartlib/pages/chat/chat.dartlib/widgets/matrix.dart
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/presentation/mixins/event_filter_mixin.dartlib/pages/chat/events/audio_message/audio_player_widget.dartlib/pages/chat/chat_audio_player_widget.dartlib/pages/chat/chat.dartlib/widgets/matrix.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/pages/chat/events/audio_message/audio_player_widget.dartlib/pages/chat/chat_audio_player_widget.dartlib/widgets/matrix.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/pages/chat/events/audio_message/audio_player_widget.dartlib/widgets/matrix.dart
📚 Learning: 2025-12-12T07:13:07.752Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2770
File: lib/pages/chat/chat.dart:623-670
Timestamp: 2025-12-12T07:13:07.752Z
Learning: In `lib/pages/chat/chat.dart`, when `_processReadMarker()` encounters a null or empty timeline, the design choice is to drop the pending read marker and wait for the next visibility event to trigger a fresh `setReadMarker()` call, rather than retrying. This is intentional behavior.
Applied to files:
lib/pages/chat/chat.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (5)
lib/pages/chat/chat.dart (1)
3128-3157: Approve audio player lifecycle management.The null-safety guards and platform-specific behavior are well implemented:
- Mobile correctly preserves playing audio across navigation
- Web correctly resets state when entering a new chat
- Optional chaining prevents NPE on nullable
audioPlayerThe logic at lines 3148-3156 could be slightly more concise, but the explicit null checks are clear and safe.
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
117-125: Approve the refactored audio initialization flow.The new approach cleanly delegates event fetching and queue management to
EventFilterMixinandMatrixState:
initAudioEventsUpToClickedfetches the audio playlistvoiceMessageEventsstores the queueautoPlayAudiohandles playback orchestrationThis separation of concerns improves maintainability.
lib/presentation/mixins/event_filter_mixin.dart (1)
247-261: Token matching issue has been addressed.The positional indexing approach correctly handles
Future.waitresults:
- When both tokens are provided:
result[0]= backward,result[1]= forward- When only one token is provided:
result[0]is that single responseThis resolves the previously identified issue with incorrect token matching.
lib/widgets/matrix.dart (2)
97-101: Approve nullable audioPlayer and queue-based design.Making
audioPlayernullable and addingvoiceMessageEventsas a queue enables proper lifecycle management:
- Audio player is only instantiated when needed
- Queue supports autoplay of consecutive voice messages
- Disposal is safe with null checks
1144-1178: Approve queue-based autoplay progression.The
setupAudioPlayerAutoDisposemethod correctly:
- Removes the completed message from the queue
- Cleans up when the queue is empty
- Triggers autoplay for the next message
The state management is consistent with the new nullable
audioPlayerpattern.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat/chat_audio_player_widget.dart (1)
196-198: Remove redundant null check on non-nullablebytes.
MatrixFile.bytesis a non-nullableUint8Listper the Matrix SDK API, making thebytes == nullcheck always false. TheisEmptycheck remains valid.🔎 Proposed fix
- if (bytes == null || bytes.isEmpty) { + if (bytes.isEmpty) { throw Exception('Downloaded file has no content'); }Based on learnings, MatrixFile.bytes is non-nullable in matrix-dart-sdk v4.1.0+.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/pages/chat/chat_audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dart
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/pages/chat/chat_audio_player_widget.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/pages/chat/chat_audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dart
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/pages/chat/chat_audio_player_widget.dartlib/presentation/mixins/event_filter_mixin.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Widget test
- GitHub Check: Analyze code
🔇 Additional comments (7)
lib/pages/chat/chat_audio_player_widget.dart (3)
32-35: LGTM: Static defaults prevent unnecessary rebuilds.The static default notifiers provide safe fallback values when
matrixis null, avoiding widget rebuilds and null-reference issues inValueListenableBuilderwidgets.
39-42: LGTM: Appropriate null guard.Returning an empty widget when
matrixis unavailable prevents null-reference errors and is the correct defensive pattern for optional dependencies.
209-219: LGTM: Proper async error handling with mounted guards.The error handling correctly logs the exception, guards against unmounted context before showing UI feedback, and recovers gracefully by resetting state.
lib/presentation/mixins/event_filter_mixin.dart (4)
247-261: LGTM: Correct positional indexing for pagination responses.The implementation correctly uses positional indexing to match responses, as
Future.waitpreserves input order. The logic properly handles all token combinations with appropriate bounds checking.
30-110: LGTM: Filter methods are clear and correctly implemented.The event filtering methods follow a consistent pattern and provide comprehensive coverage of Matrix message types. The implementations are straightforward and appropriate.
121-143: LGTM: Robust decryption with proper error handling.The method correctly handles encrypted events with parallel decryption via
Future.wait, gracefully falls back to original events on failure, and logs errors appropriately.
270-398: LGTM: Comprehensive media initialization with smart expansion.The method correctly orchestrates initial load, decryption, filtering, and automatic expansion to reach the target event count. The reversal of paginated chunks (lines 369-370) properly handles the Matrix API's ordering conventions, ensuring final events are in chronological order.
@coderabbitai about the
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1012 @nqhhdev You're absolutely correct, and I'm pleased to see the code already implements the proper checks! At line 194-198 in final bytes = matrixFile?.bytes;
if (bytes == null || bytes.isEmpty) {
throw Exception('Downloaded file has no content');
}This implementation is correct because:
This defensive approach ensures robustness when dealing with downloaded audio files. The current implementation properly guards against both scenarios before attempting to write the bytes to a file. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @lib/widgets/matrix.dart:
- Around line 1408-1411: The dispose method is missing a call to dispose the
ValueNotifier voiceMessageEvents; add voiceMessageEvents.dispose() alongside
voiceMessageEvent.dispose() (and before super.dispose()) to ensure both
notifiers are cleaned up and avoid leaking listeners, keeping the existing
audioPlayer cancellation/disposal sequence intact.
- Around line 1241-1245: The code calls audioPlayer?.setFilePath(file.path)
without awaiting it, which can cause a race when play() is invoked; update the
if branch to await the Future returned by AudioPlayer.setFilePath (i.e., use
await audioPlayer?.setFilePath(file.path)) so the file is fully loaded before
playback, mirroring the existing await used for
audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile)).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2026-01-09T07:54:23.212Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2818
File: lib/utils/matrix_sdk_extensions/event_extension.dart:427-438
Timestamp: 2026-01-09T07:54:23.212Z
Learning: In lib/utils/matrix_sdk_extensions/event_extension.dart, the method isMediaAndFilesWithCaption() intentionally returns true for ALL reply events (not just media replies), using the logic: (media/file with caption) OR (any reply event). This is the desired behavior for the reply-with-media feature in PR #2818, allowing caption input for all replies.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/widgets/matrix.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (10)
lib/widgets/matrix.dart (10)
13-22: LGTM!The new imports for
audio_play_extension.dart,opus_caf_converter_dart.dart, andpath_provider.dartare appropriate for the audio autoplay and iOS OGG conversion features.
97-101: LGTM!Making
audioPlayernullable aligns with on-demand initialization, andvoiceMessageEventsproperly supports the queue-based autoplay feature.
170-171: LGTM!Calling
cleanupAudioPlayer()when switching accounts ensures audio playback is properly stopped before transitioning to a different user session.
1147-1187: LGTM!The auto-dispose logic correctly handles:
- Canceling previous subscriptions before setting up new ones
- Filtering out completed events from the queue
- Disposing the player when the queue is empty
- Automatically playing the next queued message
The sequence of operations (filter before nulling
voiceMessageEvent.value) ensures the completed event is properly identified and removed.
1223-1234: LGTM!Proper
context.mountedchecks before accessingScaffoldMessengerin async callbacks. This prevents "looking up a deactivated widget's ancestor" errors.
1264-1276: LGTM!The iOS OGG-to-CAF conversion handles errors gracefully by returning
nulland letting the caller fall back to the original file. Caching converted files by checking existence is a good optimization.
1366-1385: LGTM!The cleanup method properly resets all audio state, including pausing, stopping, disposing the player, clearing the queue, and resetting the status notifier. The try-catch ensures cleanup failures don't crash the app.
1111-1112: LGTM!Cleanup during logout ensures audio playback is properly stopped when the user logs out.
526-527: LGTM!Consistent cleanup call when handling logout with multiple accounts.
1198-1211: No changes needed. The code at line 1211 correctly usesmatrixFile.bytes ?? []becauseMatrixFile.bytesis nullable in this repository's forked matrix-dart-sdk (twake-supported-0.22.6). This is consistent with defensive null handling patterns used throughout the codebase (e.g.,story_page.dart:212,image_viewer_view.dart:139,mxc_image.dart:207).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/widgets/matrix.dart (2)
1237-1248: Potential resource leak ifautoPlayAudiois called while a player is active.Line 1238 creates a new
AudioPlayer()instance without checking ifaudioPlayeris already non-null and active. IfautoPlayAudiois called externally while audio is playing (not through the queue mechanism), the existing player reference would be overwritten without disposal, leaking resources.Consider disposing the existing player before creating a new one:
♻️ Proposed fix
// Initialize audio player before use + await audioPlayer?.stop(); + await audioPlayer?.dispose(); audioPlayer = AudioPlayer(); voiceMessageEvent.value = currentEvent;
1209-1211: Consider validating downloaded bytes before writing to file.If
matrixFile.bytesis empty (which could happen in edge cases), writing an empty file would lead to playback failures. Based on learnings,MatrixFile.bytesshould be non-nullable in matrix-dart-sdk v4.1.0+, making?? []potentially masking an unexpected state.Consider adding validation similar to other parts of the codebase:
♻️ Proposed fix
file = File('${tempDir.path}/${fileName}_${matrixFile.name}'); + if (matrixFile.bytes.isEmpty) { + throw Exception('Downloaded file has no content'); + } await file.writeAsBytes(matrixFile.bytes ?? []);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2026-01-09T07:54:23.212Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2818
File: lib/utils/matrix_sdk_extensions/event_extension.dart:427-438
Timestamp: 2026-01-09T07:54:23.212Z
Learning: In lib/utils/matrix_sdk_extensions/event_extension.dart, the method isMediaAndFilesWithCaption() intentionally returns true for ALL reply events (not just media replies), using the logic: (media/file with caption) OR (any reply event). This is the desired behavior for the reply-with-media feature in PR #2818, allowing caption input for all replies.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/widgets/matrix.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (9)
lib/widgets/matrix.dart (9)
13-22: LGTM!The new imports appropriately support the audio playback enhancements:
audio_play_extension.dartfor audio extensions,opus_caf_converter_dartfor iOS OGG-to-CAF conversion, andpath_providerfor temporary file storage.
97-101: LGTM!Making
audioPlayernullable enables proper lazy initialization and lifecycle management. ThevoiceMessageEventsqueue supports the autoplay feature by tracking pending voice messages.
170-171: LGTM!Properly cleaning up audio resources during account switching prevents orphaned playback and ensures a clean state for the new account context.
526-527: LGTM!Consistent cleanup during logout ensures audio resources are released when user signs out.
1111-1112: LGTM!Cleanup on last logout completes the audio resource management across all logout scenarios.
1147-1187: LGTM!The refactored
setupAudioPlayerAutoDisposeproperly implements queue-based autoplay:
- Removes completed messages from the queue
- Disposes and nullifies the player when the queue is empty
- Auto-advances to the next message when available
The optional chaining on the nullable
audioPlayerensures null-safety throughout the listener.
1366-1385: LGTM!Comprehensive cleanup method that properly:
- Stops and disposes the audio player
- Cancels the state subscription
- Resets all voice message state
- Handles errors gracefully with try-catch
The sequential
pause()→stop()→dispose()pattern is defensive and ensures clean shutdown regardless of player state.
1408-1411: LGTM!The dispose method properly cleans up audio resources with null-safe optional chaining on
audioPlayerand disposes bothValueNotifierinstances (voiceMessageEventsandvoiceMessageEvent).
1264-1276: LGTM!The iOS OGG-to-CAF conversion includes sensible caching by checking file existence before converting. Returning
nullon error allows graceful fallback to the original file. The synchronousOpusCaf().convertOpusToCaf()call is acceptable for occasional audio message conversions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/widgets/matrix.dart:
- Around line 1271-1283: handleOggAudioFileIniOS currently returns the target
File immediately after calling OpusCaf().convertOpusToCaf(...) which can fail
silently; update the method to await the conversion call (if it returns a
Future) and then verify convertedFile.exists() after conversion, returning the
File only if the file actually exists and returning null otherwise; apply the
same post-conversion existence check to the other two similar methods in
chat_audio_player_widget.dart and audio_player_widget.dart and log a
warning/error when the converted file is missing.
🧹 Nitpick comments (2)
lib/widgets/matrix.dart (2)
1174-1178: Missing subscription cancellation when disposing player in auto-dispose handler.When the queue is empty and the player is disposed, the
_audioPlayerStateSubscriptionshould also be cancelled to prevent potential issues with the orphaned subscription.♻️ Suggested fix
// Check if there are more messages to play if (voiceMessageEvents.value.isEmpty) { + _audioPlayerStateSubscription?.cancel(); + _audioPlayerStateSubscription = null; await audioPlayer?.stop(); await audioPlayer?.dispose(); audioPlayer = null; return; }
1211-1215: Inconsistent null-safety handling formatrixFile.bytes.Line 1211 checks
matrixFile.bytes?.isEmpty == truebut line 1215 usesmatrixFile.bytes ?? []. Ifbyteswere null (which per the SDK v4.1.0+ it shouldn't be), the empty check would pass (evaluates tonull == true→false) and then an empty file would be written.Per the retrieved learnings,
MatrixFile.bytesis non-nullable in matrix-dart-sdk v4.1.0+, so the defensive null checks can be simplified:♻️ Suggested simplification
- if (matrixFile.bytes?.isEmpty == true) { + if (matrixFile.bytes.isEmpty) { throw Exception('Downloaded file has no content'); } - await file.writeAsBytes(matrixFile.bytes ?? []); + await file.writeAsBytes(matrixFile.bytes);Based on learnings, MatrixFile.bytes is non-nullable in matrix-dart-sdk v4.1.0+.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2818
File: lib/utils/matrix_sdk_extensions/event_extension.dart:427-438
Timestamp: 2026-01-09T07:54:23.212Z
Learning: In lib/utils/matrix_sdk_extensions/event_extension.dart, the method isMediaAndFilesWithCaption() intentionally returns true for ALL reply events (not just media replies), using the logic: (media/file with caption) OR (any reply event). This is the desired behavior for the reply-with-media feature in PR #2818, allowing caption input for all replies.
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2026-01-09T07:54:23.212Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2818
File: lib/utils/matrix_sdk_extensions/event_extension.dart:427-438
Timestamp: 2026-01-09T07:54:23.212Z
Learning: In lib/utils/matrix_sdk_extensions/event_extension.dart, the method isMediaAndFilesWithCaption() intentionally returns true for ALL reply events (not just media replies), using the logic: (media/file with caption) OR (any reply event). This is the desired behavior for the reply-with-media feature in PR #2818, allowing caption input for all replies.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/widgets/matrix.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (7)
lib/widgets/matrix.dart (7)
97-101: LGTM!The nullable
audioPlayerallows proper lazy initialization and cleanup. ThevoiceMessageEventsValueNotifier provides the queue needed for autoplay functionality.
170-171: LGTM!Properly awaiting audio cleanup when switching accounts prevents audio from continuing to play under the wrong context.
526-527: LGTM!Audio cleanup is properly added to the multi-account logout flow.
1111-1112: LGTM!Audio cleanup is properly added to the last logout flow.
1373-1392: LGTM!Comprehensive cleanup method that properly handles all audio-related state. The try-catch ensures cleanup errors don't propagate, and the sequential pause→stop→dispose pattern ensures graceful shutdown.
1416-1418: LGTM!The nullable
audioPlayeris properly disposed with null-safe access, and the newvoiceMessageEventsValueNotifier is correctly disposed alongside other ValueNotifiers.
1257-1268: Missingcontext.mountedcheck inonErrorcallback.The
onErrorcallback accessescontextto show a snackbar, but unlike the catch block (line 1229), it doesn't guard against an unmounted context before accessingScaffoldMessenger.of(context).🐛 Proposed fix
audioPlayer?.play().onError((e, s) { Logs().e('Could not play audio file', e, s); if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( e?.toLocalizedString(context) ?? L10n.of(context)!.couldNotPlayAudioFile, ), ), ); });Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/widgets/matrix.dart:
- Around line 1259-1270: When audioPlayer?.play() fails the code logs an error
and shows a SnackBar but does not reset audio playback state, which can leave
the queue stuck; update the play error handler (the onError callback for
audioPlayer?.play()) to reset or clear voiceMessageEvent and set
currentAudioStatus to an idle/none state and then trigger the same
queue-advancement logic used by setupAudioPlayerAutoDispose (so it behaves like
ProcessingState.completed), ensuring any UI/state listeners are updated and the
next item can start.
🧹 Nitpick comments (2)
lib/widgets/matrix.dart (2)
1194-1248: RedundantvoiceMessageEventassignment.
voiceMessageEvent.valueis set tocurrentEventat line 1194, then identically again at line 1248. The second assignment is unnecessary.Suggested fix
- audioPlayer = AudioPlayer(); - voiceMessageEvent.value = currentEvent; + audioPlayer = AudioPlayer();
1273-1292: Consider movingOpusCaf().convertOpusToCaf()to a background isolate.The
convertOpusToCaf()call is synchronous and blocks the UI thread. While this is acceptable for typical voice messages (which are short), larger audio files could cause noticeable jank on iOS. Consider usingcompute()or an isolate to run the conversion off the main thread if performance issues arise.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/widgets/matrix.dart
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2026-01-09T07:54:23.212Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2818
File: lib/utils/matrix_sdk_extensions/event_extension.dart:427-438
Timestamp: 2026-01-09T07:54:23.212Z
Learning: In lib/utils/matrix_sdk_extensions/event_extension.dart, the method isMediaAndFilesWithCaption() intentionally returns true for ALL reply events (not just media replies), using the logic: (media/file with caption) OR (any reply event). This is the desired behavior for the reply-with-media feature in PR #2818, allowing caption input for all replies.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T05:00:02.734Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T05:00:02.734Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-10T03:56:52.820Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2767
File: lib/pages/chat/chat_audio_player_widget.dart:264-301
Timestamp: 2025-12-10T03:56:52.820Z
Learning: In the audio player widget (lib/pages/chat/chat_audio_player_widget.dart), the supported playback speeds are 0.5x, 1.0x, 1.5x, and 2.0x. The 0.75x speed is not supported.
Applied to files:
lib/widgets/matrix.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/widgets/matrix.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy preview versions on pull requests
- GitHub Check: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (5)
lib/widgets/matrix.dart (5)
97-101: LGTM - Nullable audio player and queue state.Making
audioPlayernullable enables explicit lifecycle management, andvoiceMessageEventsproperly supports the queue-based autoplay feature.
170-171: LGTM - Proper audio cleanup on account switch.Cleaning up the audio player before switching accounts prevents orphaned playback state.
1147-1189: LGTM - Queue-based playback management.The logic correctly manages the voice message queue by removing completed items and advancing to the next, with proper cleanup when the queue empties.
1382-1401: LGTM - Comprehensive audio cleanup.The method properly resets all audio-related state and handles potential errors during cleanup.
1425-1427: LGTM - Proper resource disposal.The nullable audio player and new
voiceMessageEventsValueNotifier are properly disposed.
| /// - [clickedEvent]: The event that was clicked | ||
| /// | ||
| /// Returns a list of audio events from the clicked position onwards. | ||
| List<Event> getAudioEventsUpToClicked( |
| Room room, | ||
| ) { | ||
| return [ | ||
| ...(eventContext.eventsAfter?.reversed.toList() ?? []), |
There was a problem hiding this comment.
maybe you duplicated revert events list. Here and also in getAudioEventsUpToClicked
lib/widgets/matrix.dart
Outdated
| }); | ||
| } | ||
|
|
||
| Future<File?> handleOggAudioFileIniOS(File file) async { |
There was a problem hiding this comment.
this function be duplicated in chat_audio_player_widget & audio_player_widget & here
lib/widgets/matrix.dart
Outdated
|
|
||
| if (Platform.isIOS && | ||
| matrixFile.mimeType.toLowerCase() == 'audio/ogg') { | ||
| final oggAudioFileIniOS = await handleOggAudioFileIniOS(file); |
There was a problem hiding this comment.
how about throwing or catch oggAudioFileIniOS == null to show error to user?
| /// | ||
| /// Returns a new list containing only events with messageType of | ||
| /// [MessageTypes.Image]. | ||
| List<Event> filterImageEvents(List<Event> events) { |
| /// | ||
| /// Returns a new list containing only events with messageType of | ||
| /// [MessageTypes.Video]. | ||
| List<Event> filterVideoEvents(List<Event> events) { |
| /// | ||
| /// Returns a new list containing only events with messageType of | ||
| /// [MessageTypes.Audio]. | ||
| List<Event> filterAudioEvents(List<Event> events) { |
lib/widgets/matrix.dart
Outdated
| _audioPlayerStateSubscription?.cancel(); | ||
| _audioPlayerStateSubscription = null; | ||
| await audioPlayer?.stop(); | ||
| await audioPlayer?.dispose(); | ||
| audioPlayer = null; |
There was a problem hiding this comment.
It happens only in ProcessingState.completed, how about in other state?
lib/widgets/matrix.dart
Outdated
| final fileName = Uri.encodeComponent( | ||
| mxcUrl.pathSegments.last, | ||
| ); | ||
| file = File('${tempDir.path}/${fileName}_${matrixFile.name}'); |
There was a problem hiding this comment.
when you delete this temp file?
lib/widgets/matrix.dart
Outdated
| final fileName = Uri.encodeComponent( | ||
| mxcUrl.pathSegments.last, | ||
| ); | ||
| file = File('${tempDir.path}/${fileName}_${matrixFile.name}'); |
There was a problem hiding this comment.
how about also using cache memory -> cache file -> downloading?
… fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/presentation/mixins/audio_mixin.dart (1)
246-259: Stream listeners accumulate and cause StateError on multi-chunk reads.On each iteration of the
whileloop, new listeners are added toreader.onLoadandreader.onErrorwithout cancelling previous ones. When the FileReader firesonLoadfor chunk N, all N accumulated listeners execute—causing earlier listeners to callcomplete()on already-completed Completers, which throwsStateError.Compare with
createMatrixAudioFileFromWebFile(lines 332–345) which correctly cancels listeners after use.🐛 Proposed fix
while (offset < file.size) { final end = (offset + chunkSize) > file.size ? file.size : offset + chunkSize; final blob = file.slice(offset, end); final completer = Completer<Uint8List>(); - reader.onLoad.listen((_) { + final loadListener = reader.onLoad.listen((_) { final result = reader.result; if (result is Uint8List) { completer.complete(result); } else if (result is ByteBuffer) { completer.complete(result.asUint8List()); } else { completer.completeError('Unexpected result type'); } }); - reader.onError.listen((event) { + final errorListener = reader.onError.listen((event) { completer.completeError('FileReader failed: ${reader.error}'); }); reader.readAsArrayBuffer(blob); - yield await completer.future.timeout( - const Duration(seconds: 30), - onTimeout: () => throw TimeoutException('Chunk reading timed out'), - ); + try { + yield await completer.future.timeout( + const Duration(seconds: 30), + onTimeout: () => throw TimeoutException('Chunk reading timed out'), + ); + } finally { + await loadListener.cancel(); + await errorListener.cancel(); + } offset = end; }
… fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/presentation/mixins/audio_mixin.dart`:
- Around line 568-636: The autoPlayAudio flow can update currentAudioStatus and
caches after a newer call has changed voiceMessageEvent; to fix, after any async
download/IO work (just before setting currentAudioStatus.value =
AudioPlayerStatus.downloaded and before writing to disk/memory or doing iOS
conversion) check if voiceMessageEvent.value?.eventId == currentEvent.eventId
and if not, abort/return early so the stale task does not overwrite state or
caches; update references in autoPlayAudio (use voiceMessageEvent,
currentAudioStatus, _audioMemoryCache, and file/matrixFile logic) to perform
this guard right after the try block completes and also before any final file
writes or status mutations.
… fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
… fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
171-194: Accessingwidgetafterdispose()is unsafe.The
addPostFrameCallbackaccesseswidget.event.eventIdafterdispose()has returned. At that point, the widget is unmounted and accessingwidgetis undefined behavior in Flutter. Capture the event ID before scheduling the callback.🐛 Proposed fix
`@override` void dispose() { if (!PlatformInfos.isMobile) { + final currentEventId = widget.event.eventId; WidgetsBinding.instance.addPostFrameCallback((_) async { // Only dispose if this event is currently playing - if (matrix.voiceMessageEvent.value?.eventId == widget.event.eventId) { + if (matrix.voiceMessageEvent.value?.eventId == currentEventId) { // Stop and dispose audio player asynchronously to avoid blocking // dispose final playerToDispose = matrix.audioPlayer; if (playerToDispose != null) { await playerToDispose.stop(); await playerToDispose.dispose(); } // Schedule value updates for after the current frame to avoid // setState() during widget tree lock matrix.currentAudioStatus.value = AudioPlayerStatus.notDownloaded; matrix.voiceMessageEvent.value = null; } }); } super.dispose(); }
🧹 Nitpick comments (1)
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
117-126: Consider awaiting or explicitly ignoringautoPlayAudio.
matrix.autoPlayAudiois called withoutawaitwhile it's called withawaitelsewhere in the codebase (e.g., inaudio_mixin.dart). Though the method has internal error handling via try-catch, the inconsistency suggests this fire-and-forget pattern may be unintentional. If fire-and-forget is desired, useunawaited()fromdart:asyncto make the intent explicit and suppress lint warnings. Otherwise, consider awaiting it for consistency.♻️ Suggested change if fire-and-forget is intentional
matrix.voiceMessageEvents.value = audioPending.events; - matrix.autoPlayAudio(currentEvent: widget.event, context: context); + unawaited(matrix.autoPlayAudio(currentEvent: widget.event, context: context)); }
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
89-125: Add error handling forinitAudioEventsUpToClickedto prevent inconsistent state.
TheinitAudioEventsUpToClickedcall lacks error handling and can leave the UI in an inconsistent state if it fails. WhileautoPlayAudiohas built-in error handling, wrapping the initialization block ensures proper state reset and error feedback if the async operation fails.🔧 Suggested guard
- final audioPending = await initAudioEventsUpToClicked( - client: matrix.client, - room: widget.event.room, - clickedEvent: widget.event, - ); - - matrix.voiceMessageEvents.value = audioPending.events; - - await matrix.autoPlayAudio(currentEvent: widget.event, context: context); + try { + final audioPending = await initAudioEventsUpToClicked( + client: matrix.client, + room: widget.event.room, + clickedEvent: widget.event, + ); + + matrix.voiceMessageEvents.value = audioPending.events; + + await matrix.autoPlayAudio(currentEvent: widget.event, context: context); + } catch (e, s) { + Logs().e('Could not initialize audio playback', e, s); + matrix.voiceMessageEvent.value = null; + matrix.currentAudioStatus.value = AudioPlayerStatus.notDownloaded; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text( + e.toLocalizedString(context) ?? + L10n.of(context)!.couldNotPlayAudioFile, + ), + ), + ); + }
🤖 Fix all issues with AI agents
In `@lib/pages/chat/events/audio_message/audio_player_widget.dart`:
- Around line 227-232: The wavePosition calculation passes
_calculatedWaveform.length as maxWaves which can be zero; guard this by using
AudioPlayerStyle.maxWaveCount(context) when _calculatedWaveform.isEmpty so
calculateWaveCountAuto never receives 0. Update the call that computes
wavePosition (the expression using calculateWaveCountAuto with minWaves and
maxWaves) to compute maxWaves = _calculatedWaveform.isNotEmpty ?
_calculatedWaveform.length : AudioPlayerStyle.maxWaveCount(context) and pass
that value into calculateWaveCountAuto; keep minWaves as
AudioPlayerStyle.minWaveCount and leave the rest of the calculation unchanged.
🧹 Nitpick comments (1)
lib/pages/chat/events/audio_message/audio_player_widget.dart (1)
133-163: Avoid resetting global audio status from every widget init.
matrix.currentAudioStatusis shared; setting it unconditionally in each widget’sinitStatecan clobber the currently playing event’s state. Consider only updating when this widget is the active event (or when no event is active).♻️ Suggested guard
WidgetsBinding.instance.addPostFrameCallback((_) { - matrix.currentAudioStatus.value = AudioPlayerStatus.notDownloaded; + final isCurrentEvent = + matrix.voiceMessageEvent.value?.eventId == widget.event.eventId; + if (matrix.voiceMessageEvent.value == null || isCurrentEvent) { + matrix.currentAudioStatus.value = AudioPlayerStatus.notDownloaded; + } final durationInt = widget.event.content .tryGetMap<String, dynamic>('org.matrix.msc1767.audio') ?.tryGet<int>('duration'); @@ if (_calculatedWaveform.isEmpty) { _calculatedWaveform.addAll(waveFromHeight); - matrix.currentAudioStatus.value = AudioPlayerStatus.downloaded; + if (matrix.voiceMessageEvent.value == null || isCurrentEvent) { + matrix.currentAudioStatus.value = AudioPlayerStatus.downloaded; + } }
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
| /// | ||
| /// This mixin manages audio playback state, auto-play queue, and cleanup. | ||
| /// It should be used with State classes that need audio playback capabilities. | ||
| mixin AudioPlayerMixin<T extends StatefulWidget> on State<T> { |
| widget.matrix?.setupAudioPlayerAutoDispose(context: context); | ||
|
|
||
| matrix?.audioPlayer.play().onError((e, s) { | ||
| widget.matrix?.audioPlayer?.play().onError((e, s) { |
There was a problem hiding this comment.
why not use try/catch instead of onError?
| // Act | ||
| final result = audioMixin.calculateWaveForm( | ||
| eventWaveForm: waveform, | ||
| waveCount: 6, |
There was a problem hiding this comment.
we tried to generate to fill all the gap in message bubble
| } | ||
|
|
||
| // Memory cache for audio data | ||
| final Map<String, Uint8List> _audioMemoryCache = {}; |
There was a problem hiding this comment.
- Cache grows unbounded during session
- Moreover, we still need to re-download recent play audio.
How about implement LRU cache for audio?
| @@ -89,19 +86,21 @@ class AudioPlayerState extends State<AudioPlayerWidget> | |||
| ScaffoldMessenger.of(matrix.context).clearMaterialBanners(); | |||
| }); | |||
| if (matrix.voiceMessageEvent.value?.eventId == widget.event.eventId) { | |||
There was a problem hiding this comment.
Maybe race condition for this, in case user click to play voice message rapidly and download take time?
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@lib/pages/chat/events/audio_message/audio_player_style.dart`:
- Around line 16-26: The comment is stale and the calculation can produce a
negative wave count: update the comment in maxWaveCount to accurately list the
terms being subtracted (remove or add the mentioned safetyMargin to match the
code), then guard against negative availableWidth by clamping it to zero before
dividing (i.e., compute availableWidth using
MessageStyle.messageBubbleWidth(context) minus paddingLeft, paddingRight,
playButtonWidth, spacingAfterButton, (and optional safetyMargin if you add it to
the calculation), then if availableWidth < 0 set it to 0 before computing
availableWidth ~/ maxWaveWidth) so the returned wave count is never negative;
reference MessageStyle.messageBubbleWidth, paddingLeft, paddingRight,
playButtonWidth, spacingAfterButton, safetyMargin, availableWidth, and
maxWaveWidth.
In `@lib/presentation/mixins/audio_mixin.dart`:
- Around line 770-780: Wrap the call to
audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile)) in a try-catch
inside the block that initializes the audio player (the code that creates
audioPlayer, sets voiceMessageEvent.value, and calls
setupAudioPlayerAutoDispose). On catch, log the error, ensure the audioPlayer is
disposed/cleaned up (call dispose/stop as needed), reset voiceMessageEvent.value
to null or previous state, and return/abort further playback setup so the
exception does not propagate; keep setupAudioPlayerAutoDispose only after a
successful setAudioSource.
- Around line 547-569: putInAudioCache currently stores audio blobs even when
they're below the minimum size that getFromAudioCache rejects, wasting cache
slots; add an early size guard in putInAudioCache that mirrors getFromAudioCache
(e.g. if data.length < 1024) and simply return without modifying
_audioMemoryCache, _audioCacheAccessOrder, or _currentCacheMemoryBytes when the
blob is too small; also emit a debug/log like Logs().d indicating the small blob
was ignored so behavior matches getFromAudioCache and prevents evicting valid
entries.
In `@test/presentation/mixins/audio_mixin_test.dart`:
- Around line 672-683: The test comment is incorrect because currently
getFromAudioCache performs the size validation; update putInAudioCache to
perform the same size check (reject and do not store entries smaller than 1024
bytes) so the rejection happens at insertion time, and then adjust the test
comment in audio_mixin_test.dart to state that putInAudioCache rejects too-small
data; specifically modify the putInAudioCache implementation to check the byte
length threshold (1024) and return/skip caching when below that threshold, and
keep getFromAudioCache's existing defensive check as-is.
- Around line 685-702: The test is using a timing workaround instead of awaiting
the async cleanup; call and await the Future returned by cleanupAudioPlayer()
(the method cleanupAudioPlayer) before asserting cache state, then remove the
Future.delayed block so that after awaiting cleanupAudioPlayer you directly
verify via getFromAudioCache('event_1'), getFromAudioCache('event_2'), and
getFromAudioCache('event_3') that putInAudioCache entries were cleared.
🧹 Nitpick comments (3)
lib/presentation/mixins/audio_mixin.dart (2)
721-735: iOS OGG→CAF conversion result is not stored back in memory cache.After the OGG-to-CAF conversion on iOS (lines 722–735), the converted
audioBytesreplace the original, but the in-memory cache (populated at line 694 or 717) still holds the raw OGG bytes. On the next play of the same event, the cache hit returns OGG data, triggering conversion again every time, negating the cache benefit on iOS.Consider updating the memory cache with the converted bytes after a successful conversion:
if (oggAudioFileIniOS != null) { audioBytes = await oggAudioFileIniOS.readAsBytes(); mimeType = 'audio/x-caf'; + putInAudioCache(currentEvent.eventId, audioBytes); Logs().d('AudioMixin: Conversion successful');
498-533: LRU cache usesList.removefor access-order tracking — O(n) per access.
_audioCacheAccessOrderis aList<String>. EachgetFromAudioCachecall performs_audioCacheAccessOrder.remove(eventId)(line 528) which is O(n), andputInAudioCachealso callsremove(line 550). With up to 20 cached items this is acceptable, but using aLinkedHashMapwould provide O(1) access-order updates naturally if the cache grows.Not urgent given the small cap of 20, just noting for future consideration.
lib/presentation/mixins/event_filter_mixin.dart (1)
355-380:getAudioEventsUpToClickeddocstring is misleading about expected input order.The docstring (line 351) states the input should be "in chronological order," but the actual input from
initMediaEvents(viabuildInitialEventList) is reverse-chronological. The method internally reverses the input (line 359) to compensate. The timestamp filter at lines 375–377 acts as an additional safety net.The logic works correctly given the actual reverse-chronological input, but the docstring should reflect reality to avoid confusion for future maintainers.
| static int maxWaveCount(BuildContext context) { | ||
| return (MessageStyle.messageBubbleWidth(context) - maxBodyContentWidth) ~/ | ||
| maxWaveWidth; | ||
| // Calculate available width for waveform: | ||
| // messageBubbleWidth - padding - playButton - spacing - safetyMargin | ||
| final availableWidth = MessageStyle.messageBubbleWidth(context) - | ||
| paddingLeft - | ||
| paddingRight - | ||
| playButtonWidth - | ||
| spacingAfterButton; | ||
|
|
||
| return availableWidth ~/ maxWaveWidth; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me examine the file to see the current state of the maxWaveCount method
cat -n lib/pages/chat/events/audio_message/audio_player_style.dart | head -35Repository: linagora/twake-on-matrix
Length of output: 1468
🏁 Script executed:
# Search for minWaveCount and maxWaveCount usage
rg -n 'maxWaveCount|minWaveCount' --type=dart -C3Repository: linagora/twake-on-matrix
Length of output: 3697
🏁 Script executed:
# Look for the audio_player_style.dart file content entirely to understand context
wc -l lib/pages/chat/events/audio_message/audio_player_style.dartRepository: linagora/twake-on-matrix
Length of output: 130
Fix stale comment and guard against negative availableWidth.
Two issues:
- The comment on line 18 mentions
safetyMargin, but the calculation doesn't subtract any safety margin. - If
messageBubbleWidthis too small (e.g., on a very narrow viewport),availableWidthbecomes negative and~/ maxWaveWidthreturns a negative wave count, which is semantically invalid for a UI component.
Suggested fix
static int maxWaveCount(BuildContext context) {
- // Calculate available width for waveform:
- // messageBubbleWidth - padding - playButton - spacing - safetyMargin
+ // Calculate available width for waveform:
+ // messageBubbleWidth - padding - playButton - spacing
final availableWidth = MessageStyle.messageBubbleWidth(context) -
paddingLeft -
paddingRight -
playButtonWidth -
spacingAfterButton;
- return availableWidth ~/ maxWaveWidth;
+ return (availableWidth ~/ maxWaveWidth).clamp(minWaveCount, double.infinity).toInt();
}🤖 Prompt for AI Agents
In `@lib/pages/chat/events/audio_message/audio_player_style.dart` around lines 16
- 26, The comment is stale and the calculation can produce a negative wave
count: update the comment in maxWaveCount to accurately list the terms being
subtracted (remove or add the mentioned safetyMargin to match the code), then
guard against negative availableWidth by clamping it to zero before dividing
(i.e., compute availableWidth using MessageStyle.messageBubbleWidth(context)
minus paddingLeft, paddingRight, playButtonWidth, spacingAfterButton, (and
optional safetyMargin if you add it to the calculation), then if availableWidth
< 0 set it to 0 before computing availableWidth ~/ maxWaveWidth) so the returned
wave count is never negative; reference MessageStyle.messageBubbleWidth,
paddingLeft, paddingRight, playButtonWidth, spacingAfterButton, safetyMargin,
availableWidth, and maxWaveWidth.
| void putInAudioCache(String eventId, Uint8List data) { | ||
| if (_audioMemoryCache.containsKey(eventId)) { | ||
| _currentCacheMemoryBytes -= _audioMemoryCache[eventId]!.length; | ||
| _audioCacheAccessOrder.remove(eventId); | ||
| } | ||
|
|
||
| while ((_audioMemoryCache.length >= _maxCachedAudioItems || | ||
| _currentCacheMemoryBytes + data.length > _maxCacheMemoryBytes) && | ||
| _audioCacheAccessOrder.isNotEmpty) { | ||
| _evictOldestAudioFromCache(); | ||
| } | ||
|
|
||
| _audioMemoryCache[eventId] = data; | ||
| _audioCacheAccessOrder.add(eventId); | ||
| _currentCacheMemoryBytes += data.length; | ||
|
|
||
| Logs().d( | ||
| 'AudioMixin: Cached $eventId (${data.length} bytes). ' | ||
| 'Cache: ${_audioMemoryCache.length}/$_maxCachedAudioItems items, ' | ||
| '${_formatCacheBytes(_currentCacheMemoryBytes)}/' | ||
| '${_formatCacheBytes(_maxCacheMemoryBytes)}', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
putInAudioCache accepts entries below the minimum valid size that getFromAudioCache rejects.
getFromAudioCache (line 518) rejects entries smaller than 1024 bytes and removes them. But putInAudioCache stores them unconditionally, wasting a cache slot and potentially evicting a valid entry. Add the same size guard at the start of putInAudioCache.
Suggested fix
`@visibleForTesting`
void putInAudioCache(String eventId, Uint8List data) {
+ const minValidAudioSize = 1024;
+ if (data.length < minValidAudioSize) {
+ Logs().w(
+ 'AudioMixin: Rejecting cache entry for $eventId '
+ '(${data.length} bytes < $minValidAudioSize minimum)',
+ );
+ return;
+ }
+
if (_audioMemoryCache.containsKey(eventId)) {
_currentCacheMemoryBytes -= _audioMemoryCache[eventId]!.length;
_audioCacheAccessOrder.remove(eventId);
}🤖 Prompt for AI Agents
In `@lib/presentation/mixins/audio_mixin.dart` around lines 547 - 569,
putInAudioCache currently stores audio blobs even when they're below the minimum
size that getFromAudioCache rejects, wasting cache slots; add an early size
guard in putInAudioCache that mirrors getFromAudioCache (e.g. if data.length <
1024) and simply return without modifying _audioMemoryCache,
_audioCacheAccessOrder, or _currentCacheMemoryBytes when the blob is too small;
also emit a debug/log like Logs().d indicating the small blob was ignored so
behavior matches getFromAudioCache and prevents evicting valid entries.
| // Initialize audio player | ||
| await audioPlayer?.stop(); | ||
| await audioPlayer?.dispose(); | ||
|
|
||
| audioPlayer = AudioPlayer(); | ||
| voiceMessageEvent.value = currentEvent; | ||
|
|
||
| await audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile)); | ||
|
|
||
| // Set up auto-dispose listener | ||
| setupAudioPlayerAutoDispose(context: context); |
There was a problem hiding this comment.
setAudioSource is not wrapped in a try-catch — invalid audio data will crash.
Lines 771–777 dispose the old player and set a new audio source, but this section is outside the try-catch blocks above (download, lines 662–768) and below (play, lines 782–796). If setAudioSource throws (e.g., corrupt or unsupported audio), the exception propagates unhandled.
Suggested fix
// Initialize audio player
await audioPlayer?.stop();
await audioPlayer?.dispose();
audioPlayer = AudioPlayer();
voiceMessageEvent.value = currentEvent;
- await audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile));
-
- // Set up auto-dispose listener
- setupAudioPlayerAutoDispose(context: context);
-
try {
+ await audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile));
+
+ // Set up auto-dispose listener
+ setupAudioPlayerAutoDispose(context: context);
+
await audioPlayer?.play();
} catch (e, s) {
Logs().e('Could not play audio file', e, s);
voiceMessageEvent.value = null;
currentAudioStatus.value = AudioPlayerStatus.notDownloaded;
if (!context.mounted) return;
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(
content: Text(
e.toLocalizedString(context),
),
),
);
}🤖 Prompt for AI Agents
In `@lib/presentation/mixins/audio_mixin.dart` around lines 770 - 780, Wrap the
call to audioPlayer?.setAudioSource(MatrixFileAudioSource(matrixFile)) in a
try-catch inside the block that initializes the audio player (the code that
creates audioPlayer, sets voiceMessageEvent.value, and calls
setupAudioPlayerAutoDispose). On catch, log the error, ensure the audioPlayer is
disposed/cleaned up (call dispose/stop as needed), reset voiceMessageEvent.value
to null or previous state, and return/abort further playback setup so the
exception does not propagate; keep setupAudioPlayerAutoDispose only after a
successful setAudioSource.
| test('should clear all cache data on cleanup', () { | ||
| // Arrange | ||
| final data = Uint8List.fromList(List.generate(2048, (i) => i % 256)); | ||
| audioMixin.putInAudioCache('event_1', data); | ||
| audioMixin.putInAudioCache('event_2', data); | ||
| audioMixin.putInAudioCache('event_3', data); | ||
|
|
||
| // Act | ||
| audioMixin.cleanupAudioPlayer(); | ||
|
|
||
| // Give async cleanup time to complete | ||
| return Future.delayed(const Duration(milliseconds: 100), () { | ||
| // Assert - All cache should be cleared | ||
| expect(audioMixin.getFromAudioCache('event_1'), null); | ||
| expect(audioMixin.getFromAudioCache('event_2'), null); | ||
| expect(audioMixin.getFromAudioCache('event_3'), null); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Await cleanupAudioPlayer() instead of relying on Future.delayed.
cleanupAudioPlayer() returns a Future<void>, but line 693 discards it. The test then uses Future.delayed(100ms) as a timing-based workaround, which is inherently flaky.
Suggested fix
- test('should clear all cache data on cleanup', () {
+ test('should clear all cache data on cleanup', () async {
// Arrange
final data = Uint8List.fromList(List.generate(2048, (i) => i % 256));
audioMixin.putInAudioCache('event_1', data);
audioMixin.putInAudioCache('event_2', data);
audioMixin.putInAudioCache('event_3', data);
// Act
- audioMixin.cleanupAudioPlayer();
+ await audioMixin.cleanupAudioPlayer();
- // Give async cleanup time to complete
- return Future.delayed(const Duration(milliseconds: 100), () {
- // Assert - All cache should be cleared
- expect(audioMixin.getFromAudioCache('event_1'), null);
- expect(audioMixin.getFromAudioCache('event_2'), null);
- expect(audioMixin.getFromAudioCache('event_3'), null);
- });
+ // Assert - All cache should be cleared
+ expect(audioMixin.getFromAudioCache('event_1'), null);
+ expect(audioMixin.getFromAudioCache('event_2'), null);
+ expect(audioMixin.getFromAudioCache('event_3'), null);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should clear all cache data on cleanup', () { | |
| // Arrange | |
| final data = Uint8List.fromList(List.generate(2048, (i) => i % 256)); | |
| audioMixin.putInAudioCache('event_1', data); | |
| audioMixin.putInAudioCache('event_2', data); | |
| audioMixin.putInAudioCache('event_3', data); | |
| // Act | |
| audioMixin.cleanupAudioPlayer(); | |
| // Give async cleanup time to complete | |
| return Future.delayed(const Duration(milliseconds: 100), () { | |
| // Assert - All cache should be cleared | |
| expect(audioMixin.getFromAudioCache('event_1'), null); | |
| expect(audioMixin.getFromAudioCache('event_2'), null); | |
| expect(audioMixin.getFromAudioCache('event_3'), null); | |
| }); | |
| }); | |
| test('should clear all cache data on cleanup', () async { | |
| // Arrange | |
| final data = Uint8List.fromList(List.generate(2048, (i) => i % 256)); | |
| audioMixin.putInAudioCache('event_1', data); | |
| audioMixin.putInAudioCache('event_2', data); | |
| audioMixin.putInAudioCache('event_3', data); | |
| // Act | |
| await audioMixin.cleanupAudioPlayer(); | |
| // Assert - All cache should be cleared | |
| expect(audioMixin.getFromAudioCache('event_1'), null); | |
| expect(audioMixin.getFromAudioCache('event_2'), null); | |
| expect(audioMixin.getFromAudioCache('event_3'), null); | |
| }); |
🤖 Prompt for AI Agents
In `@test/presentation/mixins/audio_mixin_test.dart` around lines 685 - 702, The
test is using a timing workaround instead of awaiting the async cleanup; call
and await the Future returned by cleanupAudioPlayer() (the method
cleanupAudioPlayer) before asserting cache state, then remove the Future.delayed
block so that after awaiting cleanupAudioPlayer you directly verify via
getFromAudioCache('event_1'), getFromAudioCache('event_2'), and
getFromAudioCache('event_3') that putInAudioCache entries were cleared.
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
There was a problem hiding this comment.
- When select another audio record when the previous one is still loading, user has to wait for the previous one to download
ap-fast-select.mov
- There's no loading indicator when user tap an audio record
ap-loading-missing.mov
- If the audio is cached, why does it take so long to play the same audio record again?
ap-not-cached.mov
- The progresses in the audio wave and in pinned audio player are desynced
ap-progresses-desync.mov
| class _ChatAudioPlayerWidgetState extends State<ChatAudioPlayerWidget> | ||
| with AudioMixin { |
There was a problem hiding this comment.
Can AudioMixin be used with StatelessWidget? Why does ChatAudioPlayerWidget need to be StatefulWidget
There was a problem hiding this comment.
but it's mixing in AudioMixin which has mutable fields. This violates Flutter's immutability requirement.
Simulator.Screen.Recording.-.iPhone.17.-.2026-02-11.at.15.36.07.mov |
… fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! TW-2796: Implement autoplay for voice messages and enhance audio event handling
3 participants
Ticket
Related issue
Resolved
Attach screenshots or videos demonstrating the changes
Simulator.Screen.Recording.-.iPhone.Air.-.2025-12-23.at.16.58.38.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-01-12.at.14.03.35.mov
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements