Add encrypted file messaging support #367
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds end-to-end encrypted image/file messaging: encryption primitives and blob format, Blossom upload/download clients with retries, validation/sanitization, notifier shared-key access and in-memory caches, UI widgets and upload/upload flows, tests, localization entries, dependencies, and a targeted AndroidX Gradle resolution block. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as EncryptedImageMessage
participant Notifier as ChatRoomNotifier
participant ImgSvc as EncryptedImageUploadService
participant DL as BlossomDownloadService
participant Enc as EncryptionService
User->>UI: open message
UI->>Notifier: getCachedImage(messageId)
alt cached
Notifier-->>UI: image bytes
UI->>User: render image
else not cached
UI->>UI: parse metadata (blossomUrl, nonce)
UI->>Notifier: getSharedKey()
Notifier-->>UI: sharedKey
UI->>ImgSvc: downloadAndDecryptImage(blossomUrl, sharedKey)
ImgSvc->>DL: downloadWithRetry(blossomUrl)
DL-->>ImgSvc: encrypted blob
ImgSvc->>Enc: decryptFromBlob(blob, sharedKey)
Enc-->>ImgSvc: plaintext bytes
ImgSvc-->>UI: image bytes
UI->>Notifier: cacheDecryptedImage(messageId, bytes, metadata)
UI->>User: render image
end
sequenceDiagram
actor User
participant Input as MessageInput
participant Picker as FilePicker
participant FileVal as FileValidationService
participant EncSvc as EncryptedFileUploadService
participant Blossom as BlossomClient
participant Notifier as ChatRoomNotifier
User->>Input: tap attach
Input->>Picker: pickFile()
Picker-->>Input: File
Input->>Notifier: getSharedKey()
Notifier-->>Input: sharedKey
Input->>FileVal: validateFile(file)
FileVal-->>Input: FileValidationResult
Input->>EncSvc: uploadEncryptedFile(file, sharedKey)
loop try blossom servers
EncSvc->>Blossom: uploadImage(encryptedData)
Blossom-->>EncSvc: blossomUrl / error
end
EncSvc-->>Input: EncryptedFileUploadResult
Input->>Notifier: sendEncryptedFileMessage(result)
Notifier-->>User: message sent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (24)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md (3)
31-45: Add language specifier to fenced code blocks for consistency.Static analysis flagged missing language specifiers. For ASCII diagrams, use
textorplaintext.-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐
145-149: Add language specifier for blob structure diagram.-``` +```text ┌─────────────┬──────────────────┬─────────────────┐
151-156: Clarify forward secrecy claim.Line 155 states "Forward Secrecy: Compromised key doesn't affect past messages." However, based on line 140-142, the same
shared_keyis used for the entire chat session. True forward secrecy typically requires per-message or periodic key rotation. Consider clarifying this is session-level isolation rather than per-message forward secrecy.lib/services/blossom_download_service.dart (1)
62-67: Avoid mutating parameter; use a local variable for backoff.Reassigning
retryDelaymutates the parameter. WhileDurationis immutable and this doesn't affect the caller, mutating parameters is a code smell and can confuse readers.static Future<Uint8List> downloadWithRetry( String blossomUrl, { int maxRetries = 3, - Duration retryDelay = const Duration(seconds: 1), + Duration initialRetryDelay = const Duration(seconds: 1), }) async { _logger.i('📥 Download with retry: $blossomUrl (max $maxRetries attempts)'); Exception? lastException; + var currentDelay = initialRetryDelay; for (int attempt = 1; attempt <= maxRetries; attempt++) { try { _logger.d('Attempt $attempt/$maxRetries'); return await downloadFromBlossom(blossomUrl); } catch (e) { lastException = e is Exception ? e : Exception(e.toString()); _logger.w('❌ Attempt $attempt failed: $e'); if (attempt < maxRetries) { - _logger.d('⏳ Waiting ${retryDelay.inMilliseconds}ms before retry...'); - await Future.delayed(retryDelay); + _logger.d('⏳ Waiting ${currentDelay.inMilliseconds}ms before retry...'); + await Future.delayed(currentDelay); // Exponential backoff - retryDelay = Duration(milliseconds: (retryDelay.inMilliseconds * 1.5).round()); + currentDelay = Duration(milliseconds: (currentDelay.inMilliseconds * 1.5).round()); } } }lib/services/image_upload_service.dart (2)
63-94: Remove unreachable code at line 93.The
throw BlossomException('No Blossom servers available')at line 93 is unreachable. The loop either returns on success (line 78) or throws on the last iteration (line 85). Ifservers.lengthis 0, the loop body never executes, and we'd still reach line 93—but that would mean_blossomServersis empty, which is a static configuration issue.// Continue with next server continue; } } - throw BlossomException('No Blossom servers available'); + // This is only reachable if _blossomServers is empty + throw BlossomException('No Blossom servers configured'); }Alternatively, add an early guard:
Future<String> _uploadWithRetry(Uint8List imageData, String mimeType) async { final servers = _blossomServers; + + if (servers.isEmpty) { + throw BlossomException('No Blossom servers configured'); + } for (int i = 0; i < servers.length; i++) {
11-21: Consider extracting shared Blossom server list.The server list is duplicated across
ImageUploadService,EncryptedImageUploadService, andEncryptedFileUploadService. This could be centralized in a shared configuration or constant file to avoid drift.lib/features/chat/widgets/message_input.dart (1)
126-140: Avoid losing stack trace when re-throwing exceptions.Wrapping the exception with
throw Exception(...)loses the original stack trace. Consider usingrethroworError.throwWithStackTrace.Future<void> _sendEncryptedFileMessage(EncryptedFileUploadResult result) async { - try { - // Create JSON content for the rumor - final fileMessageJson = jsonEncode(result.toJson()); - - // Send via existing chat system (will be wrapped in NIP-59) - await ref - .read(chatRoomsProvider(widget.orderId).notifier) - .sendMessage(fileMessageJson); - - } catch (e) { - throw Exception('Failed to send encrypted file message: $e'); - } + // Create JSON content for the rumor + final fileMessageJson = jsonEncode(result.toJson()); + + // Send via existing chat system (will be wrapped in NIP-59) + // Exceptions will propagate with original stack trace + await ref + .read(chatRoomsProvider(widget.orderId).notifier) + .sendMessage(fileMessageJson); }The caller at line 98 already catches and handles exceptions, so this inner try-catch is redundant.
lib/services/blossom_client.dart (1)
68-72: Consider parsing the actual URL from the response body.The Blossom server typically returns JSON with the blob descriptor including the actual URL. Constructing the URL from the hash assumes a specific server structure. If the server returns the URL differently (e.g., with a file extension or different path), this could break.
if (response.statusCode == 200 || response.statusCode == 201) { - // Blossom returns the file URL in the response, construct from hash - final blossomUrl = '$serverUrl/$hashHex'; + // Parse the response to get the actual URL from Blossom + try { + final responseData = jsonDecode(response.body) as Map<String, dynamic>; + final blossomUrl = responseData['url'] as String? ?? '$serverUrl/$hashHex'; + _logger.i('✅ Image uploaded successfully to Blossom: $blossomUrl'); + return blossomUrl; + } catch (_) { + // Fallback to constructed URL if response parsing fails + final blossomUrl = '$serverUrl/$hashHex'; + _logger.i('✅ Image uploaded successfully to Blossom: $blossomUrl'); + return blossomUrl; + } - _logger.i('✅ Image uploaded successfully to Blossom: $blossomUrl'); - return blossomUrl;lib/services/media_validation_service.dart (1)
102-139: Consider offloading image processing to an isolate.Image decoding and re-encoding are CPU-intensive operations that can freeze the UI for large images. Use Flutter's
compute()function to run this work in a background isolate.// Example approach: static Future<MediaValidationResult> validateAndSanitizeImage( Uint8List imageData, ) async { // ... validation checks ... // Offload heavy work to isolate return await compute(_processImage, imageData); } static MediaValidationResult _processImage(Uint8List imageData) { // Move decoding and encoding logic here }lib/features/chat/widgets/message_bubble.dart (1)
62-134: Extract common bubble container to reduce duplication.The container, constraints, decoration, and Row structure are nearly identical for encrypted images, encrypted files, and text messages. Consider extracting a shared
_buildBubbleContainermethod.Widget _buildBubbleContainer({ required BuildContext context, required bool isFromPeer, required Widget child, EdgeInsets padding = const EdgeInsets.all(8), }) { return Container( padding: const EdgeInsets.symmetric(vertical: 4, horizontal: 16), alignment: isFromPeer ? Alignment.centerLeft : Alignment.centerRight, child: Row( mainAxisAlignment: isFromPeer ? MainAxisAlignment.start : MainAxisAlignment.end, children: [ Flexible( child: ConstrainedBox( constraints: BoxConstraints( maxWidth: MediaQuery.of(context).size.width * 0.75, minWidth: 0, ), child: Container( padding: padding, decoration: BoxDecoration( color: isFromPeer ? _getPeerMessageColor(peerPubkey) : AppTheme.purpleAccent, borderRadius: BorderRadius.only( topLeft: const Radius.circular(16), topRight: const Radius.circular(16), bottomLeft: Radius.circular(isFromPeer ? 4 : 16), bottomRight: Radius.circular(isFromPeer ? 16 : 4), ), ), child: child, ), ), ), ], ), ); }lib/services/file_validation_service.dart (1)
77-78: Use thepathpackage for cross-platform filename extraction.
file.path.split('/').lastassumes Unix-style paths. For cross-platform compatibility, use thepathpackage.+import 'package:path/path.dart' as p; - final filename = file.path.split('/').last; + final filename = p.basename(file.path);lib/features/chat/widgets/encrypted_file_message.dart (2)
36-40: Useref.watchinstead ofref.readfor reactive state inbuild().Using
ref.readinbuild()means the widget won't rebuild when the chat notifier's state changes (e.g., when cached files are updated). This could cause stale UI.@override Widget build(BuildContext context) { - final chatNotifier = ref.read(chatRoomsProvider(widget.orderId).notifier); + final chatNotifier = ref.watch(chatRoomsProvider(widget.orderId).notifier); // Check if file is already cached - final cachedFile = chatNotifier.getCachedFile(widget.message.id!); - final fileMetadata = chatNotifier.getFileMetadata(widget.message.id!); + final messageId = widget.message.id; + if (messageId == null) { + return _buildErrorWidget(); + } + final cachedFile = chatNotifier.getCachedFile(messageId); + final fileMetadata = chatNotifier.getFileMetadata(messageId);
62-70: Potential infinite rebuild loop on image auto-download.If
_downloadFile()fails and sets_errorMessage, the widget rebuilds. Since_errorMessage != nullis checked before reaching line 63, this specific path won't loop. However, if the metadata parsing succeeds but download keeps failing transiently without setting error state, this could cause repeated download attempts on each rebuild.Consider adding a flag to track that auto-download was already attempted.
class _EncryptedFileMessageState extends ConsumerState<EncryptedFileMessage> { bool _isLoading = false; String? _errorMessage; + bool _autoDownloadAttempted = false; ... // For images, try to download automatically for preview if (_isImage(metadata.fileType)) { WidgetsBinding.instance.addPostFrameCallback((_) { - if (!_isLoading) { + if (!_isLoading && !_autoDownloadAttempted) { + _autoDownloadAttempted = true; _downloadFile(); } }); }lib/services/encrypted_image_upload_service.dart (3)
46-58: Add null safety checks infromJsonfactory.Direct map access without null checks will throw if any field is missing, making debugging difficult. Consider explicit checks or using null-aware operators with defaults.
factory EncryptedImageUploadResult.fromJson(Map<String, dynamic> json) { + final blossomUrl = json['blossom_url']; + final nonce = json['nonce']; + if (blossomUrl == null || nonce == null) { + throw FormatException('Missing required fields in encrypted image metadata'); + } return EncryptedImageUploadResult( - blossomUrl: json['blossom_url'], - nonce: json['nonce'], - mimeType: json['mime_type'], - originalSize: json['original_size'], - width: json['width'], - height: json['height'], - filename: json['filename'], - encryptedSize: json['encrypted_size'], + blossomUrl: blossomUrl as String, + nonce: nonce as String, + mimeType: json['mime_type'] as String? ?? 'application/octet-stream', + originalSize: json['original_size'] as int? ?? 0, + width: json['width'] as int? ?? 0, + height: json['height'] as int? ?? 0, + filename: json['filename'] as String? ?? 'unknown', + encryptedSize: json['encrypted_size'] as int? ?? 0, ); }
3-3: Remove unused import.
flutter/foundation.dartis imported but not used in this file.import 'dart:io'; import 'dart:typed_data'; -import 'package:flutter/foundation.dart'; import 'package:logger/logger.dart';
226-231: DuplicateBlossomExceptionclass definition.This exception class is also defined in
encrypted_file_upload_service.dart. Consider consolidating into a shared location to avoid confusion and ensure consistent behavior.Consider moving
BlossomExceptionto a shared file likelib/services/blossom_exceptions.dartand importing it in both services.lib/features/chat/notifiers/chat_room_notifier.dart (4)
478-484: In-memory caches grow unbounded - potential memory leak.The
_imageCacheand_fileCachemaps store decrypted file data indefinitely. For long chat sessions with many media files, this could consume significant memory.Consider implementing cache eviction (e.g., LRU with size limit) or clearing caches on dispose.
+ static const int _maxCacheEntries = 50; + // Simple cache for decrypted images final Map<String, Uint8List> _imageCache = {}; final Map<String, EncryptedImageUploadResult> _imageMetadata = {}; // Simple cache for decrypted files final Map<String, Uint8List> _fileCache = {}; final Map<String, EncryptedFileUploadResult> _fileMetadata = {}; + void _evictOldestCacheEntries() { + while (_imageCache.length > _maxCacheEntries) { + _imageCache.remove(_imageCache.keys.first); + _imageMetadata.remove(_imageMetadata.keys.first); + } + while (_fileCache.length > _maxCacheEntries) { + _fileCache.remove(_fileCache.keys.first); + _fileMetadata.remove(_fileMetadata.keys.first); + } + }Then call
_evictOldestCacheEntries()incacheDecryptedImageandcacheDecryptedFile.
442-476: Pre-downloading blocks message processing - consider fire-and-forget.
_processEncryptedImageMessageis awaited during_onChatEvent, which means message display is delayed until the image downloads. For slow networks, this could significantly delay showing new messages.Consider using fire-and-forget for pre-downloads:
- await _processMessageContent(chat); + // Process in background - don't block message display + _processMessageContent(chat);The UI widgets already handle the case where cached data isn't available.
558-569: Clarify API:cacheDecryptedFileaccepts nullablefileData.The method signature allows
nullforfileData, which is used to store metadata without file data (lines 544, 549). This is a valid pattern but could be confusing. Consider documenting this behavior or using a separate method.- /// Cache a decrypted file for quick display + /// Cache a decrypted file for quick display. + /// Pass null for [fileData] to cache only metadata (for on-demand download). void cacheDecryptedFile( String messageId, Uint8List? fileData, EncryptedFileUploadResult metadata ) {
607-613: Clear caches on dispose to free memory.The caches should be cleared when the notifier is disposed to ensure memory is released.
@override void dispose() { _subscription?.cancel(); _sessionListener?.close(); + _imageCache.clear(); + _imageMetadata.clear(); + _fileCache.clear(); + _fileMetadata.clear(); _logger.i('Disposed chat room notifier for orderId: $orderId'); super.dispose(); }lib/services/encrypted_file_upload_service.dart (4)
43-54: Add null safety checks infromJsonfactory.Same issue as
EncryptedImageUploadResult- direct map access without null checks will throw unhelpful errors if fields are missing.factory EncryptedFileUploadResult.fromJson(Map<String, dynamic> json) { + final blossomUrl = json['blossom_url']; + final nonce = json['nonce']; + final fileType = json['file_type']; + if (blossomUrl == null || nonce == null || fileType == null) { + throw FormatException('Missing required fields in encrypted file metadata'); + } return EncryptedFileUploadResult( - blossomUrl: json['blossom_url'], - nonce: json['nonce'], - mimeType: json['mime_type'], - fileType: json['file_type'], - originalSize: json['original_size'], - filename: json['filename'], - encryptedSize: json['encrypted_size'], + blossomUrl: blossomUrl as String, + nonce: nonce as String, + mimeType: json['mime_type'] as String? ?? 'application/octet-stream', + fileType: fileType as String, + originalSize: json['original_size'] as int? ?? 0, + filename: json['filename'] as String? ?? 'unknown', + encryptedSize: json['encrypted_size'] as int? ?? 0, ); }
60-71: Duplicate Blossom server list - consider centralizing.The same server list exists in both
EncryptedFileUploadServiceandEncryptedImageUploadService. This duplication could lead to inconsistencies if servers are added/removed.Consider creating a shared configuration:
// lib/services/blossom_config.dart class BlossomConfig { static const List<String> servers = [ 'https://blossom.primal.net', 'https://blossom.band', // ... etc ]; }
3-3: Remove unused import.
flutter/foundation.dartis imported but not used.import 'dart:io'; import 'dart:typed_data'; -import 'package:flutter/foundation.dart'; import 'package:logger/logger.dart';
216-222: DuplicateBlossomExceptionclass.This is defined identically in
encrypted_image_upload_service.dart. Consolidate to avoid potential import conflicts.Move to a shared location like
lib/services/blossom_exceptions.dart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
android/app/build.gradle(1 hunks)docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md(1 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(3 hunks)lib/features/chat/screens/chat_room_screen.dart(1 hunks)lib/features/chat/widgets/chat_messages_list.dart(2 hunks)lib/features/chat/widgets/encrypted_file_message.dart(1 hunks)lib/features/chat/widgets/encrypted_image_message.dart(1 hunks)lib/features/chat/widgets/message_bubble.dart(2 hunks)lib/features/chat/widgets/message_input.dart(5 hunks)lib/services/blossom_client.dart(1 hunks)lib/services/blossom_download_service.dart(1 hunks)lib/services/encrypted_file_upload_service.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)lib/services/encryption_service.dart(1 hunks)lib/services/file_validation_service.dart(1 hunks)lib/services/image_upload_service.dart(1 hunks)lib/services/media_validation_service.dart(1 hunks)pubspec.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-17T20:45:32.468Z
Learnt from: grunch
Repo: MostroP2P/mobile PR: 306
File: docs/architecture/REQUEST_ID_ANALYSIS.md:176-183
Timestamp: 2025-09-17T20:45:32.468Z
Learning: For PR #306 in MostroP2P/mobile repository: This is a documentation-only PR intended to explain how the current requestId system works, not to fix any bugs or issues in the code. The documentation should accurately reflect existing behavior.
Applied to files:
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/chat/widgets/message_bubble.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (21)
android/app/build.gradle (1)
115-122: Targeted AndroidX conflict resolution looks appropriate.The minimal approach of forcing only the problematic dependencies rather than all AndroidX artifacts is a good practice. The comment clearly documents the intent.
Consider documenting which specific dependency introduced this conflict (likely
file_pickeroropen_file) in case future package updates resolve it naturally.lib/services/blossom_download_service.dart (1)
10-42: Download implementation looks solid.Good use of timeout, proper error handling that distinguishes domain exceptions from network errors, and appropriate logging.
lib/features/chat/screens/chat_room_screen.dart (1)
157-162: LGTM!Clean propagation of
orderIdtoChatMessagesListenabling encrypted media components to access order context.lib/features/chat/widgets/chat_messages_list.dart (2)
10-19: LGTM!Clean addition of
orderIdfield and constructor requirement.
131-138: LGTM!Correct propagation of
orderIdto eachMessageBubblefor encrypted media context.lib/services/image_upload_service.dart (1)
26-60: Upload workflow looks solid.Good separation of concerns: read → validate/sanitize → upload with retry. Logging provides visibility into the process.
lib/features/chat/widgets/message_input.dart (2)
69-99: File selection and upload flow is well-structured.Good use of loading state, mounted checks, and finally block for cleanup. The flow correctly chains file picking → key retrieval → encryption/upload → message sending.
169-200: Attach button UI implementation is clean.Good handling of loading state with disabled button and spinner indicator. Consistent styling with the existing send button.
lib/services/blossom_client.dart (1)
9-17: LGTM!The client structure is clean with configurable timeout and proper dependency injection of the server URL.
lib/services/media_validation_service.dart (1)
5-45: LGTM!The enum design with extension methods for mime type and extension mappings is clean and maintainable.
lib/features/chat/widgets/message_bubble.dart (1)
38-53: LGTM!The migration to
ConsumerWidgetand addition oforderIdfor threading context through to child widgets is clean.lib/features/chat/widgets/encrypted_image_message.dart (1)
55-139: LGTM!The image display widget with proper constraints, error handling in
Image.memory, and metadata display is well-implemented.lib/services/file_validation_service.dart (2)
24-28: LGTM!The service structure with clear constants for size limits and supported types is clean and maintainable.
76-120: Validation logic is sound.The multi-step validation (size check → MIME detection → fallback to extension → type verification) is a robust approach for file validation.
lib/features/chat/widgets/encrypted_file_message.dart (1)
511-522: LGTM - helper function correctly detects encrypted file messages.The
isEncryptedFileMessagehelper properly handles null content and JSON parsing exceptions, returning false on any failure.lib/services/encryption_service.dart (4)
55-67: LGTM - Secure random initialization is correct.The
SecureRandomis seeded with cryptographically secure bytes fromRandom.secure(). The Fortuna CSPRNG is appropriate for generating nonces.
79-132: LGTM - ChaCha20-Poly1305 encryption implementation is correct.Key and nonce length validation is properly enforced. The cipher is correctly initialized with AEAD parameters, and the authentication tag is properly extracted from the output.
134-187: LGTM - Decryption with proper authentication tag verification.The implementation correctly validates input lengths and combines encrypted data with auth tag for AEAD decryption. PointyCastle's
doFinalwill throw if authentication fails.
34-52: LGTM - Blob serialization is well-structured.The
fromBlobmethod correctly validates minimum blob size (28 bytes for nonce + auth tag) and extracts components in the documented order:[nonce][encrypted_data][auth_tag].lib/services/encrypted_image_upload_service.dart (1)
181-213: LGTM - Upload retry logic with server fallback is well implemented.The implementation correctly iterates through multiple Blossom servers and only throws after all servers have been exhausted.
lib/services/encrypted_file_upload_service.dart (1)
75-131: LGTM - Upload flow is well-structured.The upload flow correctly validates, encrypts, and uploads with proper error handling and logging.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
lib/features/chat/notifiers/chat_room_notifier.dart (1)
103-107: Don't block chat event processing on Blossom downloads/decryptions.
_onChatEventawaits_processMessageContent(chat), and the processing path can perform multi-minute network operations (BlossomdownloadWithRetry+ decryption). If Blossom is slow or unreachable, chat message delivery and event handling for this room are effectively stalled until those calls finish.Consider kicking off the encrypted-content prefetch in the background instead of blocking the event pipeline, e.g.:
- // Process special message types (e.g., encrypted images) - await _processMessageContent(chat); + // Process special message types (e.g., encrypted images) in background + // so message delivery isn't blocked by Blossom downloads/decryptions. + unawaited(_processMessageContent(chat));
_processMessageContentalready catches and logs its own errors, so running it unawaited should be safe while significantly improving responsiveness.Also applies to: 151-155, 415-451, 453-487, 518-566
♻️ Duplicate comments (3)
lib/features/chat/widgets/message_input.dart (1)
121-130: Avoid exposing raw exception details to users.The error message
'Error uploading file: $e'may expose internal details (file paths, network errors, stack traces) to users.Apply the previously suggested fix:
} catch (e) { // Show error to user if (mounted) { + debugPrint('File upload error: $e'); ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text('Error uploading file: $e'), + content: Text('Failed to upload file. Please try again.'), backgroundColor: Colors.red, ), ); } } finally {As per coding guidelines, user-facing strings should be localized via ARB files.
lib/features/chat/widgets/encrypted_file_message.dart (1)
35-43: Handle potential nullmessage.iddefensively before cache/file operations.
widget.message.id!is force-unwrapped inbuild(),_downloadFile(), and_openFile(). If an event ever arrives without an ID (or with a partially constructed model), this will throw and break the whole message list.Mirror the defensive pattern used in
EncryptedImageMessageby reading the ID into a local, checking for null once, and then using the non-null local everywhere:@override Widget build(BuildContext context) { final chatNotifier = ref.read(chatRoomsProvider(widget.orderId).notifier); - - // Check if file is already cached - final cachedFile = chatNotifier.getCachedFile(widget.message.id!); - final fileMetadata = chatNotifier.getFileMetadata(widget.message.id!); + + // Handle null message ID defensively + final messageId = widget.message.id; + if (messageId == null) { + return _buildErrorWidget(); + } + + // Check if file is already cached + final cachedFile = chatNotifier.getCachedFile(messageId); + final fileMetadata = chatNotifier.getFileMetadata(messageId);And in the async methods:
Future<void> _downloadFile() async { if (_isLoading) return; + final messageId = widget.message.id; + if (messageId == null) { + setState(() { + _errorMessage = 'Invalid message: missing ID'; + }); + return; + } + @@ - // Cache the file - chatNotifier.cacheDecryptedFile(widget.message.id!, decryptedFile, metadata); + // Cache the file + chatNotifier.cacheDecryptedFile(messageId, decryptedFile, metadata); @@ Future<void> _openFile() async { try { final chatNotifier = ref.read(chatRoomsProvider(widget.orderId).notifier); - final cachedFile = chatNotifier.getCachedFile(widget.message.id!); - final metadata = chatNotifier.getFileMetadata(widget.message.id!); + final messageId = widget.message.id; + if (messageId == null) { + throw Exception('Invalid message: missing ID'); + } + final cachedFile = chatNotifier.getCachedFile(messageId); + final metadata = chatNotifier.getFileMetadata(messageId);This keeps the widget robust even if upstream models change.
Also applies to: 407-435, 451-456
lib/services/encrypted_image_upload_service.dart (1)
150-179: Clarify or remove unusednonceHexparameter indownloadAndDecryptImage.
downloadAndDecryptImageacceptsnonceHexand logs it, but the actual decryption delegates toEncryptionService.decryptFromBlobwhich derives the nonce from the blob itself. As a result,nonceHexhas no effect on correctness and can be misleading to readers/callers.Either:
- Remove
nonceHexfrom the method signature and all call sites (e.g., inEncryptedImageMessageand any other users), updating docs/comments to state that the nonce is read from the blob; or- Use
nonceHexfor validation (e.g., decode it and verify the nonce extracted from the blob matches the expected value, failing fast with a clear error if it doesn’t).Keeping the API and its behaviour aligned will reduce confusion and potential future misuse.
Check how `EncryptionService.decryptFromBlob` for ChaCha20-Poly1305 in Flutter/Dart typically handles nonces (does it always embed the nonce in the blob, and is it standard to ignore an external nonce parameter in such APIs?).
🧹 Nitpick comments (14)
lib/services/media_validation_service.dart (3)
61-65: Consider removing unnecessaryasynckeyword.Both
validateAndSanitizeImage,validateAndSanitizeImageLight, and_sanitizeImageHeavyare marked asasyncbut contain noawaitexpressions. This adds unnecessary overhead by wrapping results in Futures.If async is intentional for future extensibility, consider documenting why; otherwise:
- static Future<MediaValidationResult> validateAndSanitizeImage( + static MediaValidationResult validateAndSanitizeImage( Uint8List imageData, - ) async { - return _sanitizeImageHeavy(imageData); + ) { + return _sanitizeImageHeavySync(imageData); }Also applies to: 72-75, 145-148
72-142: Consider extracting common validation logic.The light and heavy sanitization methods share nearly identical structure (MIME detection, decoding, re-encoding). The only differences are JPEG quality (95 vs 90) and log messages. Consider extracting to a private helper with a quality parameter.
Also applies to: 145-214
30-34: Static constant in extension is non-idiomatic.Consider moving
allto the enum itself usingSupportedImageType.valueswhich is auto-generated, or define it outside the extension.enum SupportedImageType { jpeg, png, + + static const List<SupportedImageType> all = values; } extension SupportedImageTypeExtension on SupportedImageType { // ... mimeType and extension getters only - static const List<SupportedImageType> all = [ - SupportedImageType.jpeg, - SupportedImageType.png, - ]; }Or simply use
SupportedImageType.valuesdirectly at call sites.lib/services/file_validation_service.dart (2)
280-287: AVI header validation is slightly incomplete.The AVI format technically has "AVI " (with a trailing space) or "AVIX" for extended AVI. The current check for just "AVI" (3 chars) works but may miss edge cases.
case 'video/x-msvideo': // AVI files start with RIFF header followed by AVI final riff = String.fromCharCodes(fileData.take(4)); - final avi = String.fromCharCodes(fileData.skip(8).take(3)); - if (riff != 'RIFF' || avi != 'AVI') { + final avi = String.fromCharCodes(fileData.skip(8).take(4)); + if (riff != 'RIFF' || !avi.startsWith('AVI')) { throw FileValidationException('Invalid AVI file structure'); } break;
180-201: Validation helper methods are unnecessarilyasync.
_validatePdfStructure,_validateDocumentStructure, and_validateVideoStructureare markedasyncbut contain noawaitexpressions. This adds overhead by wrapping synchronous operations in Futures.- static Future<void> _validatePdfStructure(Uint8List fileData) async { + static void _validatePdfStructure(Uint8List fileData) {Update the caller in
_performTypeSpecificValidationaccordingly.Also applies to: 204-254, 257-291
lib/features/chat/widgets/message_input.dart (2)
97-118: File is read twice - consider passing bytes to upload services.The file bytes are read at line 97 for MIME type detection, then the file is passed to upload services which read it again internally. For large files (up to 25MB), this doubles memory usage and I/O.
Consider modifying the upload services to accept
Uint8Listdirectly, or cache the bytes:if (isImage) { - final uploadResult = await _imageUploadService.uploadEncryptedImage( - imageFile: file, + final uploadResult = await _imageUploadService.uploadEncryptedImageFromBytes( + imageData: fileData, sharedKey: sharedKey, filename: filename, );
35-36: Consider using Riverpod providers for services.The upload services are instantiated as instance fields. Per coding guidelines, consider encapsulating service access through Riverpod providers for consistency.
// Define providers in a providers file final encryptedFileUploadServiceProvider = Provider((ref) => EncryptedFileUploadService()); final encryptedImageUploadServiceProvider = Provider((ref) => EncryptedImageUploadService()); // Then in the widget: final fileService = ref.read(encryptedFileUploadServiceProvider);lib/features/chat/utils/message_type_helpers.dart (1)
33-37: Minor:getMessageTypemay parse JSON twice.When the message is an encrypted file,
getMessageTypeparses the JSON content once inisEncryptedImageMessage(returns false) and again inisEncryptedFileMessage. For frequently called code paths, consider parsing once:static MessageContentType getMessageType(NostrEvent message) { try { final content = message.content; if (content == null || !content.startsWith('{')) { return MessageContentType.text; } final jsonContent = jsonDecode(content) as Map<String, dynamic>; final type = jsonContent['type']; if (type == 'image_encrypted') return MessageContentType.encryptedImage; if (type == 'file_encrypted') return MessageContentType.encryptedFile; return MessageContentType.text; } catch (e) { return MessageContentType.text; } }lib/features/chat/widgets/message_bubble.dart (2)
12-12:ConsumerWidgetnot utilized -refparameter is unused.The widget was changed to
ConsumerWidgetbut theWidgetRef refparameter is never used in thebuildmethod. If Riverpod access isn't needed here, consider reverting toStatelessWidgetto avoid unnecessary overhead.-class MessageBubble extends ConsumerWidget { +class MessageBubble extends StatelessWidget { // ... @override - Widget build(BuildContext context, WidgetRef ref) { + Widget build(BuildContext context) {Or if
refwill be needed for future features, add a TODO comment explaining the intent.Also applies to: 24-25
36-108: Consider extracting common bubble container logic.The encrypted image (lines 36-70) and encrypted file (lines 73-108) branches share identical container/decoration logic. A private helper could reduce duplication:
Widget _buildEncryptedMessageBubble({ required BuildContext context, required bool isFromPeer, required Widget child, }) { return Container( padding: const EdgeInsets.symmetric(vertical: 4, horizontal: 16), alignment: isFromPeer ? Alignment.centerLeft : Alignment.centerRight, child: Row( mainAxisAlignment: isFromPeer ? MainAxisAlignment.start : MainAxisAlignment.end, children: [ Flexible( child: ConstrainedBox( constraints: BoxConstraints( maxWidth: MediaQuery.of(context).size.width * 0.75, minWidth: 0, ), child: Container( padding: const EdgeInsets.all(8), decoration: BoxDecoration( color: isFromPeer ? _getPeerMessageColor(peerPubkey) : AppTheme.purpleAccent, borderRadius: BorderRadius.only( topLeft: const Radius.circular(16), topRight: const Radius.circular(16), bottomLeft: Radius.circular(isFromPeer ? 4 : 16), bottomRight: Radius.circular(isFromPeer ? 16 : 4), ), ), child: child, ), ), ), ], ), ); }lib/features/chat/widgets/encrypted_image_message.dart (1)
174-217: Localize all user-facing strings in loading/error states.
_buildLoadingWidget,_buildErrorWidget, and the_errorMessagevalues use hard‑coded English text ('Decrypting image...','Failed to load image','Invalid message: missing ID', and raw exception messages). Per project guidelines, these should be ARB-backed and accessed viaS.of(context)(and, for internal errors, consider mapping exceptions to user-friendly, localized copies rather than exposing rawtoString()).Also applies to: 219-273, 275-283
lib/features/chat/notifiers/chat_room_notifier.dart (1)
489-505: Add basic bounds/eviction for decrypted file/image caches.
_imageCache/_fileCacheand their metadata maps grow monotonically for the lifetime of aChatRoomNotifier. With many large images/files in a busy chat, this can accumulate substantial memory.Consider adding at least a simple bound or eviction strategy (e.g., LRU with max count or total bytes per chat room) or clearing caches when navigating away/closing the chat, while keeping the current API surface (
getCached*/cacheDecrypted*) intact.Also applies to: 518-589, 592-598
lib/features/chat/widgets/encrypted_file_message.dart (1)
81-119: Localize all user-facing strings in previews, file cards, and SnackBars.This widget contains several hard-coded English strings (
'Failed to load image','Failed to load file','File not available','Security error: Invalid characters in sanitized filename','Opening ...','Could not open file: ...','Error opening file: ...'). Project guidelines require routing user-visible text through ARB +S.of(context).Recommend:
- Add appropriate keys to your ARB files.
- Replace these literals with
S.of(context)!.<key>(or map internal exception messages to localized, user-friendly summaries before displaying).Also applies to: 186-289, 291-386, 407-422, 451-505
lib/services/encrypted_file_upload_service.dart (1)
61-71: Deduplicate Blossom server configuration and exception type across services.Both
EncryptedFileUploadServiceandEncryptedImageUploadServiceembed their own_blossomServerslist and define aBlossomExceptiontype. This increases the risk of drift (e.g., one server list updated, the other not).Consider:
- Extracting the Blossom server list into a shared config (e.g.,
blossom_config.dartor a shared service) used by both upload services.- Defining
BlossomExceptiononce in a shared module and reusing it instead of redefining it in multiple files.This keeps Blossom behaviour consistent and simplifies future changes.
Also applies to: 163-195, 215-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
lib/features/chat/notifiers/chat_room_notifier.dart(3 hunks)lib/features/chat/utils/message_type_helpers.dart(1 hunks)lib/features/chat/widgets/encrypted_file_message.dart(1 hunks)lib/features/chat/widgets/encrypted_image_message.dart(1 hunks)lib/features/chat/widgets/message_bubble.dart(2 hunks)lib/features/chat/widgets/message_input.dart(5 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)lib/services/blossom_download_service.dart(1 hunks)lib/services/encrypted_file_upload_service.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)lib/services/file_validation_service.dart(1 hunks)lib/services/media_validation_service.dart(1 hunks)pubspec.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/l10n/intl_it.arb
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/chat/utils/message_type_helpers.dartlib/services/blossom_download_service.dartlib/services/file_validation_service.dartlib/features/chat/widgets/encrypted_image_message.dartlib/services/encrypted_file_upload_service.dartlib/features/chat/widgets/message_input.dartlib/features/chat/widgets/encrypted_file_message.dartlib/services/encrypted_image_upload_service.dartlib/services/media_validation_service.dartlib/features/chat/widgets/message_bubble.dartlib/features/chat/notifiers/chat_room_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/chat/utils/message_type_helpers.dartlib/services/blossom_download_service.dartlib/services/file_validation_service.dartlib/features/chat/widgets/encrypted_image_message.dartlib/services/encrypted_file_upload_service.dartlib/features/chat/widgets/message_input.dartlib/features/chat/widgets/encrypted_file_message.dartlib/services/encrypted_image_upload_service.dartlib/services/media_validation_service.dartlib/features/chat/widgets/message_bubble.dartlib/features/chat/notifiers/chat_room_notifier.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/blossom_download_service.dartlib/services/file_validation_service.dartlib/services/encrypted_file_upload_service.dartlib/services/encrypted_image_upload_service.dartlib/services/media_validation_service.dart
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it) - use
S.of(context)!.keyNamefor all user-facing strings
Files:
lib/l10n/intl_en.arblib/l10n/intl_es.arb
lib/features/**/notifiers/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Files:
lib/features/chat/notifiers/chat_room_notifier.dart
🧠 Learnings (15)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/chat/utils/message_type_helpers.dartlib/features/chat/widgets/encrypted_image_message.dartlib/features/chat/widgets/message_bubble.dartlib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/features/chat/utils/message_type_helpers.dartlib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/l10n/**/*.arb : Add new localization keys to all three ARB files (en, es, it) - use `S.of(context)!.keyName` for all user-facing strings
Applied to files:
lib/l10n/intl_en.arb
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/chat/widgets/message_bubble.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-05-06T22:23:06.684Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/features/chat/notifiers/chat_rooms_notifier.dart:0-0
Timestamp: 2025-05-06T22:23:06.684Z
Learning: The `reload()` method in the chat_room_notifier is synchronous, so await is not required or possible.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
🔇 Additional comments (9)
lib/l10n/intl_es.arb (2)
1172-1172: LGTM - Trailing comma addition.The trailing comma on
restoreErrorMessageis a correct JSON formatting change that allows the subsequent entries to be added cleanly.
1174-1178: All file messaging localization keys are correctly added to all three ARB files.The new file messaging keys (download, openFile, encrypted, downloadingFile, and the section comment) have been successfully added to the Italian localization file (
lib/l10n/intl_it.arb) along with the Spanish and English files, meeting the coding guideline requirement.lib/l10n/intl_en.arb (2)
1194-1194: LGTM - Trailing comma addition.The trailing comma on
restoreErrorMessagecorrectly enables the subsequent file messaging entries.
1196-1200: LGTM - File messaging localization keys added.The English file messaging localization keys have been correctly added and the translations are clear and appropriate for the file messaging feature.
Note: Verification of Italian translations was requested in the review of the Spanish localization file.
lib/services/file_validation_service.dart (1)
6-22: LGTM - Well-structured result class.The
FileValidationResultclass provides comprehensive metadata for validated files.lib/features/chat/widgets/message_input.dart (1)
229-260: Good implementation of upload button with loading state.The loading indicator and disabled state during upload provide good UX feedback. The
mountedchecks beforesetStateare properly placed.lib/features/chat/utils/message_type_helpers.dart (1)
5-17: Good extraction of message type utilities.This properly centralizes the message type detection logic that was previously duplicated across files, following the past review suggestion.
lib/features/chat/widgets/message_bubble.dart (1)
8-10: Good refactoring - duplicate helpers consolidated.The message type helper functions have been moved to
message_type_helpers.dartas suggested in past reviews, eliminating code duplication.lib/services/blossom_download_service.dart (1)
110-158: HTTP client lifecycle handled correctly in streaming download.
downloadWithProgressnow creates a singlehttp.Clientand closes it in afinallyblock, ensuring no client leak even when exceptions occur duringsendor stream consumption. The progress callback and error wrapping viaBlossomDownloadExceptionalso look solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
lib/services/media_validation_service.dart (2)
61-65: Consider usingcompute()for CPU-bound image operations.Both
validateAndSanitizeImageandvalidateAndSanitizeImageLightare markedasyncbut perform synchronous CPU-bound operations (decodeImage,encodeJpg,encodePng). For large images, these operations could block the UI thread and cause jank.Consider offloading heavy work to an isolate:
import 'package:flutter/foundation.dart'; static Future<MediaValidationResult> validateAndSanitizeImageLight( Uint8List imageData, ) async { return compute(_sanitizeImageLightIsolate, imageData); } static MediaValidationResult _sanitizeImageLightIsolate(Uint8List imageData) { // Move sanitization logic here }Also applies to: 147-148
220-223: Consider adding dimension limits to prevent memory exhaustion.The service validates file size but not image dimensions. A maliciously crafted image could have small file size but extremely large dimensions (e.g., 50000×50000 pixels), potentially exhausting memory during decode.
static bool isImageTypeSupported(String mimeType) { return mimeType == 'image/jpeg' || mimeType == 'image/png'; } /// Maximum allowed image dimension (width or height) static const int maxImageDimension = 8192;Then add a check after decoding in both sanitization methods:
if (decodedImage.width > maxImageDimension || decodedImage.height > maxImageDimension) { throw MediaValidationException( 'Image dimensions exceed maximum allowed (${maxImageDimension}x$maxImageDimension)' ); }lib/features/chat/widgets/encrypted_file_message.dart (3)
109-115: Localize hard-coded error strings.'Failed to load image' (line 110) and 'Failed to load file' (line 362) should use localized strings via
S.of(context)for consistency with the rest of the UI. As per coding guidelines, all user-facing strings should be localized via ARB files.- Text( - 'Failed to load image', + Text( + S.of(context)!.failedToLoadImage,- Text( - 'Failed to load file', + Text( + S.of(context)!.failedToLoadFile,Also applies to: 362-367
62-71: Auto-download in build() may trigger multiple times.Using
addPostFrameCallbackinsidebuild()to trigger_downloadFile()can cause repeated download attempts if the widget rebuilds frequently. The_isLoadingguard helps, but there's still a brief window where multiple callbacks could be scheduled.Consider moving this to
initStateor using a flag to ensure single execution:+ bool _hasTriggeredAutoDownload = false; + @override Widget build(BuildContext context) { // ... existing code ... if (metadata != null) { // For images, try to download automatically for preview - if (_isImage(metadata.fileType)) { + if (_isImage(metadata.fileType) && !_hasTriggeredAutoDownload) { WidgetsBinding.instance.addPostFrameCallback((_) { if (!_isLoading) { + _hasTriggeredAutoDownload = true; _downloadFile(); } }); }
519-567: Sanitization covers common cases but has limited i18n support.The
_sanitizeFilenamemethod provides solid security but the accent normalization (lines 525-530) only covers Spanish characters. Other accented characters (German umlauts, French accents, etc.) will pass through unchanged, which could cause issues on some filesystems.Consider using a more comprehensive approach or simply replacing all non-ASCII with underscores for maximum compatibility:
// Alternative: Replace all non-ASCII characters String normalized = basename.replaceAll(RegExp(r'[^\x00-\x7F]'), '_');lib/services/file_validation_service.dart (2)
66-68: Cross-platform path handling issue.Using
file.path.split('/').lastwon't correctly extract the filename on Windows, where paths use backslashes. Consider usingpathpackage or a cross-platform approach:+import 'package:path/path.dart' as p; + static Future<FileValidationResult> validateFile(File file) async { - final filename = file.path.split('/').last; + final filename = p.basename(file.path); final fileData = await file.readAsBytes();Alternatively, use a regex that handles both separators:
final filename = file.path.split(RegExp(r'[/\\]')).last;
70-70: Consider sanitizing filename in logs.The log statement includes the raw filename which could contain PII or sensitive information. Consider truncating or hashing the filename for production logs.
-_logger.i('🔍 Validating file: $filename (${fileData.length} bytes)'); +_logger.i('🔍 Validating file: ${filename.length > 20 ? '${filename.substring(0, 20)}...' : filename} (${fileData.length} bytes)');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/chat/widgets/encrypted_file_message.dart(1 hunks)lib/services/file_validation_service.dart(1 hunks)lib/services/media_validation_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/file_validation_service.dartlib/features/chat/widgets/encrypted_file_message.dartlib/services/media_validation_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/file_validation_service.dartlib/features/chat/widgets/encrypted_file_message.dartlib/services/media_validation_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/file_validation_service.dartlib/services/media_validation_service.dart
🧠 Learnings (1)
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/features/chat/widgets/encrypted_file_message.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). (1)
- GitHub Check: build
🔇 Additional comments (9)
lib/services/media_validation_service.dart (3)
1-52: LGTM!The enum, extension, and data class are well-structured. The
SupportedImageTypeenum with its extension provides clean access to MIME types and extensions.
100-115: LGTM – Exception handling fix properly applied.The
on MediaValidationException { rethrow; }block at lines 109-110 correctly prevents double-wrapping ofMediaValidationException, addressing the previous review feedback.
175-191: LGTM – Heavy sanitization exception handling also fixed.The same exception handling pattern is correctly applied here at lines 185-186.
lib/features/chat/widgets/encrypted_file_message.dart (2)
461-471: LGTM – Path traversal vulnerability properly mitigated.The implementation now uses
_sanitizeFilename()at line 463 with defense-in-depth validation at lines 468-471. The_sanitizeFilenamemethod (lines 519-567) comprehensively handles:
- Directory traversal patterns
- Dangerous characters
- Windows reserved names
- Length limits
This addresses the previous security concern.
96-100: LGTM – Color.withValues alpha values corrected.All
withValues(alpha:)calls now use the correct 0.0-1.0 range (e.g.,alpha: 0.2,alpha: 0.7,alpha: 0.098), fixing the previous transparency issue.Also applies to: 133-136, 344-349
lib/services/file_validation_service.dart (4)
31-35: LGTM – Invalid MIME type removed.The
supportedTypesmap now correctly uses onlyimage/jpegandimage/png, removing the non-standardimage/jpgas flagged in the previous review.
291-328: LGTM – Efficient byte pattern search implemented.The macro detection now uses an efficient sliding-window byte pattern search (
_containsPattern) instead of converting the entire file to a string. This addresses the memory efficiency and false-positive concerns from the previous review.
179-201: PDF validation is basic but appropriate for this use case.The PDF validation checks header (
%PDF-) and warns on missing trailer (%%EOF). While this doesn't catch all malformed PDFs, it's reasonable for proof-of-payment exchange validation without adding heavyweight PDF parsing dependencies.
254-289: LGTM – Video structure validation.The video validation correctly checks magic bytes for MP4, MOV, and AVI formats. The
ftypbox check for MP4/MOV and RIFF/AVI header checks are appropriate structural validations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/chat/widgets/message_input.dart (1)
127-136: Avoid exposing raw exception details to users.The error message still exposes raw exception details that could reveal internal information (file paths, network errors, stack traces).
Apply this diff as suggested in the previous review:
} catch (e) { // Show error to user if (mounted) { + // Log the full error for debugging + debugPrint('File upload error: $e'); ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text('Error uploading file: $e'), + content: Text(S.of(context)!.failedToUploadFile), backgroundColor: Colors.red, ), ); } } finally {Note: You'll need to add the
failedToUploadFilelocalization key to your ARB files.
🧹 Nitpick comments (1)
lib/features/chat/widgets/message_input.dart (1)
103-104: Consider optimizing memory usage for large files.Reading the entire file into memory with
readAsBytes()before MIME detection could cause memory pressure for large files (especially videos). Consider reading only the header bytes needed for MIME detection first.- final fileData = await file.readAsBytes(); - final isImage = await _isImageFile(fileData, filename); + // Read only first few KB for MIME detection to avoid loading large files + final headerSize = 8192; // 8KB should be enough for MIME detection + final fileHandle = await file.open(); + final headerBytes = await fileHandle.read(headerSize); + await fileHandle.close(); + final isImage = await _isImageFile(headerBytes, filename);Note: The upload services will still read the full file, but this avoids loading it twice in memory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/chat/widgets/message_input.dart(5 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/l10n/intl_it.arb
- lib/l10n/intl_es.arb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/chat/widgets/message_input.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/chat/widgets/message_input.dart
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it) - use
S.of(context)!.keyNamefor all user-facing strings
Files:
lib/l10n/intl_en.arb
🧠 Learnings (1)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/l10n/**/*.arb : Add new localization keys to all three ARB files (en, es, it) - use `S.of(context)!.keyName` for all user-facing strings
Applied to files:
lib/l10n/intl_en.arb
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
lib/l10n/intl_en.arb (2)
1194-1194: LGTM: Trailing comma addition is correct.The trailing comma added to
restoreErrorMessageis proper JSON formatting for a non-final entry.
1196-1205: Localization keys properly added to all three ARB files.All seven new keys (download, openFile, encrypted, downloadingFile, confirmFileUpload, sendThisFile, send) are correctly present in the English, Spanish, and Italian ARB files. The comment markers (@_comment_file_messaging and @_comment_file_confirmation) are also consistently added across all three locales.
lib/features/chat/widgets/message_input.dart (4)
158-167: MIME detection logic is correct.The approach of checking magic bytes first, then falling back to extension-based detection is solid and follows best practices.
209-285: Well-implemented confirmation dialog.The file upload confirmation provides clear user feedback with proper localization, theming, and null handling. Good UX pattern to confirm before uploading.
313-343: Clean UI implementation with proper loading states.The file attachment button provides good visual feedback during uploads with the loading indicator and properly disables interaction while uploading.
81-87: Thesubstring(1)operation is correct.FileValidationService.getSupportedExtensions()returns extensions with leading dots (e.g.,".pdf",".jpg") from theextensionToMimemap (lines 48–63). Thesubstring(1)call properly removes the dot to match FilePicker's expected format of['jpg', 'pdf', 'png']without dots. No issue exists in this code.Likely an incorrect or invalid review comment.
|
I uploaded a jpeg file and it gets uploaded correctly, the counterpart can see the preview but non of them can download the entire file 
|
ok I'll make sure the image opens properly, because it downloads but only shows a preview. |
|
@grunch voy a hacer en otro PR que los archivos se guarden en memoria mientras dure la sesion (ahora 72 horas) para que no tengan que volver a descargarlos cada vez que abran el chat. Tambien que el user pueda cambiar los servidores blossom que usará porque ahora tengo varios harcodeados, pero serán en otros PRs para no hacer tan grande este, te parece bien? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/features/chat/widgets/encrypted_file_message.dart (5)
15-29: Unused parameterisOwnMessage.The
isOwnMessageparameter is defined but never referenced in the widget implementation. Per coding guidelines, remove unused dependencies to maintain code cleanliness.class EncryptedFileMessage extends ConsumerStatefulWidget { final NostrEvent message; final String orderId; - final bool isOwnMessage; const EncryptedFileMessage({ super.key, required this.message, required this.orderId, - required this.isOwnMessage, });Note: Verify all call sites are updated if you remove this parameter.
116-122: Localize user-facing string.The hard-coded
'Failed to load image'violates coding guidelines requiring localization via ARB files. As per coding guidelines, access user-facing strings withS.of(context)rather than hard-coded literals.Text( - 'Failed to load image', + S.of(context)!.failedToLoadImage, style: TextStyle( color: AppTheme.textSecondary, fontSize: 12, ), ),Add the corresponding key to your ARB files.
368-374: Localize user-facing string.The hard-coded
'Failed to load file'should use localization. As per coding guidelines, access user-facing strings withS.of(context)rather than hard-coded literals.Text( - 'Failed to load file', + S.of(context)!.failedToLoadFile, style: TextStyle( fontSize: 12, color: Colors.red, ),Add the corresponding key to your ARB files.
496-514: Localize user-facing error messages in SnackBars.The hard-coded error messages violate coding guidelines. As per coding guidelines, localize all user-facing strings via ARB files.
ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text('Could not open file: ${result.message}'), + content: Text(S.of(context)!.couldNotOpenFile(result.message)), backgroundColor: Colors.orange, duration: const Duration(seconds: 3), ), ); } } } catch (e) { if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text('Error opening file: $e'), + content: Text(S.of(context)!.errorOpeningFile(e.toString())), backgroundColor: Colors.red, ), );Add corresponding parameterized ARB keys.
528-576: Good security implementation with a note on accent handling.The sanitization properly addresses path traversal concerns from the previous review. The implementation handles:
- Path separator extraction (basename)
- Dangerous character removal
- Length limits
- Windows reserved names
- Fallback for invalid filenames
The accent normalization (lines 534-539) only covers Spanish characters. Consider using a more comprehensive Unicode normalization approach in a future iteration if international filenames are expected:
// Alternative: Use Unicode normalization + ASCII-only filtering // import 'package:diacritic/diacritic.dart'; // or similar // String normalized = removeDiacritics(basename);This is a nice-to-have improvement for broader language support but not blocking for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/chat/widgets/encrypted_file_message.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/chat/widgets/encrypted_file_message.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/chat/widgets/encrypted_file_message.dart
🧠 Learnings (3)
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/features/chat/widgets/encrypted_file_message.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
lib/features/chat/widgets/encrypted_file_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/features/chat/widgets/encrypted_file_message.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). (1)
- GitHub Check: build
🔇 Additional comments (9)
lib/features/chat/widgets/encrypted_file_message.dart (9)
1-13: LGTM!Imports are well-organized and all appear to be necessary for the widget functionality.
31-82: LGTM!The build method properly addresses previous review concerns:
- Null check on
messageIdprevents null assertion crashes.- Race condition fix by setting
_isLoading = truesynchronously before scheduling_downloadFile.The control flow is clear and handles all states appropriately.
84-86: LGTM!Simple and effective type check.
193-296: LGTM!Good use of localized strings (
S.of(context)!.encrypted,S.of(context)!.openFile,S.of(context)!.download) and correctColor.withValuesalpha values in the 0.0–1.0 range.
298-339: LGTM!Properly localized loading message and correct alpha values.
395-412: LGTM!Defensive JSON parsing with null return on failure. Consider adding debug logging in the catch block for troubleshooting, but not critical for this PR.
414-461: LGTM!Proper use of
mountedchecks beforesetStateafter async operations, as per coding guidelines. The download flow correctly handles loading states and errors.
518-520: LGTM!Clean delegation to the service layer.
522-526: LGTM!Simple and effective file size formatting. The unit abbreviations (B, KB, MB) are universally recognized and typically not localized.
- Add 31 tests covering encryption, validation, upload/download - Include edge cases and security validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
test/features/chat/file_messaging_test.dart (10)
16-17: Consider centralizing BlossomException definition.The
hide BlossomExceptionsuggests this exception is defined in multiple places (likely in both upload services and BlossomClient). This creates maintenance overhead and potential inconsistencies.Consider moving
BlossomExceptionto a shared location (e.g.,lib/services/blossom_exceptions.dart) and importing it from there in all services that need it.
27-34: Consider extracting shared key derivation to test helper.This key derivation setup is duplicated from
mostro_service_test.dart(per the comment). Extracting it to a shared test helper (e.g.,test/helpers/key_test_helpers.dart) would reduce duplication and ensure consistency across tests.Example helper:
class KeyTestHelpers { static const validMnemonic = 'leader monkey parrot ring guide accident before fence cannon height naive bean'; static DerivedTestKeys deriveTestKeys() { final keyDerivator = KeyDerivator(Config.keyDerivationPath); // ... derivation logic return DerivedTestKeys(...); } }
88-194: Consider moving EncryptionService unit tests to dedicated test file.These tests comprehensively cover
EncryptionServicestatic methods (shared key derivation, encryption/decryption, nonce generation, blob serialization). However, they belong intest/services/encryption_service_test.dartrather than the file messaging integration test.Keep only the integration-focused tests here (e.g., testing that ChatRoomNotifier correctly uses encryption in the file messaging flow). Move pure EncryptionService unit tests to a dedicated
test/services/encryption_service_test.dartfile for better organization and maintainability.
196-269: Move validation service unit tests to dedicated test files.These tests cover
FileValidationServiceandMediaValidationServicestatic methods. They should be in:
test/services/file_validation_service_test.dart(lines 196-240)test/services/media_validation_service_test.dart(lines 242-269)This file should focus on integration testing: verifying that file messaging correctly uses these validation services, handles validation failures, and propagates errors appropriately.
293-300: Remove or improve weak BlossomClient test.This test creates a real
BlossomClientinstance but only verifies theserverUrlproperty, which is trivial. The comment acknowledges the limitation. Either:
- Remove this test entirely (the URL construction is too simple to warrant testing)
- Or mock
http.Clientto properly test the upload flowThe other tests in this group (lines 272-291, 302-317) properly use mocks and are more valuable.
377-402: Download integrity test duplicates earlier encryption tests.Lines 383-401 repeat the encryption/decryption round-trip test from lines 117-138. Since this is primarily testing
EncryptionServicerather than download integration, consider removing this duplication.
474-500: Use flutter_test exception matchers instead of manual try-catch.Lines 487-499 manually catch exceptions and set a boolean flag. This is less idiomatic than using
expect(..., throwsA(isA<EncryptionException>()))as done elsewhere (e.g., lines 150-158).Apply this pattern:
- bool threwEncryptionException = false; - try { - EncryptionService.decryptChaCha20Poly1305( - key: key, - nonce: encrypted.nonce, - encryptedData: encrypted.encryptedData, - authTag: corruptedAuthTag, - ); - } catch (e) { - threwEncryptionException = e is EncryptionException; - } - - expect(threwEncryptionException, isTrue); + expect( + () => EncryptionService.decryptChaCha20Poly1305( + key: key, + nonce: encrypted.nonce, + encryptedData: encrypted.encryptedData, + authTag: corruptedAuthTag, + ), + throwsA(isA<EncryptionException>()), + );
520-546: Use flutter_test exception matchers instead of manual try-catch.Lines 534-545 use manual exception handling similar to lines 474-500. Use
expect(..., throwsA(isA<ArgumentError>()))for consistency with flutter_test conventions.
453-472: Duplicate test: cross-session decryption already tested.This test duplicates the "wrong key" test from lines 140-159. Both verify that decryption fails with an incorrect key. Consider removing this duplication or consolidating into a single test with clearer naming.
24-549: Add integration test coverage for file messaging flows.This PR introduces file messaging with networking (Blossom uploads/downloads), background work (retries), and end-to-end encryption. Per coding guidelines and learnings, flows touching networking require integration test coverage in
integration_test/.Current unit tests mock all network interactions, which won't catch real-world issues like the download bug reported by grunch (uploaded files not downloadable by counterpart).
Suggested integration test scenarios for
integration_test/file_messaging_integration_test.dart:
- Upload an image to real Blossom server (or test server), verify URL returned
- Download and decrypt a file from Blossom server
- End-to-end flow: User A uploads encrypted file, User B receives message and downloads
- Retry logic when Blossom server is temporarily unavailable
- Error handling for oversized files, unsupported formats
Based on coding guidelines: "Ensure integration test coverage for flows touching networking, background work, or session recovery in
integration_test/"Would you like me to help draft an integration test file for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/features/chat/file_messaging_test.dart(1 hunks)test/mocks.dart(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.darttest/mocks.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.darttest/mocks.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (19)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.darttest/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.darttest/mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.darttest/mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.darttest/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.darttest/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{g.dart,mocks.dart} : Never manually edit generated files (`.g.dart`, `.mocks.dart`) or add individual ignore comments - regenerate with `dart run build_runner build -d` instead
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
test/mocks.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/orders/notifiers/add_order_notifier.dart : Start session timeout cleanup in `submitOrder()` method to prevent orphan sessions when Mostro doesn't respond within 10 seconds
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Use `startSessionTimeoutCleanupForRequestId()` for order creation timeout protection and cancel timer automatically when any Mostro response received
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
test/mocks.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
test/mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
test/mocks.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). (1)
- GitHub Check: build
🔇 Additional comments (3)
test/mocks.dart (2)
22-29: LGTM! Service imports support new file messaging test infrastructure.The imported services (BlossomClient, EncryptionService, upload/download services, validation services, ChatRoomNotifier) correspond to the file messaging feature. Note that EncryptionService, FileValidationService, and MediaValidationService are imported but not added to
@GenerateMocks—this is correct if they are static utility classes.
51-55: Mock generation correctly configured for file messaging services.The new mocks (BlossomClient, EncryptedFileUploadService, EncryptedImageUploadService, BlossomDownloadService, ChatRoomNotifier) enable comprehensive testing of the file messaging feature. These will be generated in
mocks.mocks.dartviadart run build_runner build -d.test/features/chat/file_messaging_test.dart (1)
415-449: Good security test for cross-session isolation.This test verifies that different sessions derive different shared keys, ensuring files encrypted for one session cannot be decrypted by another. This is critical for the security model.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/features/chat/file_messaging_test.dart (6)
62-87:ProviderContaineris created but never used
containeris instantiated and disposed, but not used to read or override any Riverpod providers. This is likely to trigger an “unused variable” analyzer warning and adds noise to the setup.You can either:
- Remove
ProviderContainerentirely for now, or- Wire it to the file-messaging providers (e.g., overriding the
ChatRoomNotifier/ upload/download services) so tests exercise real Riverpod state.This keeps tests focused and avoids analyzer noise, as required by the guidelines.
102-115:shared key used for both text and filestest is tautological with a mockThis test stubs
MockChatRoomNotifier.getSharedKey()to returnexpectedBytesand then asserts thatgetSharedKey()returns those same bytes. It doesn’t touch production code that actually derives or reuses the shared key for text vs. files, so it provides little real coverage.Consider rewriting this test to:
- Exercise the real
ChatRoomNotifier(or whatever orchestrates text/file encryption) againsttestSession.sharedKey, or- Remove the test to avoid a false sense of coverage.
161-176: Nonce uniqueness and wrong-key tests are good but a bit redundant
- The nonce-uniqueness test is useful, but with 100 iterations you still have a (tiny) theoretical chance of random collision. It’s probably fine in practice, but if flakiness ever appears you could reduce the loop count or assert nonce length/format in addition.
prevents cross-session file decryptioneffectively duplicatesfails decryption with wrong shared keyusing different hard-coded keys. To better differentiate it, you could tie it toSession.sharedKeyfor two sessions and show that cross-session decrypt fails, rather than re-testing the same key-mismatch property already covered above.These are non-blocking refinements to keep the suite lean and focused.
Also applies to: 451-471
196-240: Avoid allocating 24–26MB buffers in tests; assert on sizes insteadThe file-size tests currently allocate 24MB, 25MB, and 26MB
Uint8Lists just to compare.lengthagainstFileValidationService.maxFileSize. That adds unnecessary memory pressure in the test runner.You can keep the same assertions using plain integers, e.g.:
- test('accepts files within 25MB limit', () { - final file24MB = Uint8List(24 * 1024 * 1024); - expect(file24MB.length, lessThanOrEqualTo(FileValidationService.maxFileSize)); - - final file25MB = Uint8List(25 * 1024 * 1024); - expect(file25MB.length, lessThanOrEqualTo(FileValidationService.maxFileSize)); - }); + test('accepts files within 25MB limit', () { + final size24MB = 24 * 1024 * 1024; + final size25MB = 25 * 1024 * 1024; + expect(size24MB, lessThanOrEqualTo(FileValidationService.maxFileSize)); + expect(size25MB, lessThanOrEqualTo(FileValidationService.maxFileSize)); + }); @@ - test('rejects oversized files', () { - final file26MB = Uint8List(26 * 1024 * 1024); - expect(file26MB.length, greaterThan(FileValidationService.maxFileSize)); - }); + test('rejects oversized files', () { + final size26MB = 26 * 1024 * 1024; + expect(size26MB, greaterThan(FileValidationService.maxFileSize)); + });Similarly, the zero-byte test currently only checks
length < maxFileSizewithout going through a validation API. OnceFileValidationServiceexposes a “validate size” entry point, it’s worth asserting that behavior directly instead of only comparing raw lengths.
376-381: Tests that only assert mocks are non-null add little valueThe tests:
download service can be mockedimage upload service can be mockedfile upload service can be mockedonly check that the mock instances are non-null and of the expected mock types. Mockito already guarantees that, so these tests don’t verify any production behavior and can inflate perceived coverage.
Consider either:
- Removing these tests, or
- Replacing them with flow tests that assert real orchestration, e.g., a higher-level “file share” API calling into
EncryptedImageUploadService/EncryptedFileUploadService/BlossomDownloadServicewith the right arguments and error handling.This will keep the suite focused on behavior rather than on Mockito’s own guarantees.
Also applies to: 403-412
414-448: Session isolation test could tie more directly into encryption
handles file sharing between different sessionscorrectly asserts that twoSessioninstances derived from different key indices produce differentsharedKeyvalues. That’s a good invariant.To fully validate cross-session isolation, you might extend this by:
- Encrypting data with
session1.sharedKey, and- Asserting that decryption with
session2.sharedKeyfails viaEncryptionService.decryptChaCha20Poly1305.That would bridge the
Sessionmodel to the encryption layer and avoid relying only on the separate raw-key wrong-decryption tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/features/chat/file_messaging_test.dart(1 hunks)test/mocks.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/mocks.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (8)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (3)
test/features/chat/file_messaging_test.dart (3)
24-52: Key derivation and Mockito dummy setup look solidUsing a NIP-06-style mnemonic with
KeyDerivatorplusprovideDummy<T>registrations gives realistic keys and proper Mockito fallbacks forSession, Blossom services, andChatRoomNotifier. No changes needed here.
473-545: Security edge-case coverage looks strongThe tests around corrupted auth tags, malformed blobs, and invalid key lengths (
handles corrupted encryption data,handles malformed blob data,validates key format and length) provide good defensive coverage of the crypto API’s failure modes and error types. The manualtry/catchchecks for specific exception classes are clear and adequate here.
550-576: Helper image generators are appropriate and minimalThe
_createTestJpegand_createTestPnghelpers use compact, header-only byte sequences to identify JPEG/PNG signatures, which keeps tests fast and avoids bundling real fixture files. This is a good trade-off for the current assertions that only inspect magic bytes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/features/chat/file_messaging_test.dart (1)
300-315: UseexpectLaterfor async error handling.The synchronous
expect(() => ..., throwsA(...))pattern doesn't properly await the Future. WhenuploadImagethrows asynchronously, this test may pass incorrectly or produce misleading results.This was flagged in a previous review. Apply the suggested fix:
- test('handles upload error responses', () async { + test('handles upload error responses', () async { const mimeType = 'image/jpeg'; when(mockBlossomClient.uploadImage( imageData: anyNamed('imageData'), mimeType: mimeType, )).thenThrow(BlossomException('Upload failed: 413 - File too large')); - expect( - () => mockBlossomClient.uploadImage( + await expectLater( + mockBlossomClient.uploadImage( imageData: Uint8List.fromList([1, 2, 3, 4, 5]), mimeType: mimeType, ), throwsA(isA<BlossomException>()), ); });
🧹 Nitpick comments (8)
test/features/chat/file_messaging_test.dart (8)
16-17: Clarify whichBlossomExceptionis used in tests.Both imports hide
BlossomException, but line 306 throwsBlossomExceptionfromblossom_client.dart. This works but thehidedirectives suggest potential naming conflicts between the services. Verify this is intentional and consider documenting which exception class is being used.
102-115: Test verifies mock stub, not production behavior.This test stubs
mockChatRoomNotifier.getSharedKey()then immediately calls it and asserts the stubbed value. This only confirms Mockito works—it doesn't validate that the shared key is actually used for both text and file encryption in production code.Consider either:
- Removing this test (the encryption round-trip tests already validate key usage)
- Testing the actual
ChatRoomNotifier.getSharedKey()implementation
197-208: Tests don't exercise actual validation methods.These tests create
Uint8Listarrays and compare their lengths againstFileValidationService.maxFileSize, but they never call the actual validation method (e.g.,FileValidationService.validateFile()). If the service has validation logic beyond size checks, it's untested.Consider calling the actual validation methods:
test('accepts files within 25MB limit', () { final file24MB = Uint8List(24 * 1024 * 1024); - expect(file24MB.length, lessThanOrEqualTo(FileValidationService.maxFileSize)); + final result = FileValidationService.validateFileSize(file24MB); + expect(result.isValid, isTrue); final file25MB = Uint8List(25 * 1024 * 1024); - expect(file25MB.length, lessThanOrEqualTo(FileValidationService.maxFileSize)); + final result25 = FileValidationService.validateFileSize(file25MB); + expect(result25.isValid, isTrue); }); test('rejects oversized files', () { final file26MB = Uint8List(26 * 1024 * 1024); - expect(file26MB.length, greaterThan(FileValidationService.maxFileSize)); + final result = FileValidationService.validateFileSize(file26MB); + expect(result.isValid, isFalse); });
253-268: Tests validate helper functions, not MediaValidationService.These tests verify that
_createTestJpeg()and_createTestPng()produce correct magic bytes, but they don't exercise anyMediaValidationServicemethods. If the service has image validation logic (e.g.,validateImageFormat()), it remains untested.
375-399: Trivial mock check and duplicate encryption test.
- Lines 376-379: Testing that
mockDownloadServiceis not null and is aMockBlossomDownloadServiceprovides no value—Mockito always returns valid mocks.- Lines 381-399: This test is nearly identical to
'encrypts and decrypts files correctly'at lines 117-138.Consider removing the mock existence check and either consolidating the duplicate encryption tests or differentiating this one to test actual download+decrypt flow.
403-411: Remove trivial mock existence checks.These tests only verify that mock objects exist and have the correct type. Mockito guarantees this—these tests cannot fail and provide no coverage value.
- test('image upload service can be mocked', () { - expect(mockImageUploadService, isNotNull); - expect(mockImageUploadService, isA<MockEncryptedImageUploadService>()); - }); - - test('file upload service can be mocked', () { - expect(mockFileUploadService, isNotNull); - expect(mockFileUploadService, isA<MockEncryptedFileUploadService>()); - });
500-507: Test validates string inequality, not session isolation.This test creates two string constants and asserts they're not equal, then checks
testSession.orderIdagainst those constants. This doesn't test any session isolation logic—it confirms that different strings are different.Either remove this test or make it meaningful by testing actual session isolation behavior (e.g., that a session cannot access data from another session's orderId).
53-86: Consider consolidating test setup with provider overrides.The
ProviderContainer(line 81) is created and disposed but never used for provider overrides. The tests use mocks directly instead of through Riverpod's override mechanism. This is fine for unit testing encryption utilities, but if you want to test Riverpod integration (e.g., notifier behavior), consider:container = ProviderContainer( overrides: [ blossomClientProvider.overrideWithValue(mockBlossomClient), // ... other overrides ], );As per coding guidelines, Riverpod should be used for state management with business logic in Notifiers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/features/chat/file_messaging_test.dart(1 hunks)test/mocks.mocks.dart(36 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
**/*.{g.dart,mocks.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit generated files (
.g.dart,.mocks.dart) or add individual ignore comments - regenerate withdart run build_runner build -dinstead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually modify
test/mocks.mocks.dart- it already has file-level// ignore_for_file: must_be_immutableignore directives
Files:
test/mocks.mocks.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (28)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{g.dart,mocks.dart} : Never manually edit generated files (`.g.dart`, `.mocks.dart`) or add individual ignore comments - regenerate with `dart run build_runner build -d` instead
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/mocks.mocks.darttest/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Track relay sources (user, mostro, default) using RelaySource enum for appropriate handling and storage strategy
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/models/relay_list_event.dart : Parse NIP-65 (kind 10002) events with robust handling of different timestamp formats and null-safe parsing for malformed events
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/data/repositories/**/*.dart : Implement Repository pattern for all data access - all data operations must go through repository classes
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Use `removeRelayWithBlacklist()` for Mostro/default relays (adds to blacklist) and `removeRelay()` for permanent user relay deletion
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement relay URL normalization by removing trailing slashes in all blacklist operations to ensure consistent matching
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/mostro_fsm.dart : Use MostroFSM for managing order state transitions - all state changes must go through FSM state methods
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (1)
test/features/chat/file_messaging_test.dart (1)
549-575: LGTM!The helper functions create properly structured minimal JPEG and PNG test data with correct magic bytes and headers. These are useful for testing image format validation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/features/chat/file_messaging_test.dart (1)
270-315: Blossom upload tests still operate on mocks only, and error test uses a non-async-safe pattern
delegates to BlossomClient uploadImage methodstubs and callsMockBlossomClient.uploadImagedirectly, so it only verifies Mockito behavior, not that any higher-level upload service or UI flow delegates correctly.exposes configured server URLis fine but very shallow.handles upload error responsesstubsuploadImageto throw and then usesexpect(() => mockBlossomClient.uploadImage(...), throwsA(...))on aFuture<String>API, which is brittle and doesn’t assert on how your code under test reacts to failed uploads.To make these tests more meaningful and future-proof:
- Test a real component that depends on
BlossomClient(e.g.,EncryptedImageUploadServiceor the file-messaging flow) by injectingmockBlossomClientand asserting it callsuploadImagewith the correct bytes/MIME and propagates or wrapsBlossomException.- For the error case, prefer stubbing the async failure and asserting on the returned
Future:- test('handles upload error responses', () async { + test('handles upload error responses', () async { @@ - when(mockBlossomClient.uploadImage( - imageData: anyNamed('imageData'), - mimeType: mimeType, - )).thenThrow(BlossomException('Upload failed: 413 - File too large')); - - expect( - () => mockBlossomClient.uploadImage( - imageData: Uint8List.fromList([1, 2, 3, 4, 5]), - mimeType: mimeType, - ), - throwsA(isA<BlossomException>()), - ); + when( + mockBlossomClient.uploadImage( + imageData: anyNamed('imageData'), + mimeType: mimeType, + ), + ).thenAnswer( + (_) async => throw BlossomException('Upload failed: 413 - File too large'), + ); + + await expectLater( + mockBlossomClient.uploadImage( + imageData: Uint8List.fromList([1, 2, 3, 4, 5]), + mimeType: mimeType, + ), + throwsA(isA<BlossomException>()), + ); });This both uses Mockito’s async stubbing conventions and checks the
Futurebehavior directly instead of relying on synchronous throws.To confirm behavior after refactoring, run:
#!/bin/bash # Run only this suite while iterating on the Blossom tests flutter test test/features/chat/file_messaging_test.dart
🧹 Nitpick comments (6)
test/features/chat/file_messaging_test.dart (6)
101-114:shared key used for both text and filestest only validates a Mockito stubThis test stubs
MockChatRoomNotifier.getSharedKey()and then asserts that the stubbed value is returned, without exercising realChatRoomNotifieror any text/file pipeline. To actually prove that the same shared key is used for both text and file encryption, consider:
- Instantiating a real
ChatRoomNotifier(via its provider andProviderContainer) withtestSession, and- Asserting that
getSharedKey()(and any file-encryption entrypoint) derive the same bytes fromtestSession.sharedKey.
195-239: File-size tests assert on constants rather than validation behaviorThe size tests currently only compare
Uint8List.lengthagainstFileValidationService.maxFileSize. They don’t call any actual validation API, so they’ll still pass if validation logic is broken but the constant remains unchanged. IfFileValidationServiceexposes methods likevalidateFileSizeor a higher-levelvalidateFile(...), consider invoking those here and asserting success/failure outcomes instead of (or in addition to) the raw length comparisons.
241-268: Media validation tests only assert on handcrafted headers, not service logic
_createTestJpeg/_createTestPngand the associated tests validate the synthetic headers themselves (length and magic bytes) but don’t exerciseMediaValidationServiceor any real image validation behavior. To better cover the media validation path, consider:
- Adding tests that feed these buffers into the real media/file-validation APIs and assert acceptance or rejection per MIME/extension, and
- Keeping the header checks as helper assertions if you want, but making them secondary to the production API tests.
Also applies to: 573-599
399-424: Download/decrypt tests are correct but partially redundant
verifies file integrity after decryptionessentially duplicates the earlier “encrypts and decrypts files correctly” test with a shorter payload, whiledownload service can be mockedonly asserts that the mock exists. Consider:
- Dropping or strengthening the “can be mocked” test by wiring a real
BlossomDownloadServicethat depends onMockBlossomClient/ HTTP stubs, and- Either merging the integrity check with the main encryption test or tweaking it to model a more realistic downloaded blob path (e.g., using
EncryptionResult.toBlob/fromBlob).
426-471: “Can be mocked” upload service tests provide little valueThe
image upload service can be mockedandfile upload service can be mockedtests only assert that the mocks are non-null and of the right type. They don’t exercise any upload flow, encryption, or messaging behavior, so they are unlikely to catch regressions.If you keep this group, consider replacing them with an end-to-end style test that:
- Uses
MockEncryptedFileUploadService/MockEncryptedImageUploadServiceinjected into the chat/file flow, and- Asserts that sending a file/image triggers the appropriate encrypted upload call and produces the expected gift-wrap metadata.
This would align better with the “Complete File Sharing Flows” group name.
474-569: Security & edge-case coverage is strong, but some tests are misnamed / low-signalPositives:
- Cross-key decryption failure, corrupted auth-tag handling, malformed blobs, and key-length validation all give solid safety coverage around
EncryptionServiceandEncryptionResult.- The explicit check for
EncryptionExceptionon corrupted tags is especially useful.Less strong:
validates session isolationonly compares hard-coded order IDs andtestSession.orderId, not actual session-related cryptographic state or isolation guarantees. The name overpromises.If you want this group to focus on isolation, consider adjusting that test to:
- Create two distinct
Sessions and assert that blobs encrypted with one session’s shared key cannot be decrypted with the other (similar to your other cross-session tests), or- Assert on whatever session-bound state actually governs decryption access in
ChatRoomNotifier/ related services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/chat/file_messaging_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (9)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (4)
test/features/chat/file_messaging_test.dart (4)
23-51: Test bootstrap and key derivation match production patternsUsing
TestWidgetsFlutterBinding.ensureInitialized(), NIP-06-style mnemonic derivation, andprovideDummyforSessionand services mirrors the app’s setup and should keep tests aligned with real key material and non-nullable mocks. No issues here.
116-193: Encryption tests give solid coverage of ChaCha20-Poly1305 behaviorThese tests nicely cover round-trip encryption/decryption, nonce/auth-tag lengths, failure on wrong keys, nonce uniqueness, and blob
toBlob/fromBlobserialization. AssumingEncryptionServiceis the single entrypoint for ChaCha20-Poly1305, this is good defensive coverage for the new encrypted file format.
317-397: Gift-wrap JSON tests now exercise real result typesThese tests now construct
EncryptedFileUploadResult/EncryptedImageUploadResultand assert on thetoJson()payload, including type tags, sizes, dimensions, and absence of raw key fields. This addresses the prior issue where tests only checked hand-built maps, and it gives good regression coverage for the NIP-59 metadata structure.
573-599: Helper image generators are fine and self-containedThe minimal JPEG/PNG helpers accurately encode the expected magic numbers and header structure for 1×1 images. They’re appropriate as test fixtures and can be reused if you later add real media-validation or thumbnailing tests.
|
Hi @Catrya did you applied nitpick rabbit suggestions? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/features/chat/file_messaging_test.dart (1)
441-486: Blossom upload tests now reflect behavior; consider deeper HTTP/path coverageRenaming to
delegates to BlossomClient uploadImage method/exposes configured server URLand explicitly checking the delegated call andserverUrlmakes the tests honest about what they cover, andhandles upload error responsescorrectly asserts aBlossomException.If you later want to verify HTTP headers/status handling in
BlossomClientitself, you’ll likely need to:
- Inject an
http.ClientintoBlossomClient, and- Assert on the constructed request and response handling,
or complement these unit tests with an integration test that runs the full upload path under
integration_test/, as suggested in earlier feedback and project guidelines for networking-heavy flows. Based on learnings, integration tests are recommended for such paths.
🧹 Nitpick comments (4)
test/features/chat/file_messaging_test.dart (4)
64-100: ProviderContainer override is currently unused in tests
ProviderContainerwith thesessionNotifierProvideroverride andMockSessionNotifierisn’t read or used anywhere in this file. That makes the setup a bit noisy and potentially confusing.Either:
- Add at least one test that exercises provider-based behavior (e.g.,
ChatRoomNotifier/session-dependent flows) through this container, or- Drop the container + override from
setUpto keep the test harness minimal.
230-274: Simplify size-limit tests and avoid large allocations
accepts files within 25MB limit,rejects oversized files, andhandles zero-byte filesallocate 24–26 MBUint8Lists but only assert onlengthvsFileValidationService.maxFileSize.You can get the same signal with simpler, cheaper assertions, e.g.:
test('uses 25MB max file size', () { expect(FileValidationService.maxFileSize, equals(25 * 1024 * 1024)); });and keep zero-byte checks if you want, without large buffers. That makes intent clearer and reduces memory use in tests.
570-606: Mock-existence tests provide little signal—consider tightening or removing
download service can be mocked,image upload service can be mocked, andfile upload service can be mockedonly assert that the mocks are non-null/expected types, which is almost guaranteed by the setup.Either:
- Extend these to assert on real interactions (e.g., that upload/download services call BlossomClient and EncryptionService as expected), or
- Drop them to avoid low-value tests.
667-739: Prefer matcher-based exception assertions over manual flagsIn
handles corrupted encryption dataandvalidates key format and length, you manually flip booleans inside try/catch to assert exceptions:bool threwEncryptionException = false; try { EncryptionService.decryptChaCha20Poly1305(...); } catch (e) { threwEncryptionException = e is EncryptionException; } expect(threwEncryptionException, isTrue);This can be simplified and made more expressive with
expect+throwsA, e.g.:expect( () => EncryptionService.decryptChaCha20Poly1305( key: key, nonce: encrypted.nonce, encryptedData: encrypted.encryptedData, authTag: corruptedAuthTag, ), throwsA(isA<EncryptionException>()), ); expect( () => EncryptionService.encryptChaCha20Poly1305( key: invalidKey, plaintext: testData, ), throwsArgumentError, );This reduces boilerplate and yields clearer failure messages when expectations aren’t met.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/chat/file_messaging_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (9)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (6)
test/features/chat/file_messaging_test.dart (6)
27-55: Solid deterministic key setup and Mockito dummy configurationUsing a fixed mnemonic +
KeyDerivatorandprovideDummyforSessionand Blossom-related services gives you stable, realistic crypto context for all tests. This harness looks coherent and matches the rest of the codebase patterns.
102-228: Encryption & key-management coverage looks strongThese tests cover shared-key symmetry, using the same shared key for text/files, ChaCha20-Poly1305 round-trips, wrong-key failures, nonce uniqueness, and blob encoding/decoding. This gives very good safety nets around the crypto primitives and serialization.
275-342: Good API-level file validation testsThe JPEG/PNG/oversized/unsupported-type API tests exercise
FileValidationService.validateFileend to end with realFileinstances and expected exceptions. This is the right level for catching regressions in the validation pipeline.
344-439: MediaValidationService tests are comprehensive and realisticThe combination of header-based checks, “real” 10x10 JPEG/PNG generation using the
imagepackage, and both light/heavy sanitization paths gives strong coverage of media validation behavior, including invalid/empty data and MIME-type support. Nicely done.
488-568: Gift-wrap tests now exercise production JSON structureSwitching to
EncryptedFileUploadResult/EncryptedImageUploadResultand asserting ontoJson()output (types, sizes, dimensions, absence ofencryption_key/shared_key, etc.) directly tests the real gift-wrap message format rather than hand-crafted maps. This closes the earlier gap and should catch breaking changes in the serialized structure.
608-642: Nice explicit session-isolation check for shared keys
handles file sharing between different sessionsderives separate trade/peer keys and asserts thatsession1.sharedKey!.privatediffers fromsession2.sharedKey!.private. This is a clear, valuable assertion about session isolation and will guard against accidental key reuse across orders.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/features/chat/file_messaging_test.dart (1)
401-446: Blossom upload tests remain unchanged from previous review.The concerns raised in the previous review about these tests still apply:
- Test at lines 402-421 only verifies the mock was called once; it doesn't assert on HTTP headers or authorization behavior
- Test at lines 423-428 only checks the
serverUrlproperty, not upload behavior- Test at lines 430-445 correctly uses mock setup but doesn't validate the HTTP error response details
The previous review suggested either:
- Renaming tests to match what they actually verify (e.g., "delegates to BlossomClient.uploadImage")
- Extending
BlossomClientto accept injectablehttp.Clientfor deeper HTTP testingAdditionally, as noted in the previous review, flows involving networking should ideally have integration test coverage under
integration_test/per project guidelines. Based on learnings, integration tests are recommended for networking-heavy flows.
🧹 Nitpick comments (1)
test/features/chat/file_messaging_test.dart (1)
16-17: Consider centralizingBlossomExceptionto avoid multiple definitions.The
hide BlossomExceptiondirectives suggest this exception type is defined in bothencrypted_file_upload_service.dartandencrypted_image_upload_service.dart. This duplication creates maintenance overhead and potential inconsistencies.Consider moving
BlossomExceptionto a shared location (e.g.,lib/services/blossom_client.dartor a dedicated exceptions file) and having both upload services import it from there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/chat/file_messaging_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
🧠 Learnings (9)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (6)
test/features/chat/file_messaging_test.dart (6)
73-199: Excellent encryption test coverage!This test group provides comprehensive coverage of the encryption system:
- Validates shared key derivation correctness between parties
- Confirms the same shared key works for both text and file encryption
- Tests ChaCha20-Poly1305 encryption/decryption round-trips
- Ensures decryption fails with wrong keys (security critical)
- Verifies nonce uniqueness across encryptions (replay attack prevention)
- Validates blob serialization format
201-302: Thorough file validation test coverage with proper I/O cleanup.The tests comprehensively cover:
- Size limit enforcement (25MB)
- Extension and MIME type validation
- Real file I/O with proper temp directory cleanup
- Both supported and unsupported file types
- Edge cases like zero-byte and oversized files
304-399: Comprehensive media validation test coverage.The tests validate:
- Supported image type definitions
- JPEG and PNG header format requirements
- API-based validation with real image data
- Both light and heavy sanitization paths
- Proper error handling for invalid and empty data
448-528: Excellent improvement! Tests now use production code instead of hand-crafted maps.These tests have been significantly improved since the last review. They now:
- Use actual
EncryptedFileUploadResultandEncryptedImageUploadResultinstances- Test the real
toJson()serialization logic- Validate proper message type discrimination (
file_encryptedvsimage_encrypted)- Ensure encryption keys are not exposed in the JSON output
- Verify all required metadata fields are present
This provides much more valuable test coverage than the previous hand-crafted map approach.
552-677: Strong security and edge case test coverage.These test groups provide excellent coverage of critical security concerns:
- Session isolation: Verifies different sessions derive different shared keys
- Cross-session prevention: Ensures files encrypted in one session can't be decrypted in another
- Data integrity: Validates that corrupted auth tags are detected and rejected
- Input validation: Tests malformed blob handling and key length requirements
This comprehensive security testing helps ensure the encryption system is robust against common attack vectors.
681-726: Helper function documentation is now accurate and clear.The doc comments have been improved since the last review:
_createTestJpeg()and_createTestPng()are correctly described as creating "minimal byte sequences with valid header markers for header validation tests"_createRealJpeg()and_createRealPng()are clearly marked as creating "real valid images for MediaValidationService API tests"The distinction between minimal header-only fixtures and full images is now crystal clear, which will help future maintainers understand the test structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/services/encrypted_image_upload_service.dart (2)
65-75: Same note as file service: dedupe Blossom server list/retry helperThis service repeats the same
_blossomServerslist and_uploadWithRetryimplementation as the file service; centralizing this in a shared helper or config will make future server list changes less error‑prone.Also applies to: 181-224
150-179:nonceHexparameter is unused in decryption and should be removed or validated
downloadAndDecryptImageacceptsnonceHexbut only logs it; actual decryption usesdecryptFromBlob, which extracts the nonce from the blob. This mirrors the earlier situation you already fixed inEncryptedFileUploadService.To avoid a misleading API surface, either:
- Drop
nonceHexfrom the signature and all callers (preferred, consistent with file service), or- Parse the blob first and assert that its nonce matches
nonceHex, throwing if it doesn’t.Right now it’s dead weight and may confuse future callers.
test/features/chat/file_messaging_test.dart (1)
401-446: Blossom upload tests still don’t match names and use a non‑idiomatic async error pattern
delegates to BlossomClient uploadImage methodis fine as a delegation test.exposes configured server URLonly asserts the stored URL, not any upload behavior (name suggests more).handles upload error responsesstubsuploadImageon aFuture<String>API but usesexpect(() => mockBlossomClient.uploadImage(...), throwsA(...)), which treats the failure as a synchronous throw rather than a failingFuture. The more idiomatic pattern inpackage:testis to assert on the returned Future itself, e.g.:when(mockBlossomClient.uploadImage( imageData: anyNamed('imageData'), mimeType: mimeType, )).thenThrow(BlossomException('Upload failed: 413 - File too large')); await expectLater( mockBlossomClient.uploadImage( imageData: Uint8List.fromList([1, 2, 3, 4, 5]), mimeType: mimeType, ), throwsA(isA<BlossomException>()), );You might also rename the tests if you decide not to inspect HTTP/auth details here.
In Dart's `package:test`, what is the recommended pattern for asserting that an async function (returning `Future<T>`) throws—`expect(() => asyncFn(), throwsA(...))` with an async closure, or `await expectLater(asyncFn(), throwsA(...))`? Provide guidance specifically for mocked methods returning `Future`s.
🧹 Nitpick comments (1)
lib/services/encrypted_file_upload_service.dart (1)
60-71: Consider centralizing Blossom server config and retry logic
_blossomServersand_uploadWithRetryhere are effectively identical to the image service; in a follow‑up it would be cleaner to share a single configurable Blossom host list + upload helper to avoid drift when servers change.Also applies to: 163-212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/services/encrypted_file_upload_service.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)test/features/chat/file_messaging_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
test/features/chat/file_messaging_test.dartlib/services/encrypted_image_upload_service.dartlib/services/encrypted_file_upload_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
test/features/chat/file_messaging_test.dartlib/services/encrypted_image_upload_service.dartlib/services/encrypted_file_upload_service.dart
**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.dart: Tests should mirror the feature layout undertest/with the*_test.dartsuffix; end-to-end flows should run fromintegration_test/
Place unit tests beside their feature counterparts using the*_test.dartsuffix and prefer Mockito fakes generated viabuild_runner
Files:
test/features/chat/file_messaging_test.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/encrypted_image_upload_service.dartlib/services/encrypted_file_upload_service.dart
🧠 Learnings (9)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.{dart,flutter} : Run `flutter test` after any code change - Mandatory before commits to ensure all unit tests pass
Applied to files:
test/features/chat/file_messaging_test.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/features/chat/file_messaging_test.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). (1)
- GitHub Check: build
🔇 Additional comments (8)
lib/services/encrypted_file_upload_service.dart (2)
10-55: EncryptedFileUploadResult JSON shape looks solidFields and
toJson/fromJsonare consistent and carry the right metadata for NIP‑59 file messages; no changes needed here.
75-161: End‑to‑end upload/download flow is correct and side‑effect safeFile validation → ChaCha20‑Poly1305 encryption → blob upload → download +
decryptFromBloball line up cleanly, with errors logged and rethrown; this looks good and matches the intended blob format.lib/services/encrypted_image_upload_service.dart (2)
10-59: EncryptedImageUploadResult structure is appropriateModel cleanly captures Blossom URL, nonce, image dimensions, sizes, and MIME type with consistent JSON mapping for NIP‑59; looks good.
79-148: Image upload pipeline is well‑structuredRead → validate/sanitize (light) → encrypt → upload with retry → filename generation is straightforward and matches the media validation tests; no issues spotted here.
test/features/chat/file_messaging_test.dart (4)
73-199: Strong coverage of encryption and key‑management behaviorThese tests exercise shared‑key derivation, AEAD correctness, nonce uniqueness, blob round‑tripping, and key‑length validation; this gives good confidence in the ChaCha20‑Poly1305 plumbing.
201-399: File/media validation tests look thorough and realisticYou cover supported types, zero‑byte and oversized files, header‑only fixtures plus real encoded JPEG/PNG images via
MediaValidationService, and the helper comments now match their implementations. Nicely comprehensive.
448-528: Gift‑wrap JSON tests now exercise real modelsThese tests construct
EncryptedFileUploadResult/EncryptedImageUploadResultand assert on theirtoJson()output, which directly validates the production metadata shape and fixes the earlier issue of only testing hand‑crafted maps.
552-676: Security and cross‑session edge‑case coverage is solidThe tests for cross‑session key mismatch, corrupted auth tags, malformed blobs, and key‑length validation provide good defense‑in‑depth checks around the crypto and session model.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/services/encrypted_file_upload_service.dart (2)
11-56: Tighten JSON typing and make the result model constructorconst.The model looks good and matches the NIP‑59 payload needs, but you can make it a bit safer and more in line with the “use
constwhere possible” guideline:
- Add a
constconstructor (call sites don’t need to change, but you gain the option of const instances where applicable).- Add explicit casts in
fromJsonto avoid silent type drift and give clearer failures if the JSON shape is wrong (e.g.numvsint, missing keys).Example refactor:
- EncryptedFileUploadResult({ + const EncryptedFileUploadResult({ required this.blossomUrl, required this.nonce, required this.mimeType, required this.fileType, required this.originalSize, required this.filename, required this.encryptedSize, }); @@ /// Create from JSON (for receiving messages) factory EncryptedFileUploadResult.fromJson(Map<String, dynamic> json) { return EncryptedFileUploadResult( - blossomUrl: json['blossom_url'], - nonce: json['nonce'], - mimeType: json['mime_type'], - fileType: json['file_type'], - originalSize: json['original_size'], - filename: json['filename'], - encryptedSize: json['encrypted_size'], + blossomUrl: json['blossom_url'] as String, + nonce: json['nonce'] as String, + mimeType: json['mime_type'] as String, + fileType: json['file_type'] as String, + originalSize: (json['original_size'] as num).toInt(), + filename: json['filename'] as String, + encryptedSize: (json['encrypted_size'] as num).toInt(), ); }This keeps behavior the same for well‑formed payloads but makes deserialization failures more predictable and easier to debug. As per coding guidelines, this also leverages
constconstructors where possible.
64-120: Capture and log stack traces for upload/download failures.The overall flow in
uploadEncryptedFileanddownloadAndDecryptFilelooks solid (validate/encrypt/upload and download/decrypt), and the updated nonce handling indownloadAndDecryptFilenow clearly matches the blob format, addressing the earlier unused-nonceHexconcern.Right now both methods do:
} catch (e) { _logger.e('❌ ... failed: $e'); rethrow; }which logs only the error message and discards the stack trace in logs. For crypto and network issues, the stack trace is usually critical when diagnosing failures in the field.
Consider:
- Using
catch (e, stackTrace)in both methods.- Passing both
eandstackTraceto_logger.e(...)according to theloggerpackage version you’re on (either positional or namederror/stackTraceparameters, depending on API).This keeps the
rethrowbehavior (so upstream callers still see the original exception) but makes production debugging a lot easier.Also applies to: 122-150
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/services/encrypted_file_upload_service.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)lib/services/image_upload_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/encrypted_file_upload_service.dartlib/services/encrypted_image_upload_service.dartlib/services/image_upload_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/encrypted_file_upload_service.dartlib/services/encrypted_image_upload_service.dartlib/services/image_upload_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/encrypted_file_upload_service.dartlib/services/encrypted_image_upload_service.dartlib/services/image_upload_service.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). (1)
- GitHub Check: build
🔇 Additional comments (4)
lib/services/encrypted_image_upload_service.dart (2)
68-137: LGTM: Comprehensive encrypted upload flow.The encryption and upload logic is well-structured with proper sanitization, encryption, and metadata capture. The auto-generated filename fallback on line 113-114 is a good defensive practice.
209-212: LGTM: Clean hex conversion implementation.The
_bytesToHexhelper correctly converts bytes to a zero-padded hex string.lib/services/image_upload_service.dart (1)
1-7: No action needed.BlossomExceptionis properly available through the existing import ofblossom_client.darton line 5. In Dart, all public classes defined in an imported module are automatically accessible without requiring a separate import statement.lib/services/encrypted_file_upload_service.dart (1)
152-184: The review comment is incorrect.BlossomExceptionis already defined inlib/services/blossom_client.dart(line 86) and properly imported in the file viaimport 'package:mostro_mobile/services/blossom_client.dart'. No action is needed; the analyzer will not flag this as an undefined identifier.Likely an incorrect or invalid review comment.
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/features/chat/notifiers/chat_room_notifier.dart (1)
151-166: Remove the blockingawait _processMessageContent(chat)call to improve message appearance latency.The
awaitat line 154 blocks the state update while downloading and decrypting images, delaying when the message appears in the chat list. Since theEncryptedImageMessagewidget already has a fallback_loadImage()method that downloads images on-demand and displays a loading placeholder, the pre-download in the notifier is redundant. Move this to a non-blocking operation usingunawaited()orFuture.microtask()(patterns already used inabstract_mostro_notifier.dartandrelays_notifier.dart) to allow the message to appear immediately with a loading indicator for the image.
♻️ Duplicate comments (5)
lib/services/encrypted_image_upload_service.dart (2)
47-62: Previous feedback addressed.The
fromJsonfactory now includes proper try/catch with a descriptiveFormatException. This addresses the prior review comment about null safety checks.
142-169: Previous feedback addressed.The
downloadAndDecryptImagemethod no longer has the unusednonceHexparameter. The nonce is correctly extracted from the blob viadecryptFromBlob.lib/services/image_upload_service.dart (1)
50-53: Previous feedback addressed.The
_uploadWithRetrymethod now correctly delegates toBlossomUploadHelper.uploadWithRetry, eliminating the code duplication that was previously flagged.lib/features/chat/widgets/encrypted_image_message.dart (1)
33-59: Previous feedback addressed - proper lifecycle handling.The widget now correctly triggers image loading in
initStateviaaddPostFrameCallbackinstead of frombuild(). Themountedchecks are properly placed beforesetStatecalls, and null message ID is handled defensively.lib/features/chat/notifiers/chat_room_notifier.dart (1)
385-413: Previous feedback addressed - hex key validation added.The
getSharedKeymethod now validates hex key length (line 395-397) and wraps the parsing in try/catch with descriptive error messages (lines 402-410).
🧹 Nitpick comments (8)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md (2)
31-45: Add language specifier to fenced code block.The ASCII diagram code block is missing a language specifier. Use
textorplaintextto satisfy markdownlint (MD040).-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐
145-149: Add language specifier to fenced code block.Similar to the architecture diagram above, this blob structure diagram needs a language specifier.
-``` +```text ┌─────────────┬──────────────────┬─────────────────┐lib/services/encrypted_image_upload_service.dart (1)
66-69: Minor: Remove extra blank line.There's an extra blank line after the Logger declaration.
class EncryptedImageUploadService { final Logger _logger = Logger(); - - + EncryptedImageUploadService();lib/services/image_upload_service.dart (1)
7-11: Minor: Remove extra blank line.Same pattern as the encrypted service - extra blank line after Logger declaration.
class ImageUploadService { final Logger _logger = Logger(); - - + ImageUploadService();lib/services/blossom_upload_helper.dart (1)
55-58: Unreachable code - deadthrowstatement.Line 57 is unreachable. When
i == servers.length - 1(last server), line 49 throws, so the loop never exits normally when all servers fail. This throw will only execute ifserversis empty.Consider restructuring to handle the empty servers case explicitly:
static Future<String> uploadWithRetry( Uint8List data, String mimeType, ) async { final servers = BlossomConfig.defaultServers; + + if (servers.isEmpty) { + throw BlossomException('No Blossom servers configured'); + } for (int i = 0; i < servers.length; i++) { final serverUrl = servers[i]; _logger.d('Attempting upload to server ${i + 1}/${servers.length}: $serverUrl'); try { final client = BlossomClient(serverUrl: serverUrl); final blossomUrl = await client.uploadImage( imageData: data, mimeType: mimeType, ); _logger.i('✅ Upload successful to: $serverUrl'); return blossomUrl; } catch (e) { _logger.w('❌ Upload failed to $serverUrl: $e'); // If it's the last server, re-throw the error if (i == servers.length - 1) { throw BlossomException('All Blossom servers failed. Last error: $e'); } - - // Continue with next server - continue; } } - - throw BlossomException('No Blossom servers available'); + + // This should never be reached due to the throw on last server failure + throw StateError('Unreachable: upload loop exited unexpectedly'); }lib/features/chat/widgets/encrypted_image_message.dart (1)
209-219: Localize user-facing strings.Per coding guidelines, user-facing strings should use
S.of(context)rather than hardcoded literals. This applies to "Decrypting image..." and similar strings in_buildErrorWidgetand_openImage.Padding( padding: const EdgeInsets.symmetric(horizontal: 16), child: Text( - 'Decrypting image...', + S.of(context).decryptingImage, style: TextStyle(Also update the error messages at lines 254, 371, and 382.
lib/features/chat/notifiers/chat_room_notifier.dart (2)
488-494: Consider cache cleanup on dispose.The in-memory caches (
_imageCache,_imageMetadata,_fileCache,_fileMetadata) are not cleared indispose(). While the references will be garbage collected when the notifier is disposed, explicitly clearing large binary data may help with memory pressure in long chat sessions.@override void dispose() { _subscription?.cancel(); _sessionListener?.close(); + _imageCache.clear(); + _fileCache.clear(); _logger.i('Disposed chat room notifier for orderId: $orderId'); super.dispose(); }
469-474: Consider reusing service instances.New
EncryptedImageUploadServiceinstances are created for each message. Since the service appears stateless, consider making it a field or using dependency injection to avoid repeated instantiation.+ final EncryptedImageUploadService _imageUploadService = EncryptedImageUploadService(); + final EncryptedFileUploadService _fileUploadService = EncryptedFileUploadService(); // In _processEncryptedImageMessage: - final uploadService = EncryptedImageUploadService(); - final decryptedImage = await uploadService.downloadAndDecryptImage( + final decryptedImage = await _imageUploadService.downloadAndDecryptImage(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md(1 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(3 hunks)lib/features/chat/widgets/encrypted_image_message.dart(1 hunks)lib/services/blossom_upload_helper.dart(1 hunks)lib/services/encrypted_file_upload_service.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)lib/services/image_upload_service.dart(1 hunks)test/mocks.mocks.dart(36 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/image_upload_service.dartlib/services/blossom_upload_helper.dartlib/services/encrypted_file_upload_service.dartlib/features/chat/widgets/encrypted_image_message.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/services/encrypted_image_upload_service.darttest/mocks.mocks.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/image_upload_service.dartlib/services/blossom_upload_helper.dartlib/services/encrypted_file_upload_service.dartlib/features/chat/widgets/encrypted_image_message.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/services/encrypted_image_upload_service.darttest/mocks.mocks.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/image_upload_service.dartlib/services/blossom_upload_helper.dartlib/services/encrypted_file_upload_service.dartlib/services/encrypted_image_upload_service.dart
lib/features/**/notifiers/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Files:
lib/features/chat/notifiers/chat_room_notifier.dart
**/*.{g.dart,mocks.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit generated files (
.g.dart,.mocks.dart) or add individual ignore comments - regenerate withdart run build_runner build -dinstead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually modify
test/mocks.mocks.dart- it already has file-level// ignore_for_file: must_be_immutableignore directives
Files:
test/mocks.mocks.dart
🧠 Learnings (33)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dartlib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-05-06T22:23:06.684Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/features/chat/notifiers/chat_rooms_notifier.dart:0-0
Timestamp: 2025-05-06T22:23:06.684Z
Learning: The `reload()` method in the chat_room_notifier is synchronous, so await is not required or possible.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.darttest/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/features/chat/notifiers/chat_room_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to integration_test/**/*.dart : Ensure integration test coverage for flows touching networking, background work, or session recovery in `integration_test/`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*_test.dart : Tests should mirror the feature layout under `test/` with the `*_test.dart` suffix; end-to-end flows should run from `integration_test/`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Track relay sources (user, mostro, default) using RelaySource enum for appropriate handling and storage strategy
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/models/relay_list_event.dart : Parse NIP-65 (kind 10002) events with robust handling of different timestamp formats and null-safe parsing for malformed events
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/data/repositories/**/*.dart : Implement Repository pattern for all data access - all data operations must go through repository classes
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Use `removeRelayWithBlacklist()` for Mostro/default relays (adds to blacklist) and `removeRelay()` for permanent user relay deletion
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement relay URL normalization by removing trailing slashes in all blacklist operations to ensure consistent matching
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/mostro_fsm.dart : Use MostroFSM for managing order state transitions - all state changes must go through FSM state methods
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-09-17T20:45:32.468Z
Learnt from: grunch
Repo: MostroP2P/mobile PR: 306
File: docs/architecture/REQUEST_ID_ANALYSIS.md:176-183
Timestamp: 2025-09-17T20:45:32.468Z
Learning: For PR #306 in MostroP2P/mobile repository: This is a documentation-only PR intended to explain how the current requestId system works, not to fix any bugs or issues in the code. The documentation should accurately reflect existing behavior.
Applied to files:
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
📚 Learning: 2025-09-17T20:45:07.179Z
Learnt from: grunch
Repo: MostroP2P/mobile PR: 306
File: docs/architecture/REQUEST_ID_ANALYSIS.md:114-118
Timestamp: 2025-09-17T20:45:07.179Z
Learning: For the MostroP2P/mobile project, grunch prefers to separate documentation PRs from code improvement PRs. Documentation PRs should focus on explaining how the current system works, while code fixes and improvements should be handled in separate PRs.
Applied to files:
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (15)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md (1)
1-805: Comprehensive and well-structured documentation.This architecture document provides excellent coverage of the encrypted file messaging implementation, including security considerations, message flows, and future improvements. The documentation aligns well with the code implementation.
lib/services/encrypted_image_upload_service.dart (1)
71-140: Well-structured upload flow.The
uploadEncryptedImagemethod follows a clean pipeline: read → validate/sanitize → encrypt → upload → return metadata. Proper error handling with informative logging at each step.lib/services/image_upload_service.dart (1)
13-48: Clean implementation.The upload flow is straightforward: read → validate/sanitize → upload. Uses
validateAndSanitizeImage(full sanitization) appropriately for non-encrypted uploads.lib/services/blossom_upload_helper.dart (1)
6-42: Good extraction of retry logic.This helper properly consolidates the retry-across-servers pattern that was previously duplicated. Documentation is clear about behavior, parameters, and exceptions.
lib/features/chat/widgets/encrypted_image_message.dart (3)
61-89: Clean build method.The
build()method is now side-effect free, correctly delegating to the appropriate widget based on cache state, loading status, or error condition.
390-437: Thorough filename sanitization.The
_sanitizeFilenamefunction provides good security coverage: path traversal prevention, Windows reserved name handling, and character normalization. The post-sanitization check at lines 357-360 provides defense in depth.
281-341: Good async error handling in_loadImage.Proper use of
mountedchecks after async operations, defensive null ID handling, and error state management. The error message is preserved for display in the error widget.lib/features/chat/notifiers/chat_room_notifier.dart (2)
415-451: Good message content processing flow.The
_processMessageContentmethod correctly parses JSON content and delegates to type-specific handlers. Errors are caught and logged without disrupting message display.
517-565: Smart selective pre-download strategy.Auto-downloading images for preview while deferring other file types is a good UX decision. The fallback to storing metadata-only on download failure ensures the message remains displayable.
lib/services/encrypted_file_upload_service.dart (5)
10-59: LGTM! Well-structured data class with proper serialization.The
EncryptedFileUploadResultclass correctly implements immutability, complete JSON serialization for NIP-59 messages, and safe deserialization with appropriate error handling.
68-123: LGTM! Clean encryption and upload flow.The method correctly orchestrates validation, ChaCha20-Poly1305 encryption, and upload with retry logic. Using
application/octet-streamfor encrypted data while preserving original MIME type in metadata is the right approach.
165-175: LGTM! Utility helpers are correct.Both
_bytesToHexand_formatFileSizeare correctly implemented for their purposes: hex encoding for nonces and human-readable size formatting for logs.
125-153: LGTM! Past review feedback successfully addressed.The
nonceHexparameter has been correctly removed from the method signature, and the documentation now clearly states that the nonce is automatically extracted from the encrypted blob. All callers in the codebase (chat_room_notifier.dart and encrypted_file_message.dart) have been updated to use the new signature with onlyblossomUrlandsharedKeyparameters.
155-163: External service dependencies verified.The methods correctly delegate to
BlossomUploadHelper.uploadWithRetry(Uint8List data, String mimeType) → Future<String>andBlossomDownloadService.downloadWithRetry(String blossomUrl, ...) → Future<Uint8List>. Both classes exist and are properly imported.test/mocks.mocks.dart (1)
1-3556: LGTM! Generated mocks are correct.The Mockito-generated mocks properly reflect the new services introduced for encrypted file messaging:
MockBlossomClient,MockEncryptedFileUploadService,MockEncryptedImageUploadService,MockBlossomDownloadService, andMockChatRoomNotifierare correctly generated- Mock method signatures align with the service interfaces, including the corrected
downloadAndDecryptFilesignature without thenonceHexparameter- Supporting fake classes provide appropriate return values
As per coding guidelines: never manually edit this file. Regenerate with
dart run build_runner build -dif changes are needed.Based on learnings, this file already has the required file-level ignore directives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/features/chat/widgets/encrypted_image_message.dart (3)
364-367: Consider removing redundant security check.The security validation here duplicates checks already performed by
_sanitizeFilename(lines 400, 414, 434-441). While defensive programming is valuable, the sanitization function already handles path traversal, empty filenames, and dangerous characters.You could simplify by relying on the sanitization result:
final tempDir = await getTemporaryDirectory(); final sanitizedFilename = _sanitizeFilename(metadata.filename); - final tempFile = File('${tempDir.path}/$sanitizedFilename'); - - // Security check - if (sanitizedFilename.contains('/') || sanitizedFilename.contains('\\') || - sanitizedFilename.contains('..') || sanitizedFilename.trim().isEmpty) { - throw Exception(securityErrorMsg); - } - + final tempFile = File('${tempDir.path}/$sanitizedFilename'); + await tempFile.writeAsBytes(imageData);
402-408: Consider using a Unicode normalization package.The manual accent replacement handles common Spanish characters but won't cover other languages. For better i18n support, consider using a package like
diacriticorunorm_dartfor comprehensive Unicode normalization.Example with the
diacriticpackage:import 'package:diacritic/diacritic.dart'; String _sanitizeFilename(String filename) { final basename = filename.split(RegExp(r'[/\\]')).last; // Normalize using package String normalized = removeDiacritics(basename); // Rest of sanitization logic... }
304-314: JSON parsing could be more robust.The simple check
!content.startsWith('{')at line 305 is fragile. Consider validating that the parsed JSON contains expected fields before passing tofromJson.final content = widget.message.content; - if (content == null || !content.startsWith('{')) { + if (content == null || content.trim().isEmpty) { throw Exception(S.of(context)!.invalidImageMessageFormat); } - final imageData = EncryptedImageUploadResult.fromJson( - Map<String, dynamic>.from( - // ignore: avoid_dynamic_calls - jsonDecode(content) as Map - ) - ); + final parsedJson = jsonDecode(content); + if (parsedJson is! Map<String, dynamic>) { + throw Exception(S.of(context)!.invalidImageMessageFormat); + } + final imageData = EncryptedImageUploadResult.fromJson(parsedJson);lib/services/encrypted_image_upload_service.dart (1)
47-62: Consider more detailed error messages infromJson.While the try-catch addresses type safety, the generic error message doesn't indicate which field is missing or invalid. For better debuggability, consider catching specific field errors.
factory EncryptedImageUploadResult.fromJson(Map<String, dynamic> json) { try { + if (!json.containsKey('blossom_url')) { + throw FormatException('Missing required field: blossom_url'); + } return EncryptedImageUploadResult( blossomUrl: json['blossom_url'] as String, nonce: json['nonce'] as String, mimeType: json['mime_type'] as String, originalSize: json['original_size'] as int, width: json['width'] as int, height: json['height'] as int, filename: json['filename'] as String, encryptedSize: json['encrypted_size'] as int, ); - } catch (e) { - throw FormatException('Invalid EncryptedImageUploadResult JSON: $e'); + } on TypeError catch (e) { + throw FormatException('Invalid field type in JSON: $e'); + } on FormatException { + rethrow; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md(1 hunks)lib/features/chat/widgets/encrypted_image_message.dart(1 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/blossom_upload_helper.dart(1 hunks)lib/services/encrypted_image_upload_service.dart(1 hunks)lib/services/image_upload_service.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/ENCRYPTED_IMAGE_MESSAGING_IMPLEMENTATION.md
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/services/blossom_upload_helper.dart
- lib/services/image_upload_service.dart
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/encrypted_image_upload_service.dartlib/features/chat/widgets/encrypted_image_message.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/encrypted_image_upload_service.dartlib/features/chat/widgets/encrypted_image_message.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/encrypted_image_upload_service.dart
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it) - use
S.of(context)!.keyNamefor all user-facing strings
Files:
lib/l10n/intl_en.arblib/l10n/intl_it.arblib/l10n/intl_es.arb
🧠 Learnings (4)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/chat/widgets/encrypted_image_message.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/l10n/**/*.arb : Add new localization keys to all three ARB files (en, es, it) - use `S.of(context)!.keyName` for all user-facing strings
Applied to files:
lib/l10n/intl_en.arblib/l10n/intl_it.arblib/l10n/intl_es.arb
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
lib/l10n/intl_en.arb (1)
310-312: LGTM! Localization keys properly added across all three locale files.The new localization keys for file messaging, file confirmation, and image decryption have been correctly added to all three ARB files (English, Spanish, and Italian) as required by the coding guidelines. The 16 new keys are:
- Validation/Security errors:
invalidMessageMissingId,invalidImageMessageFormat,securityErrorInvalidChars- File messaging:
download,openFile,encrypted,downloadingFile- File confirmation:
confirmFileUpload,sendThisFile,send- Image decryption:
decryptingImage,failedToLoadImage,imageDecryptionError,imageNotAvailable,couldNotOpenFile,errorOpeningFileAll keys follow consistent naming conventions, are properly organized with comment sections, and the translations appear appropriate for each locale. This implementation correctly follows the guideline to add new localization keys to all three ARB files.
Based on coding guidelines.
Also applies to: 1197-1216
lib/features/chat/widgets/encrypted_image_message.dart (1)
34-60: LGTM! Previous review concerns addressed.The initialization logic correctly uses a post-frame callback to trigger image loading, avoiding side effects in
build(). The defensive null check formessageIdwith proper error handling addresses previous review concerns.lib/services/encrypted_image_upload_service.dart (2)
141-168: LGTM! Previous review concern addressed.The unused
nonceHexparameter has been correctly removed from the method signature. The nonce is now properly read from the encrypted blob bydecryptFromBlob, eliminating the redundancy.
115-116: Filename generation is acceptable.Using
millisecondsSinceEpochfor filename generation is appropriate for this use case. While theoretical collisions are possible with rapid uploads, the simplicity and chronological ordering benefits outweigh the negligible risk.
to fix #351
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.