phase 3 add multimedia sending to dispute chat#509
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReorders multimedia plan phases and implements Phase 3 sending: message model changed to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as DisputeMessageInput
participant Picker as FilePicker
participant Notifier as DisputeChatNotifier
participant Upload as EncryptedUploadService
participant Chat as P2P/ChatProvider
User->>UI: tap attach
UI->>Picker: open file chooser
Picker-->>UI: selected file (metadata)
UI->>User: show confirm dialog (filename/size)
User-->>UI: confirm
UI->>Notifier: getAdminSharedKey()
UI->>Upload: encrypt & upload file/image
Upload-->>UI: success (media metadata/URL)
UI->>Chat: send DisputeChatMessage (event with media metadata)
Chat-->>UI: ack
UI-->>User: show placeholder bubble (sending/complete)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md`:
- Around line 295-302: Three fenced code blocks are missing language specifiers
(MD040); add the language label "text" to each fenced block that contains the
listed lines so the linter accepts them — specifically update the block
containing the service signatures (e.g.,
EncryptedImageUploadService.uploadEncryptedImage(imageFile, sharedKey) and
EncryptedFileUploadService.uploadEncryptedFile(file, sharedKey)), and the two UI
placeholder mockup blocks that start with the ASCII box/emoji lines (the block
showing "┌─────────────────────────────┐" and the one showing "│ 📄
document.pdf │"); simply change the opening ``` to ```text for those
three blocks.
- Add attachment button to DisputeMessageInput matching P2P chat UX - Encrypt and upload images/files to Blossom using admin shared key - Show placeholder in bubble for sent multimedia (icon + filename + size) - Update DisputeMessageBubble to accept DisputeChatMessage and detect type - Reuse existing upload services with zero changes to P2P chat code
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/features/disputes/widgets/dispute_message_input.dart (1)
89-90: Avoid reading entire files just to detect MIME type.
readAsBytes()loads the full attachment into memory before upload. For large files, this can cause avoidable memory spikes; reading only header bytes is enough for MIME sniffing.Suggested refactor
- final fileData = await file.readAsBytes(); - final isImage = await _isImageFile(fileData, filename); + final isImage = await _isImageFile(file, filename); - Future<bool> _isImageFile(Uint8List fileData, String filename) async { + Future<bool> _isImageFile(File file, String filename) async { try { - final mimeType = await _getMimeType(fileData, filename); + final mimeType = await _getMimeType(file, filename); return MediaValidationService.isImageTypeSupported(mimeType ?? ''); } catch (e) { return false; } } - Future<String?> _getMimeType(Uint8List fileData, String filename) async { - final mimeFromBytes = lookupMimeType('', headerBytes: fileData); + Future<String?> _getMimeType(File file, String filename) async { + final headerBytes = await file.openRead(0, 64).expand((chunk) => chunk).take(64).toList(); + final mimeFromBytes = lookupMimeType(filename, headerBytes: headerBytes); if (mimeFromBytes != null) { return mimeFromBytes; } return lookupMimeType(filename); }Also applies to: 133-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/widgets/dispute_message_input.dart` around lines 89 - 90, The code currently calls file.readAsBytes() before calling _isImageFile, which reads the entire file into memory; change this to read only the minimal header bytes for MIME sniffing (e.g., open a stream and read the first N bytes or use file.openRead(0, N)) and pass that slice to _isImageFile instead of the full payload; update both places where file.readAsBytes() is used (the call before _isImageFile and the similar block around lines 133–147) and ensure _isImageFile accepts/handles the header byte buffer only or add a small helper that extracts header bytes before invoking _isImageFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/disputes/widgets/dispute_message_bubble.dart`:
- Around line 178-181: The timestamp text in _buildTimestamp() uses
_formatTime(...) which returns hardcoded English strings (e.g., "now", "m ago");
update localization by routing all user-facing timestamp fragments through ARB
keys and S.of(context)!.keyName. Modify _formatTime to accept a BuildContext (or
add a wrapper that maps its output) and replace hardcoded literals inside
_formatTime with calls to S.of(context)!.timestampNow,
S.of(context)!.minutesAgo, S.of(context)!.hoursAgo, etc.; update
_buildTimestamp() to pass its context into _formatTime (or call the new wrapper)
so the rendered Text uses localized strings. Ensure new ARB keys are added and
used consistently for singular/plural forms where needed.
- Around line 92-94: The code unsafely casts metadata entries causing CastError;
replace the direct casts for original_size (and optionally filename) with
defensive parsing: read metadata['original_size'] into a local var, handle if
it's an int, num (call toInt()), or String (use int.tryParse) and default to 0
if parsing fails, and for filename ensure you coerce non-null values to String
safely (e.g., using toString() or check type) before assigning to filename;
update the variables originalSize and filename in dispute_message_bubble.dart
accordingly so they never throw on unexpected backend types.
In `@lib/features/disputes/widgets/dispute_message_input.dart`:
- Around line 65-79: After the await FilePicker.platform.pickFiles(...) call,
ensure you check the widget's mounted flag before using context: if (!mounted)
return; then call _showFileConfirmationDialog(filename). Also add another
mounted check immediately after awaiting _showFileConfirmationDialog(filename)
(and before any subsequent async operations or usage of context); if the widget
is no longer mounted, abort the upload flow. This ensures
FilePicker.platform.pickFiles, _showFileConfirmationDialog, and any following
context usage are guarded by mounted checks.
---
Nitpick comments:
In `@lib/features/disputes/widgets/dispute_message_input.dart`:
- Around line 89-90: The code currently calls file.readAsBytes() before calling
_isImageFile, which reads the entire file into memory; change this to read only
the minimal header bytes for MIME sniffing (e.g., open a stream and read the
first N bytes or use file.openRead(0, N)) and pass that slice to _isImageFile
instead of the full payload; update both places where file.readAsBytes() is used
(the call before _isImageFile and the similar block around lines 133–147) and
ensure _isImageFile accepts/handles the header byte buffer only or add a small
helper that extracts header bytes before invoking _isImageFile.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/disputes/widgets/dispute_message_bubble.dartlib/features/disputes/widgets/dispute_message_input.dartlib/features/disputes/widgets/dispute_messages_list.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md (1)
37-47: Use consistent English terminology for “decoupling.”The plan mixes English with
desacoplamiento. Consider replacing withdecouplingin these headings/body sections for consistency and easier team-wide readability.Also applies to: 558-561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md` around lines 37 - 47, Replace the Spanish word "desacoplamiento" with the English term "decoupling" throughout the document (e.g., update the heading "### 2. Sending does not require desacoplamiento" and any occurrences in the body and in the "New Phase 4" bullet), keeping surrounding context intact; ensure references like EncryptedImageMessage, EncryptedFileMessage, EncryptedImageUploadService, EncryptedFileUploadService, BlossomUploadHelper and DisputeChatNotifier remain unchanged and that capitalization matches the existing text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md`:
- Around line 267-270: Update the Phase 3 status field in the document where it
currently reads "Status: `TODO`" (near the Phase 3 header referencing branch
`phase-3-dispute-multimedia-send`) to reflect the PR work — change it to `IN
PROGRESS` or `DONE` as appropriate so the roadmap matches the implemented
multimedia send UI changes in this PR.
---
Nitpick comments:
In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md`:
- Around line 37-47: Replace the Spanish word "desacoplamiento" with the English
term "decoupling" throughout the document (e.g., update the heading "### 2.
Sending does not require desacoplamiento" and any occurrences in the body and in
the "New Phase 4" bullet), keeping surrounding context intact; ensure references
like EncryptedImageMessage, EncryptedFileMessage, EncryptedImageUploadService,
EncryptedFileUploadService, BlossomUploadHelper and DisputeChatNotifier remain
unchanged and that capitalization matches the existing text.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.mdlib/features/disputes/widgets/dispute_message_input.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/disputes/widgets/dispute_message_input.dart
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/features/disputes/widgets/dispute_message_bubble.dart (1)
92-93:⚠️ Potential issue | 🟠 MajorDefensively parse multimedia metadata to prevent render-time type crashes.
Direct casts here still throw when backend metadata drifts (e.g.,
original_size: "50000"or non-stringfilename). That can crash bubble rendering for a message.🛠️ Proposed fix
- final filename = metadata['filename'] as String? ?? ''; - final originalSize = (metadata['original_size'] as num?)?.toInt() ?? 0; + final filenameRaw = metadata['filename']; + final filename = switch (filenameRaw) { + String value when value.trim().isNotEmpty => value, + null => isImage ? S.of(context)!.imageSent : S.of(context)!.fileSent, + _ => filenameRaw.toString(), + }; + + final originalSizeRaw = metadata['original_size']; + final originalSize = switch (originalSizeRaw) { + int value => value, + num value => value.toInt(), + String value => int.tryParse(value) ?? 0, + _ => 0, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/widgets/dispute_message_bubble.dart` around lines 92 - 93, The current direct casts for metadata in dispute_message_bubble.dart (the filename and originalSize assignments) can throw if backend types drift; update the parsing to be defensive: for filename use metadata['filename'] != null ? metadata['filename'].toString() : '' (or metadata['filename']?.toString() ?? ''), and for originalSize read the raw value (var v = metadata['original_size']), then if v is num use v.toInt(), else if v is String use int.tryParse(v) ?? 0, else default to 0; replace the existing filename and originalSize assignments with these safe conversions so the widget (in the DisputeMessageBubble code) never crashes on unexpected metadata types.docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md (1)
266-270:⚠️ Potential issue | 🟡 MinorPhase 3 status is stale for a Phase 3 implementation PR.
This section still marks Phase 3 as
TODO. Please update it toIN PROGRESSorDONEso the roadmap matches the current PR scope.📝 Proposed fix
-**Status:** `TODO` +**Status:** `IN PROGRESS`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md` around lines 266 - 270, Update the Phase 3 status under the "## Phase 3: Multimedia Sending in Dispute Chat" section by replacing the current `Status: TODO` with the appropriate current state (`IN PROGRESS` or `DONE`) so the doc reflects the PR scope; specifically edit the "Status:" line in DISPUTE_CHAT_MULTIMEDIA_PLAN.md to the chosen state while leaving the rest of the header and branch/PR scope text intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/disputes/widgets/dispute_message_bubble.dart`:
- Around line 204-210: The _formatFileSize function emits hardcoded unit
strings; change it to use localized strings by accepting a BuildContext (e.g.,
rename to _formatFileSize(BuildContext context, int bytes) or otherwise obtain
context) and replace 'B', 'KB', 'MB' with S.of(context)!.fileSizeBytes,
S.of(context)!.fileSizeKB, S.of(context)!.fileSizeMB (or the correct keys in
your S class), formatting the numeric portion the same way but concatenating
with the localized unit strings so all user-facing text comes from localization.
---
Duplicate comments:
In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md`:
- Around line 266-270: Update the Phase 3 status under the "## Phase 3:
Multimedia Sending in Dispute Chat" section by replacing the current `Status:
TODO` with the appropriate current state (`IN PROGRESS` or `DONE`) so the doc
reflects the PR scope; specifically edit the "Status:" line in
DISPUTE_CHAT_MULTIMEDIA_PLAN.md to the chosen state while leaving the rest of
the header and branch/PR scope text intact.
In `@lib/features/disputes/widgets/dispute_message_bubble.dart`:
- Around line 92-93: The current direct casts for metadata in
dispute_message_bubble.dart (the filename and originalSize assignments) can
throw if backend types drift; update the parsing to be defensive: for filename
use metadata['filename'] != null ? metadata['filename'].toString() : '' (or
metadata['filename']?.toString() ?? ''), and for originalSize read the raw value
(var v = metadata['original_size']), then if v is num use v.toInt(), else if v
is String use int.tryParse(v) ?? 0, else default to 0; replace the existing
filename and originalSize assignments with these safe conversions so the widget
(in the DisputeMessageBubble code) never crashes on unexpected metadata types.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.mdlib/features/disputes/widgets/dispute_message_bubble.dart
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Phase 3 — Dispute Multimedia Sending
CI passes ✅, the architecture decision to reverse phases 3/4 is well-reasoned, and reusing the existing upload services is clean. However, there are several issues that need to be addressed before merging.
Summary
- 3 blocking issues (resource leak, memory safety, missing validation)
- 3 non-blocking suggestions
…tion - Make upload service instances static final (stateless, no resource leak) - Add file.length() pre-check before readAsBytes() to prevent OOM on large files - Replace readAsBytes() MIME sniffing with lookupMimeType(filename) - Remove unused _isImageFile(), _getMimeType() helpers and dart:typed_data import
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/features/disputes/widgets/dispute_message_input.dart (1)
125-131:⚠️ Potential issue | 🟠 MajorDon’t expose raw exception text in the snackbar.
Line 129 passes
e.toString()directly to UI. This can leak internal details and creates inconsistent user messaging. Show a safe localized message and log full details with the sharedlogger.💡 Suggested fix
+import 'package:mostro_mobile/services/logger_service.dart'; ... - } catch (e) { + } catch (e, st) { + logger.e('Dispute file upload failed', error: e, stackTrace: st); if (mounted) { SnackBarHelper.showTopSnackBar( context, - S.of(context)!.errorUploadingFile(e.toString()), + S.of(context)!.errorUploadingFile(S.of(context)!.uploadFailedTryAgain), backgroundColor: Colors.red, ); } }Based on learnings: "do not expose raw exception messages (e.g.,
e.toString()) in the UI... Full error information is logged vialogger.e()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/widgets/dispute_message_input.dart` around lines 125 - 131, The catch block in dispute_message_input.dart currently shows raw exception text via SnackBarHelper.showTopSnackBar(context, S.of(context)!.errorUploadingFile(e.toString())), which can leak internals; instead log the full error with the shared logger (e.g., call logger.e(e) or logger.e('Upload error', e)) and pass a safe localized message to the UI (use S.of(context)!.errorUploadingFile() or a new generic localization key) so the snackbar displays user-friendly text; update the code around the catch (e) { if (mounted) { ... } } block to remove e.toString() from the UI and add the logger.e(...) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.md`:
- Around line 403-408: The doc snippet shows instance fields (_isUploadingFile,
EncryptedImageUploadService, EncryptedFileUploadService) but the widget uses
static final services; update the snippet to declare the upload services as
static final (e.g., make the EncryptedImageUploadService and
EncryptedFileUploadService fields static final) while keeping _isUploadingFile
as an instance bool, so the snippet matches the implemented lifecycle of the
services and avoids reverting to the old instance-field pattern.
- Around line 426-427: Update the MIME detection docs to match the shipped
implementation: replace references to byte/header-based helpers (_isImageFile,
_getMimeType) with the actual flow that uses lookupMimeType(filename) for MIME
detection and falls back to extension-based logic; also ensure the step
numbering and examples in the same section (including the repeated content at
440-454) mention the current call pattern and not the old byte helpers, and keep
the existing note about obtaining the shared key via await
ref.read(disputeChatNotifierProvider(disputeId).notifier).getAdminSharedKey().
In `@lib/features/disputes/widgets/dispute_message_input.dart`:
- Around line 84-87: The hardcoded English string in the
S.of(context)!.errorUploadingFile call should be moved into the ARB/localization
system; create a new localized key (e.g. fileTooLargeWithSize) in your .arb(s)
that accepts two parameters (actualSizeMb and maxSizeMb), update the generated S
class, and replace the inline concatenation in dispute_message_input.dart so you
call S.of(context)!.fileTooLargeWithSize(actualSizeMb, maxSizeMb) using the
computed values from FileValidationService.maxFileSize and (fileSize / (1024 *
1024)).toStringAsFixed(1); ensure plural/formatting rules in the ARB entry as
needed.
---
Duplicate comments:
In `@lib/features/disputes/widgets/dispute_message_input.dart`:
- Around line 125-131: The catch block in dispute_message_input.dart currently
shows raw exception text via SnackBarHelper.showTopSnackBar(context,
S.of(context)!.errorUploadingFile(e.toString())), which can leak internals;
instead log the full error with the shared logger (e.g., call logger.e(e) or
logger.e('Upload error', e)) and pass a safe localized message to the UI (use
S.of(context)!.errorUploadingFile() or a new generic localization key) so the
snackbar displays user-friendly text; update the code around the catch (e) { if
(mounted) { ... } } block to remove e.toString() from the UI and add the
logger.e(...) call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbc8904a-6c4c-42a9-8af4-28860513cdca
📒 Files selected for processing (2)
docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.mdlib/features/disputes/widgets/dispute_message_input.dart
|
@mostronatorcoder Addressed (with adjustments):
|
There was a problem hiding this comment.
All three blocking issues resolved in ca4ca72:
- ✅ Services are now
static final— no resource leak - ✅
file.length()pre-check prevents OOM on large files - ✅ MIME detection via
lookupMimeType(filename)— no more full-filereadAsBytes() - ✅ Removed unused
_isImageFile(),_getMimeType()helpers
The three non-blocking suggestions (double JSON parse, timeago TODO, admin key error UX) are fine to address in a future phase.
LGTM 👍
2 participants
How to test
Known limitations (intentional in this phase)
Summary by CodeRabbit
Documentation
New Features
UI Improvements
Tests