feat: upload media to blossom servers optimistically #728
Conversation
WalkthroughRefactors chat input to replace string-based selectedImages with MediaFileUpload items, adds an injectable async upload function ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ChatInput Widget
participant Notifier as ChatInputNotifier
participant Upload as uploadMediaFn
participant State as ChatInputState
User->>UI: select images(paths)
UI->>Notifier: onImagesSelected(paths)
Notifier->>State: set selectedMedia -> uploading items
State-->>UI: rebuild (show uploading overlays)
loop per file
Notifier->>Upload: uploadMediaFn(accountPubkey, groupId, filePath)
alt success
Upload-->>Notifier: MediaFile
Notifier->>State: replace item -> uploaded (with uploadedFile)
else failure
Upload-->>Notifier: throws
Notifier->>State: replace item -> failed (error)
end
State-->>UI: rebuild (update thumbnail/preview)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
josefinalliende
force-pushed
the
feat/media-upload-ui
branch
2 times, most recently
from
c28a784 to
561a675
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
3441a4c to
9ff939a
Compare
josefinalliende
changed the base branch from
master
to
feat/upload-chat-media-method
josefinalliende
force-pushed
the
feat/upload-images
branch
from
9ff939a to
3e901ab
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
3e901ab to
6992ef6
Compare
josefinalliende
changed the base branch from
feat/upload-chat-media-method
to
feat/media-file-upload-model
josefinalliende
force-pushed
the
feat/media-file-upload-model
branch
from
af3ced0 to
f8781f7
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
6992ef6 to
71c5641
Compare
josefinalliende
force-pushed
the
feat/media-file-upload-model
branch
from
f8781f7 to
f9d7a27
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
71c5641 to
8b446c8
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
8b446c8 to
d73b757
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/config/providers/active_account_provider_test.dart (1)
468-476: Fix typo in test description ("thows" → "throws").Keeps test names clear and searchable.
- test('thows error but does not update state', () async { + test('throws error but does not update state', () async {
🧹 Nitpick comments (7)
lib/ui/chat/widgets/chat_input_media_preview.dart (2)
83-86: Declare explicit types for locals per guidelines.Use explicit types for readability and to comply with project rules.
- final mediaItem = widget.mediaItems[index]; + final MediaFileUpload mediaItem = widget.mediaItems[index]; ... - final itemIndex = index - 1; - final mediaItem = widget.mediaItems[itemIndex]; + final int itemIndex = index - 1; + final MediaFileUpload mediaItem = widget.mediaItems[itemIndex];Also applies to: 172-179
84-147: Preview states look solid; add small robustness/perf tweaks.
- Add errorBuilder to avoid red-frame on decode errors.
- Use responsive sizing for the 32x32 loader to match ScreenUtil usage elsewhere.
- ClipRRect( - child: Image.file( + ClipRRect( + child: Image.file( File(filePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => const SizedBox.shrink(), ), ), ... - child: SizedBox( - width: 32, - height: 32, + child: SizedBox( + width: 32.w, + height: 32.h, child: CircularProgressIndicator(lib/ui/chat/widgets/media_thumbnail.dart (2)
24-46: Nice state-driven tap behavior. Consider basic semantics.Expose status to screen readers (uploading/failed/ready) so disabled taps aren’t confusing.
- return GestureDetector( + return Semantics( + label: mediaItem.isUploading + ? 'Uploading media' + : (mediaItem.isFailed ? 'Media upload failed' : 'Media ready'), + button: true, + enabled: !mediaItem.isUploading, + child: GestureDetector( onTap: mediaItem.isUploading ? null : onTap, child: mediaItem.when( ... - ), - ); + ), + );
49-76: Downscale decode for 32×32 thumbnails to save memory.Hint the codec with cacheWidth/height to avoid decoding full-size images.
child: ClipRRect( child: Image.file( File(filePath), height: 32.h, width: 32.w, fit: BoxFit.cover, + cacheWidth: (32.w * MediaQuery.of(context).devicePixelRatio).round(), + cacheHeight: (32.h * MediaQuery.of(context).devicePixelRatio).round(), errorBuilder: (context, error, stackTrace) => const SizedBox.shrink(), ), ),lib/config/providers/chat_input_provider.dart (2)
86-112: Make fire-and-forget intent explicit and type locals.
- Use unawaited(...) to satisfy lints and document intent.
- Declare explicit types for imagePaths, uploadingItems, and loop variable.
Future<void> handleImagesSelected() async { try { - final imagePaths = await _imagePickerService.pickMultipleImages(); + final List<String> imagePaths = await _imagePickerService.pickMultipleImages(); if (imagePaths.isEmpty) { state = state.copyWith(showMediaSelector: false); return; } - final uploadingItems = - imagePaths - .map( - (path) => MediaFileUpload.uploading(filePath: path), - ) - .toList(); + final List<MediaFileUpload> uploadingItems = imagePaths + .map((String path) => MediaFileUpload.uploading(filePath: path)) + .toList(); state = state.copyWith( showMediaSelector: false, selectedMedia: [...state.selectedMedia, ...uploadingItems], ); - for (final path in imagePaths) { - _uploadImage(path); + for (final String path in imagePaths) { + unawaited(_uploadImage(path)); } } catch (e) { _logger.warning('Failed to select images for group $_groupId', e); state = state.copyWith(showMediaSelector: false); return; } }
155-162: Minor: consistent naming and explicit type.
- Use “media” in logs to match state.
- Type the updated list.
void removeImage(int index) { if (index < 0 || index >= state.selectedMedia.length) { - _logger.warning('Invalid image index: $index'); + _logger.warning('Invalid media index: $index'); return; } - final updatedMedia = List<MediaFileUpload>.from(state.selectedMedia); + final List<MediaFileUpload> updatedMedia = List<MediaFileUpload>.from(state.selectedMedia); updatedMedia.removeAt(index); state = state.copyWith(selectedMedia: updatedMedia); }test/config/providers/chat_input_provider_test.dart (1)
57-74: Consider using the test constant for account pubkey.The
createMockMediaFilefunction hardcodesaccountPubkeyas'test-account-pubkey-hex', but the test setup definestestAccountPubkeywith a different value (line 84). While this may not affect current tests, using the constant would improve consistency and prevent potential confusion.Apply this diff to use the constant:
rust_media_files.MediaFile createMockMediaFile({ required String id, required String groupId, required String filePath, + String? accountPubkey, }) { return rust_media_files.MediaFile( id: id, mlsGroupId: groupId, - accountPubkey: 'test-account-pubkey-hex', + accountPubkey: accountPubkey ?? 'test-account-pubkey-hex', filePath: filePath, fileHash: 'test-hash-$id', mimeType: 'image/jpeg', mediaType: 'image', blossomUrl: 'https://test.com/media/$id', nostrKey: 'test-nostr-key-$id', createdAt: DateTime(2025, 1, 3), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/config/providers/chat_input_provider.dart(3 hunks)lib/ui/chat/states/chat_input_state.dart(2 hunks)lib/ui/chat/states/chat_input_state.freezed.dart(13 hunks)lib/ui/chat/widgets/chat_input.dart(3 hunks)lib/ui/chat/widgets/chat_input_media_preview.dart(5 hunks)lib/ui/chat/widgets/media_thumbnail.dart(1 hunks)test/config/providers/active_account_provider_test.dart(1 hunks)test/config/providers/chat_input_provider_test.dart(8 hunks)test/config/providers/group_messages_provider_test.dart(1 hunks)test/config/providers/group_provider_test.dart(1 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(1 hunks)test/shared/mocks/mock_active_pubkey_notifier.dart(1 hunks)test/shared/mocks/mock_draft_message_service.dart(1 hunks)test/shared/mocks/mock_image_picker_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value)
Avoid using dynamic and Object without justification
Create necessary types instead of overusing primitives or dynamic
One export per file
Use PascalCase for classes
Use camelCase for variables, functions, and methods
Avoid magic numbers and define constants
Start each function name with a verb
Use verbs for boolean variables (e.g., isLoading, hasError, canDelete)
Write short functions with a single purpose (under ~20 instructions)
Name functions with a verb plus context; for booleans use isX/hasX/canX; for void use executeX/saveX
Avoid deep nesting via early returns and extraction to utility functions
Use higher-order functions (map, where/filter, reduce) to avoid nesting
Use arrow functions for simple functions (under ~3 statements); use named functions otherwise
Use default parameter values instead of null checks
Reduce function parameters using RO-RO: pass/return parameter objects with declared types
Maintain a single level of abstraction within functions
Encapsulate data in composite types; avoid overusing primitives
Prefer validating data within classes rather than in functions
Prefer immutability; use final for runtime constants and const for compile-time constants
Use const constructors and const literals where possible
Follow SOLID principles
Prefer composition over inheritance
Declare interfaces (abstract classes) to define contracts
Write small classes with a single purpose (under ~200 instructions, <10 public methods, <10 properties)
Use exceptions for unexpected errors
Only catch exceptions to fix expected problems or add context; otherwise use a global handler
Files:
test/config/providers/group_provider_test.darttest/shared/mocks/mock_active_pubkey_notifier.darttest/config/providers/group_messages_provider_test.dartlib/ui/chat/states/chat_input_state.darttest/config/providers/chat_input_provider_test.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/widgets/chat_input.dartlib/ui/chat/widgets/media_thumbnail.darttest/config/providers/profile_ready_card_visibility_provider_test.darttest/shared/mocks/mock_draft_message_service.darttest/config/providers/active_account_provider_test.darttest/shared/mocks/mock_image_picker_service.dartlib/ui/chat/widgets/chat_input_media_preview.dartlib/config/providers/chat_input_provider.dart
test/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
test/**/*.dart: Follow Arrange-Act-Assert in tests
Name test variables clearly using inputX, mockX, actualX, expectedX
Write unit tests for each public function; use test doubles for dependencies (except cheap third-party)
Use standard Flutter widget testing
Files:
test/config/providers/group_provider_test.darttest/shared/mocks/mock_active_pubkey_notifier.darttest/config/providers/group_messages_provider_test.darttest/config/providers/chat_input_provider_test.darttest/config/providers/profile_ready_card_visibility_provider_test.darttest/shared/mocks/mock_draft_message_service.darttest/config/providers/active_account_provider_test.darttest/shared/mocks/mock_image_picker_service.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
lib/**/*.dart: Use flutter_rust_bridge to access core app functionality
Use Riverpod for state management; prefer StreamProviders for Rust API streams; use keepAlive if needed
Use freezed to model/manage UI states
Controllers should expose methods as inputs and update UI state that drives the UI
Use AutoRoute for navigation and use extras to pass data between pages
Use Dart extensions to manage reusable code
Use ThemeData to manage themes
Use AppLocalizations for translations
Use constants to manage constant values
Avoid deeply nested widget trees; aim for a flatter widget structure for performance and readability
Break down large widgets into smaller, focused, reusable components
Keep the widget tree shallow to simplify state management and data flow
Utilize const constructors and const widgets wherever possible to reduce rebuilds
Files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/widgets/chat_input.dartlib/ui/chat/widgets/media_thumbnail.dartlib/ui/chat/widgets/chat_input_media_preview.dartlib/config/providers/chat_input_provider.dart
🧠 Learnings (6)
📚 Learning: 2025-09-07T02:15:31.931Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/src/rust/api/utils.dart:12-16
Timestamp: 2025-09-07T02:15:31.931Z
Learning: The lingering PublicKey references in lib/config/providers/auth_provider.dart, lib/config/providers/group_provider.dart, and rust_builder/cargokit/build_tool/lib/src/options.dart identified during the utils.dart API cleanup were fixed in PR #597 in the whitenoise_flutter codebase.
Applied to files:
test/config/providers/group_provider_test.darttest/config/providers/active_account_provider_test.dart
📚 Learning: 2025-09-08T17:39:13.711Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#599
File: test/ui/contact_list/start_chat_bottom_sheet_test.dart:173-176
Timestamp: 2025-09-08T17:39:13.711Z
Learning: In the whitenoise Flutter app, the followProvider uses followsProvider behind the scenes, so mocking followsProvider with MockFollowsNotifier effectively controls what followProvider sees in tests.
Applied to files:
test/config/providers/group_provider_test.darttest/shared/mocks/mock_active_pubkey_notifier.darttest/config/providers/profile_ready_card_visibility_provider_test.darttest/config/providers/active_account_provider_test.dart
📚 Learning: 2025-09-07T13:10:16.542Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#597
File: lib/config/providers/group_provider.dart:311-314
Timestamp: 2025-09-07T13:10:16.542Z
Learning: In the whitenoise_flutter codebase, the User class used in group_provider.dart (and similar contexts) is presentational only, not the actual user class from the Rust API. There are plans to remove this User class and replace it with UserProfileData, similar to the planned consolidation with ContactModel.
Applied to files:
test/config/providers/group_provider_test.dart
📚 Learning: 2025-10-15T02:08:41.843Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#721
File: lib/ui/chat/widgets/chat_input.dart:80-87
Timestamp: 2025-10-15T02:08:41.843Z
Learning: In `lib/ui/chat/widgets/chat_input.dart`, the `_toggleMediaSelector()` method correctly reads `chatInputState` before toggling and checks `!chatInputState.showMediaSelector` to unfocus when opening the selector. This matches the design requirement where the input should be unfocused when the media selector is shown.
Applied to files:
lib/ui/chat/states/chat_input_state.darttest/config/providers/chat_input_provider_test.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/widgets/chat_input.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to lib/**/*.dart : Use freezed to model/manage UI states
Applied to files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/states/chat_input_state.freezed.dart
📚 Learning: 2025-10-24T13:53:05.425Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#740
File: lib/domain/models/media_file_upload.dart:30-34
Timestamp: 2025-10-24T13:53:05.425Z
Learning: In the MediaFileUpload model (lib/domain/models/media_file_upload.dart), the originalFilePath getter correctly returns the local file path for all states: during uploading and failed states the path has not changed (upload didn't succeed), and only in the uploaded state does the file get a new remote path, with originalFilePath preserving the original local path. This design prevents UI blinking during state transitions.
Applied to files:
lib/ui/chat/widgets/media_thumbnail.dart
🔇 Additional comments (16)
test/shared/mocks/mock_draft_message_service.dart (1)
4-35: LGTM!Clean mock implementation with appropriate fields for testing draft message operations and a useful
reset()utility for test cleanup.test/config/providers/profile_ready_card_visibility_provider_test.dart (1)
6-6: LGTM!Successfully migrated to the shared mock, reducing test duplication across the codebase.
test/config/providers/group_provider_test.dart (1)
12-12: LGTM!Successfully migrated to the shared mock, consistent with the codebase-wide consolidation effort.
test/shared/mocks/mock_image_picker_service.dart (1)
3-14: LGTM!Well-structured mock that supports both success and error test scenarios with configurable behavior.
lib/ui/chat/states/chat_input_state.dart (2)
2-2: LGTM!Added import for the new
MediaFileUploadmodel to support the refactoring from string-based paths to structured media objects.
11-11: LGTM!Refactoring from
List<String> selectedImagestoList<MediaFileUpload> selectedMediaimproves type safety and enables richer media metadata (loading states, errors, URLs) for the optimistic upload feature.lib/ui/chat/widgets/chat_input.dart (1)
169-169: LGTM!All references consistently updated from
selectedImagestoselectedMedia, maintaining uniform behavior across validation, preview rendering, and send button state.Also applies to: 237-237, 244-244, 280-280
test/config/providers/group_messages_provider_test.dart (1)
10-10: LGTM!Successfully migrated to the shared mock, completing the consolidation of
MockActivePubkeyNotifieracross the test suite.test/config/providers/active_account_provider_test.dart (1)
7-7: Good move: shared mock import reduces duplication.Importing MockActivePubkeyNotifier from a shared mock centralizes behavior and keeps tests DRY.
lib/ui/chat/states/chat_input_state.freezed.dart (1)
171-181: Freezed output matches the model change.selectedImages → selectedMedia is consistently applied (ctor, getter, copyWith, equality). No manual changes needed.
Also applies to: 189-196, 203-207
test/config/providers/chat_input_provider_test.dart (6)
12-55: LGTM! Well-structured test mock.The
MockUploadMediaFnclass effectively simulates upload behavior with configurable success/failure outcomes and tracks calls for verification. The implementation is clean and follows test mock best practices.
76-111: LGTM! Proper test setup and teardown.The test setup correctly configures all required mocks and overrides, including the new
uploadMediaFnparameter. The teardown properly resets state and disposes resources.
113-143: LGTM! Initial state tests properly updated.The initial state tests correctly verify the new
selectedMediaproperty and maintain proper test isolation by creating fresh containers.
392-408: LGTM! Good verification of upload parameters.The test properly verifies that the upload function receives the correct
groupId,filePath, andaccountPubkeyparameters.
411-483: LGTM! Comprehensive removal test coverage.The
removeImagetests thoroughly cover various scenarios including edge cases (invalid/negative indices) and different upload states (uploading, uploaded, failed). The test logic is sound and properly validates state transitions.
485-516: LGTM! Clear functionality properly tested.The tests correctly verify that
clear()removes all media along with other state, and properly calls theclearDraftservice.
josefinalliende
force-pushed
the
feat/upload-images
branch
from
eab2a76 to
5d4fb82
Compare
josefinalliende
force-pushed
the
feat/upload-images
branch
from
5d4fb82 to
3ca9821
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/config/providers/chat_input_provider.dart (1)
87-92: Log when skipping due to missing pubkey + use explicit types per guidelines.Add a warning to aid diagnostics and declare local types explicitly (required by our guidelines).
Apply this diff:
- final accountPubkey = ref.read(activePubkeyProvider); - final accountHexPubkey = PubkeyFormatter(pubkey: accountPubkey).toHex() ?? ''; + final String? accountPubkey = ref.read(activePubkeyProvider); + final String accountHexPubkey = PubkeyFormatter(pubkey: accountPubkey).toHex() ?? ''; if (accountHexPubkey.isEmpty) { + _logger.warning('Skip media upload: missing active pubkey for group $_groupId'); state = state.copyWith(showMediaSelector: false); return; }
🧹 Nitpick comments (16)
lib/ui/chat/states/chat_input_state.dart (1)
11-11: Prefer a typed const default for immutability clarity.Use a const, typed empty list to make intent explicit and avoid lint noise.
- @Default([]) List<MediaFileUpload> selectedMedia, + @Default(<MediaFileUpload>[]) List<MediaFileUpload> selectedMedia,lib/ui/chat/widgets/chat_input.dart (1)
237-241: Keep UI state consistent with media semantics.
- Media preview wiring with mediaItems looks good.
- Align the send button boolean with the same non‑failed predicate used for sending to avoid UI/behavior mismatch.
- hasImages: chatInputState.selectedMedia.isNotEmpty, + hasImages: chatInputState.selectedMedia + .any((m) => m.isUploading || m.isUploaded),Optionally, consider renaming the parameter in ChatInputSendButton from hasImages to hasMedia in a follow‑up for clarity.
Also applies to: 281-281
lib/ui/chat/widgets/chat_input_media_preview.dart (2)
44-56: Guard _activeThumbIndex against out‑of‑range after list updates.If items are removed externally, the saved index can point past the end and cause inconsistent UI. Clamp or reset on updates.
class _ChatInputMediaPreviewState extends State<ChatInputMediaPreview> { @@ void _handleThumbnailTap(int index) { @@ } + + @override + void didUpdateWidget(covariant ChatInputMediaPreview oldWidget) { + super.didUpdateWidget(oldWidget); + final int len = widget.mediaItems.length; + if (_activeThumbIndex != null && (_activeThumbIndex! < 0 || _activeThumbIndex! >= len)) { + setState(() => _activeThumbIndex = len == 0 ? null : len - 1); + } + }Also applies to: 171-179
88-96: Add errorBuilder to main preview images for robustness.Match the thumbnail’s resilience to missing/invalid files to avoid layout exceptions.
- ClipRRect( - child: Image.file( - File(filePath), + ClipRRect( + child: Image.file( + File(filePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => const SizedBox.shrink(), ), ), @@ - (file, originalFilePath) => ClipRRect( - child: Image.file( - File(originalFilePath), + (file, originalFilePath) => ClipRRect( + child: Image.file( + File(originalFilePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => const SizedBox.shrink(), ), ), @@ - ClipRRect( - child: Image.file( - File(filePath), + ClipRRect( + child: Image.file( + File(filePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => const SizedBox.shrink(), ), ),Also applies to: 114-121, 126-133
lib/ui/chat/widgets/media_thumbnail.dart (1)
112-118: Use responsive sizing for progress indicator.Align with ScreenUtil usage elsewhere for consistency.
- width: 18, - height: 18, + width: 18.w, + height: 18.h,lib/ui/chat/states/chat_input_state.freezed.dart (1)
203-205: Optional: avoid logging large lists in toString.If logs get noisy, override toString in the source model to summarize selectedMedia length instead of listing contents. Freezed will honor the custom toString.
lib/config/providers/chat_input_provider.dart (5)
141-141: Use “media” in logs for consistency.Align wording with selectedMedia.
- _logger.severe('Failed to upload image: $filePath', e, st); + _logger.severe('Failed to upload media: $filePath', e, st);
159-166: Type the temporary list explicitly.Satisfy “always declare the type” guideline.
- final updatedMedia = List<MediaFileUpload>.from(state.selectedMedia); + final List<MediaFileUpload> updatedMedia = List<MediaFileUpload>.from(state.selectedMedia);
160-161: Minor: log message wording.It manages media items, not only images.
- _logger.warning('Invalid image index: $index'); + _logger.warning('Invalid media index: $index');
185-186: Prefer const empty list literal.Small perf/readability win and intent clear.
- selectedMedia: [], + selectedMedia: const <MediaFileUpload>[],
110-112: Optional: cap parallel uploads and/or add a timeout.For large selections, consider a small concurrency pool (e.g., 3–4) and wrapping each _uploadMediaFn call with a timeout to avoid indefinite “uploading” states.
test/config/providers/chat_input_provider_test.dart (5)
301-302: Fix variable typo.Rename uploadeddImagePath → uploadedImagePath.
- const uploadeddImagePath = '/path/to/uploaded/image1.jpg'; + const uploadedImagePath = '/path/to/uploaded/image1.jpg'; @@ - filePath: uploadeddImagePath, + filePath: uploadedImagePath, @@ - expect(state.selectedMedia[0].uploadedFile?.filePath, uploadeddImagePath); + expect(state.selectedMedia[0].uploadedFile?.filePath, uploadedImagePath);Also applies to: 336-336
119-146: Dispose temporary ProviderContainers to avoid leaks.Each test creates a fresh ProviderContainer but doesn’t dispose it. Add disposal for these “initial state” tests.
Example pattern per test:
- final testContainer = ProviderContainer(); + final testContainer = ProviderContainer(); + addTearDown(testContainer.dispose);Apply to the four tests in this group.
315-321: Use explicit typing forstateper repo guidelines.Replace inferred
varwithChatInputState.- var state = container.read(chatInputProvider(testGroupId)); + ChatInputState state = container.read(chatInputProvider(testGroupId));Same change applies to other occurrences of
var state.
249-266: Also assert that no upload calls occur without an active account.Strengthens the guard behavior test.
test('keeps selectedMedia empty', () async { await notifier.handleImagesSelected(); final state = container.read(chatInputProvider(testGroupId)); expect(state.selectedMedia, isEmpty); + expect(mockUploadMedia.uploadCalls, isEmpty); });
585-596: Optional: remove real sleeps by using FakeAsync for timer tests.Replace Future.delayed with fakeAsync to make draft-save tests fully deterministic and faster.
I can provide a small FakeAsync-based rewrite if you want to adopt it now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/config/providers/chat_input_provider.dart(3 hunks)lib/ui/chat/states/chat_input_state.dart(2 hunks)lib/ui/chat/states/chat_input_state.freezed.dart(13 hunks)lib/ui/chat/widgets/chat_input.dart(3 hunks)lib/ui/chat/widgets/chat_input_media_preview.dart(5 hunks)lib/ui/chat/widgets/media_thumbnail.dart(1 hunks)test/config/providers/active_account_provider_test.dart(1 hunks)test/config/providers/chat_input_provider_test.dart(8 hunks)test/config/providers/group_messages_provider_test.dart(1 hunks)test/config/providers/group_provider_test.dart(1 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(1 hunks)test/shared/mocks/mock_active_pubkey_notifier.dart(1 hunks)test/shared/mocks/mock_draft_message_service.dart(1 hunks)test/shared/mocks/mock_image_picker_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/shared/mocks/mock_image_picker_service.dart
- test/config/providers/profile_ready_card_visibility_provider_test.dart
- test/config/providers/group_provider_test.dart
- test/config/providers/group_messages_provider_test.dart
- test/config/providers/active_account_provider_test.dart
- test/shared/mocks/mock_draft_message_service.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value)
Avoid using dynamic and Object without justification
Create necessary types instead of overusing primitives or dynamic
One export per file
Use PascalCase for classes
Use camelCase for variables, functions, and methods
Avoid magic numbers and define constants
Start each function name with a verb
Use verbs for boolean variables (e.g., isLoading, hasError, canDelete)
Write short functions with a single purpose (under ~20 instructions)
Name functions with a verb plus context; for booleans use isX/hasX/canX; for void use executeX/saveX
Avoid deep nesting via early returns and extraction to utility functions
Use higher-order functions (map, where/filter, reduce) to avoid nesting
Use arrow functions for simple functions (under ~3 statements); use named functions otherwise
Use default parameter values instead of null checks
Reduce function parameters using RO-RO: pass/return parameter objects with declared types
Maintain a single level of abstraction within functions
Encapsulate data in composite types; avoid overusing primitives
Prefer validating data within classes rather than in functions
Prefer immutability; use final for runtime constants and const for compile-time constants
Use const constructors and const literals where possible
Follow SOLID principles
Prefer composition over inheritance
Declare interfaces (abstract classes) to define contracts
Write small classes with a single purpose (under ~200 instructions, <10 public methods, <10 properties)
Use exceptions for unexpected errors
Only catch exceptions to fix expected problems or add context; otherwise use a global handler
Files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/widgets/chat_input_media_preview.dartlib/ui/chat/widgets/chat_input.dartlib/config/providers/chat_input_provider.darttest/config/providers/chat_input_provider_test.dartlib/ui/chat/widgets/media_thumbnail.dartlib/ui/chat/states/chat_input_state.freezed.darttest/shared/mocks/mock_active_pubkey_notifier.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
lib/**/*.dart: Use flutter_rust_bridge to access core app functionality
Use Riverpod for state management; prefer StreamProviders for Rust API streams; use keepAlive if needed
Use freezed to model/manage UI states
Controllers should expose methods as inputs and update UI state that drives the UI
Use AutoRoute for navigation and use extras to pass data between pages
Use Dart extensions to manage reusable code
Use ThemeData to manage themes
Use AppLocalizations for translations
Use constants to manage constant values
Avoid deeply nested widget trees; aim for a flatter widget structure for performance and readability
Break down large widgets into smaller, focused, reusable components
Keep the widget tree shallow to simplify state management and data flow
Utilize const constructors and const widgets wherever possible to reduce rebuilds
Files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/widgets/chat_input_media_preview.dartlib/ui/chat/widgets/chat_input.dartlib/config/providers/chat_input_provider.dartlib/ui/chat/widgets/media_thumbnail.dartlib/ui/chat/states/chat_input_state.freezed.dart
test/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
test/**/*.dart: Follow Arrange-Act-Assert in tests
Name test variables clearly using inputX, mockX, actualX, expectedX
Write unit tests for each public function; use test doubles for dependencies (except cheap third-party)
Use standard Flutter widget testing
Files:
test/config/providers/chat_input_provider_test.darttest/shared/mocks/mock_active_pubkey_notifier.dart
🧠 Learnings (4)
📚 Learning: 2025-10-15T02:08:41.843Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#721
File: lib/ui/chat/widgets/chat_input.dart:80-87
Timestamp: 2025-10-15T02:08:41.843Z
Learning: In `lib/ui/chat/widgets/chat_input.dart`, the `_toggleMediaSelector()` method correctly reads `chatInputState` before toggling and checks `!chatInputState.showMediaSelector` to unfocus when opening the selector. This matches the design requirement where the input should be unfocused when the media selector is shown.
Applied to files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/widgets/chat_input.darttest/config/providers/chat_input_provider_test.dartlib/ui/chat/states/chat_input_state.freezed.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to lib/**/*.dart : Use freezed to model/manage UI states
Applied to files:
lib/ui/chat/states/chat_input_state.dartlib/ui/chat/states/chat_input_state.freezed.dart
📚 Learning: 2025-10-24T13:53:05.425Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#740
File: lib/domain/models/media_file_upload.dart:30-34
Timestamp: 2025-10-24T13:53:05.425Z
Learning: In the MediaFileUpload model (lib/domain/models/media_file_upload.dart), the originalFilePath getter correctly returns the local file path for all states: during uploading and failed states the path has not changed (upload didn't succeed), and only in the uploaded state does the file get a new remote path, with originalFilePath preserving the original local path. This design prevents UI blinking during state transitions.
Applied to files:
lib/ui/chat/widgets/media_thumbnail.dart
📚 Learning: 2025-09-08T17:39:13.711Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#599
File: test/ui/contact_list/start_chat_bottom_sheet_test.dart:173-176
Timestamp: 2025-09-08T17:39:13.711Z
Learning: In the whitenoise Flutter app, the followProvider uses followsProvider behind the scenes, so mocking followsProvider with MockFollowsNotifier effectively controls what followProvider sees in tests.
Applied to files:
test/shared/mocks/mock_active_pubkey_notifier.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: Flutter CI
🔇 Additional comments (5)
lib/ui/chat/widgets/chat_input.dart (1)
80-87: Media selector toggle/unfocus flow is correct.Reading state before toggling and unfocusing only when opening matches the UX requirement. Nicely done.
Based on learnings
test/shared/mocks/mock_active_pubkey_notifier.dart (1)
3-18: LGTM: mock correctly updates field and state.Shadowing issue is resolved; build() and setActivePubkey() behavior is consistent for tests.
lib/ui/chat/widgets/media_thumbnail.dart (1)
10-46: State‑driven thumbnail rendering looks solid.Good use of MediaFileUpload.when, disabled taps while uploading, and clear overlays.
lib/ui/chat/states/chat_input_state.freezed.dart (1)
22-23: State field migration looks correct.selectedImages → selectedMedia is consistently applied with proper typing and immutability wrappers.
lib/config/providers/chat_input_provider.dart (1)
18-28: Good DI for upload function.Injectable uploadMediaFn with a sensible default keeps tests clean and production flexible. LGTM.
Also applies to: 34-40
josefinalliende
force-pushed
the
feat/upload-images
branch
from
3ca9821 to
8ff50e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/config/providers/chat_input_provider_test.dart (1)
87-90: Deterministic upload wait looks good.Yielding with Duration.zero is appropriate since the mock completes synchronously.
🧹 Nitpick comments (11)
lib/config/providers/chat_input_provider.dart (4)
18-24: Introduce a typedef for the upload function to simplify signatures.Improves readability and reuse across constructor/field.
Apply this diff to update the parameter/field types:
- Future<rust_media_files.MediaFile> Function({ + UploadMediaFn? uploadMediaFn, - })? - uploadMediaFn, ... - final Future<rust_media_files.MediaFile> Function({ - required String accountPubkey, - required String groupId, - required String filePath, - }) - _uploadMediaFn; + final UploadMediaFn _uploadMediaFn;Add this typedef near the imports:
typedef UploadMediaFn = Future<rust_media_files.MediaFile> Function({ required String accountPubkey, required String groupId, required String filePath, });Also applies to: 34-40
86-92: Log when skipping uploads due to missing pubkey.Helps diagnostics in support logs without changing behavior.
if (accountHexPubkey.isEmpty) { + _logger.warning('Skip media selection: missing/invalid active pubkey for group $_groupId'); state = state.copyWith(showMediaSelector: false); return; }
158-166: Consider renaming for clarity: removeImage → removeMediaAt.Aligns method name with
selectedMedia.
178-186: Use a typed const empty list on clear.Minor alloc/immutability win and clearer intent.
state = state.copyWith( showMediaSelector: false, isLoadingDraft: false, previousEditingMessageContent: null, - selectedMedia: [], + selectedMedia: const <MediaFileUpload>[], );lib/ui/chat/widgets/chat_input.dart (1)
276-281: Naming nit: hasImages now represents media presence.Consider renaming the prop to
hasMediain a follow-up to matchselectedMedia(cross-file change).lib/ui/chat/widgets/chat_input_media_preview.dart (2)
84-147: Add errorBuilder to Image.file to avoid red-screen on missing/unreadable files.Improves robustness without changing UX.
- Image.file( + Image.file( File(filePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => Container( + height: _imageHeight.h, + width: _imageWidth.w, + color: context.colors.solidNeutralBlack.withValues(alpha: 0.2), + alignment: Alignment.center, + child: WnImage(AssetsPaths.icErrorFilled, color: context.colors.destructive, size: 32.w), + ), ) ... - Image.file( + Image.file( File(originalFilePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => Container( + height: _imageHeight.h, + width: _imageWidth.w, + color: context.colors.solidNeutralBlack.withValues(alpha: 0.2), + ), ) ... - Image.file( + Image.file( File(filePath), height: _imageHeight.h, width: _imageWidth.w, fit: BoxFit.cover, + errorBuilder: (_, __, ___) => Container( + height: _imageHeight.h, + width: _imageWidth.w, + color: context.colors.solidNeutralBlack.withValues(alpha: 0.2), + ), )
77-96: Provide stable Keys for list items (supports duplicates).Helps Flutter keep item identity when the same path appears multiple times and when removing items.
- return mediaItem.when( + return KeyedSubtree( + key: mediaItem.when( + uploading: (path) => ValueKey<String>('uploading:$path#$index'), + uploaded: (_, originalPath) => ValueKey<String>('uploaded:$originalPath#$index'), + failed: (path, _) => ValueKey<String>('failed:$path#$index'), + ), + child: mediaItem.when( uploading: (filePath) => Stack( ... ), uploaded: (file, originalFilePath) => ClipRRect( ... ), failed: (filePath, error) => Stack( ... ), - ); + ), + );And for thumbnails:
- return MediaThumbnail( + return KeyedSubtree( + key: ValueKey<String>('thumb:$itemIndex'), + child: MediaThumbnail( mediaItem: mediaItem, isActive: _activeThumbIndex == itemIndex, onTap: () => _handleThumbnailTap(itemIndex), - ); + ), + );Also applies to: 150-181
lib/ui/chat/widgets/media_thumbnail.dart (3)
18-21: Extract sizes into constants and use .w/.h consistently; drop redundant overlay sizing.Removes magic numbers (32/18/14), fixes raw 18 vs 18.w/h, and relies on Positioned.fill instead of width/height inside overlays. As per coding guidelines
class MediaThumbnail extends StatelessWidget { const MediaThumbnail({ super.key, required this.mediaItem, required this.isActive, required this.onTap, }); final MediaFileUpload mediaItem; final bool isActive; final VoidCallback onTap; + + // Sizes + static const double _thumbSize = 32; + static const double _overlayIconSize = 18; + static const double _errorIconSize = 14; @@ child: Image.file( File(filePath), - height: 32.h, - width: 32.w, + height: _thumbSize.h, + width: _thumbSize.w, fit: BoxFit.cover, errorBuilder: (context, error, stackTrace) => const SizedBox.shrink(), ), ), ), @@ Widget _uploadedOverlay(BuildContext context) { return Positioned.fill( child: Center( child: Container( - width: 32.w, - height: 32.h, decoration: BoxDecoration( color: context.colors.solidNeutralBlack.withValues(alpha: 0.5), ), child: Center( child: SizedBox( - width: 18.w, - height: 18.h, + width: _overlayIconSize.w, + height: _overlayIconSize.h, child: WnImage( AssetsPaths.icTrashCan, color: context.colors.solidNeutralWhite, ), ), ), ), ), ); } @@ Widget _uploadingOverlay(BuildContext context) { return Positioned.fill( child: Container( - width: 32.w, - height: 32.h, decoration: BoxDecoration( color: context.colors.solidNeutralBlack.withValues(alpha: 0.5), ), child: Center( child: SizedBox( - width: 18, - height: 18, + width: _overlayIconSize.w, + height: _overlayIconSize.h, child: CircularProgressIndicator( strokeWidth: 2, color: context.colors.solidNeutralWhite, ), ), ), ), ); } @@ Widget _failedOverlay(BuildContext context) { return Positioned.fill( child: Container( - width: 32.w, - height: 32.h, decoration: BoxDecoration( color: context.colors.solidNeutralBlack.withValues(alpha: 0.5), ), child: Center( child: WnImage( AssetsPaths.icErrorFilled, color: context.colors.destructive, - size: 14.w, + size: _errorIconSize.w, ), ), ), ); }Also applies to: 66-68, 82-84, 89-91, 105-107, 112-114, 127-129, 136-137
64-70: Add cacheWidth/cacheHeight and gaplessPlayback to Image.file for perf and visual stability.Reduces decode/memory cost and avoids brief flicker on rebuilds.
child: Image.file( File(filePath), height: _thumbSize.h, width: _thumbSize.w, fit: BoxFit.cover, + gaplessPlayback: true, + cacheWidth: (_thumbSize.w * MediaQuery.of(context).devicePixelRatio).round(), + cacheHeight: (_thumbSize.h * MediaQuery.of(context).devicePixelRatio).round(), errorBuilder: (context, error, stackTrace) => const SizedBox.shrink(), ),
102-122: Add basic semantics to overlays for accessibility.Expose state to assistive tech; optional to wrap overlays with IgnorePointer to guarantee taps bubble to parent.
Widget _uploadingOverlay(BuildContext context) { - return Positioned.fill( - child: Container( + return Positioned.fill( + child: Semantics( + label: 'Uploading', + child: Container( // ... - ), - ), + ), + ), ); } Widget _failedOverlay(BuildContext context) { - return Positioned.fill( - child: Container( + return Positioned.fill( + child: Semantics( + label: 'Upload failed', + child: Container( // ... - ), - ), + ), + ), ); }Also applies to: 124-141
test/config/providers/chat_input_provider_test.dart (1)
12-16: Optional: make uploadCalls typed (records) instead of Map to reduce test bugs.Improves readability and avoids key typos.
class MockUploadMediaFn { final Map<String, rust_media_files.MediaFile> _uploadResults = {}; final List<String> _failingPaths = []; - final List<Map<String, String>> _uploadCalls = []; + final List<({String accountPubkey, String groupId, String filePath})> _uploadCalls = []; @@ - List<Map<String, String>> get uploadCalls => _uploadCalls; + List<({String accountPubkey, String groupId, String filePath})> get uploadCalls => _uploadCalls; @@ - _uploadCalls.add({ - 'accountPubkey': accountPubkey, - 'groupId': groupId, - 'filePath': filePath, - }); + _uploadCalls.add((accountPubkey: accountPubkey, groupId: groupId, filePath: filePath));And in the assertion:
- final call = mockUploadMedia.uploadCalls[0]; - expect(call['groupId'], testGroupId); - expect(call['filePath'], imagePath); - expect(call['accountPubkey'], testAccountPubkey); + final call = mockUploadMedia.uploadCalls[0]; + expect(call.groupId, testGroupId); + expect(call.filePath, imagePath); + expect(call.accountPubkey, testAccountPubkey);Also applies to: 25-36, 426-431
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/config/providers/chat_input_provider.dart(3 hunks)lib/ui/chat/states/chat_input_state.dart(2 hunks)lib/ui/chat/states/chat_input_state.freezed.dart(13 hunks)lib/ui/chat/widgets/chat_input.dart(3 hunks)lib/ui/chat/widgets/chat_input_media_preview.dart(5 hunks)lib/ui/chat/widgets/media_thumbnail.dart(1 hunks)test/config/providers/chat_input_provider_test.dart(8 hunks)test/shared/mocks/mock_draft_message_service.dart(1 hunks)test/shared/mocks/mock_image_picker_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/shared/mocks/mock_image_picker_service.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value)
Avoid using dynamic and Object without justification
Create necessary types instead of overusing primitives or dynamic
One export per file
Use PascalCase for classes
Use camelCase for variables, functions, and methods
Avoid magic numbers and define constants
Start each function name with a verb
Use verbs for boolean variables (e.g., isLoading, hasError, canDelete)
Write short functions with a single purpose (under ~20 instructions)
Name functions with a verb plus context; for booleans use isX/hasX/canX; for void use executeX/saveX
Avoid deep nesting via early returns and extraction to utility functions
Use higher-order functions (map, where/filter, reduce) to avoid nesting
Use arrow functions for simple functions (under ~3 statements); use named functions otherwise
Use default parameter values instead of null checks
Reduce function parameters using RO-RO: pass/return parameter objects with declared types
Maintain a single level of abstraction within functions
Encapsulate data in composite types; avoid overusing primitives
Prefer validating data within classes rather than in functions
Prefer immutability; use final for runtime constants and const for compile-time constants
Use const constructors and const literals where possible
Follow SOLID principles
Prefer composition over inheritance
Declare interfaces (abstract classes) to define contracts
Write small classes with a single purpose (under ~200 instructions, <10 public methods, <10 properties)
Use exceptions for unexpected errors
Only catch exceptions to fix expected problems or add context; otherwise use a global handler
Files:
test/shared/mocks/mock_draft_message_service.darttest/config/providers/chat_input_provider_test.dartlib/ui/chat/widgets/chat_input_media_preview.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/states/chat_input_state.dartlib/config/providers/chat_input_provider.dartlib/ui/chat/widgets/chat_input.dartlib/ui/chat/widgets/media_thumbnail.dart
test/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
test/**/*.dart: Follow Arrange-Act-Assert in tests
Name test variables clearly using inputX, mockX, actualX, expectedX
Write unit tests for each public function; use test doubles for dependencies (except cheap third-party)
Use standard Flutter widget testing
Files:
test/shared/mocks/mock_draft_message_service.darttest/config/providers/chat_input_provider_test.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
lib/**/*.dart: Use flutter_rust_bridge to access core app functionality
Use Riverpod for state management; prefer StreamProviders for Rust API streams; use keepAlive if needed
Use freezed to model/manage UI states
Controllers should expose methods as inputs and update UI state that drives the UI
Use AutoRoute for navigation and use extras to pass data between pages
Use Dart extensions to manage reusable code
Use ThemeData to manage themes
Use AppLocalizations for translations
Use constants to manage constant values
Avoid deeply nested widget trees; aim for a flatter widget structure for performance and readability
Break down large widgets into smaller, focused, reusable components
Keep the widget tree shallow to simplify state management and data flow
Utilize const constructors and const widgets wherever possible to reduce rebuilds
Files:
lib/ui/chat/widgets/chat_input_media_preview.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/states/chat_input_state.dartlib/config/providers/chat_input_provider.dartlib/ui/chat/widgets/chat_input.dartlib/ui/chat/widgets/media_thumbnail.dart
🧠 Learnings (5)
📚 Learning: 2025-10-15T02:08:41.843Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#721
File: lib/ui/chat/widgets/chat_input.dart:80-87
Timestamp: 2025-10-15T02:08:41.843Z
Learning: In `lib/ui/chat/widgets/chat_input.dart`, the `_toggleMediaSelector()` method correctly reads `chatInputState` before toggling and checks `!chatInputState.showMediaSelector` to unfocus when opening the selector. This matches the design requirement where the input should be unfocused when the media selector is shown.
Applied to files:
test/config/providers/chat_input_provider_test.dartlib/ui/chat/states/chat_input_state.freezed.dartlib/ui/chat/states/chat_input_state.dartlib/ui/chat/widgets/chat_input.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to lib/**/*.dart : Use freezed to model/manage UI states
Applied to files:
lib/ui/chat/states/chat_input_state.dart
📚 Learning: 2025-10-24T17:31:14.169Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#728
File: lib/config/providers/chat_input_provider.dart:94-112
Timestamp: 2025-10-24T17:31:14.169Z
Learning: In the whitenoise_flutter project's chat input provider, duplicate image selections should be allowed (not deduplicated) when a user picks images, as users may intentionally want to send the same image multiple times.
Applied to files:
lib/config/providers/chat_input_provider.dart
📚 Learning: 2025-10-24T17:30:23.117Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#728
File: lib/config/providers/chat_input_provider.dart:94-112
Timestamp: 2025-10-24T17:30:23.117Z
Learning: In the whitenoise_flutter project, the team prefers to use type inference for local variables in Dart code rather than explicit type annotations.
Applied to files:
lib/config/providers/chat_input_provider.dart
📚 Learning: 2025-10-24T13:53:05.425Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#740
File: lib/domain/models/media_file_upload.dart:30-34
Timestamp: 2025-10-24T13:53:05.425Z
Learning: In the MediaFileUpload model (lib/domain/models/media_file_upload.dart), the originalFilePath getter correctly returns the local file path for all states: during uploading and failed states the path has not changed (upload didn't succeed), and only in the uploaded state does the file get a new remote path, with originalFilePath preserving the original local path. This design prevents UI blinking during state transitions.
Applied to files:
lib/ui/chat/widgets/media_thumbnail.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: Flutter CI
🔇 Additional comments (4)
lib/ui/chat/states/chat_input_state.dart (1)
11-14: State refactor to MediaFileUpload looks solid.Field rename and typing align with the new upload flow and Freezed patterns. LGTM.
test/shared/mocks/mock_draft_message_service.dart (1)
4-35: Mock is clean and fit for purpose.Clear overrides and reset. No issues.
lib/config/providers/chat_input_provider.dart (1)
120-156: ref.mounted does not exist in Riverpod 2.6.1—this fix will not compile.The review comment proposes using
ref.mounted, but ref.mounted was added in Riverpod 3.0 and shipped on September 10, 2025. Your codebase usesflutter_riverpod: ^2.6.1, which predates this feature. The check will fail at compile time.The concern itself is valid: state updates after disposal must be guarded. For Riverpod 2.6, the correct pattern is to use ref.onDispose to cancel async work or flip a local mounted flag and check it before assigning state.
Your notifier already has
ref.onDispose(() { _draftSaveTimer?.cancel(); }). To safely guard the async uploads, add a_mountedboolean field, set it to false on dispose, and check it before state mutations—or upgrade to Riverpod 3.0+ and use the suggestedref.mountedapproach.Likely an incorrect or invalid review comment.
lib/ui/chat/widgets/media_thumbnail.dart (1)
25-45: State-driven rendering and tap gating look solid.Using mediaItem.when with originalFilePath in the uploaded branch avoids preview flicker; disabling onTap while uploading prevents accidental actions. Based on learnings
josefinalliende
requested review from
Quwaysim,
codeswot,
erskingardner and
untreu2
codeswot
left a comment
codeswot
left a comment
There was a problem hiding this comment.
Ran the app on Physical android and ios Sim.
works great, I think the optimistic load can be improved. just as we do for text (show the local path while media is being uploaded to blossom)
@codeswot The local path is shown while uploading, which is why the image is visible when the spinner is loading. So I think I might not fully understand what the suggestion refers to. What I meant by optimistic is that the image is actually uploaded to the Blossom server before the message is sent — assuming the selected image will be included in the message. In the future, the plan is to handle the case where we remove the image from the Blossom server if it’s not eventually sent. If you mean showing the image in the message bubble, that’ll be handled in another PR. The next one will focus on sending the media in the Nostr event — displaying it in the message bubble will come afterward. |
That's why I added the warning in the PR description |
|
More detail in case is not clear. The file path, for both media file uploads that are uploading or failed is the original local path. In the uploaded one the file path is the new one (which es local too), but the original file path is for the purpose of avoid the "blink" when it changes from uploading to uploaded |
Description
This PR uses the
MediaFileUploadclass and the rust method to upload media files to blossom servers in tha chat input provider. Adapts the UI to show loading and error.ScreenRecording_10-24-2025.12-58-15_1.MP4
Commit by commit:
Next step ➡️ : add media tags to event
Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit
New Features
Bug Fixes
Tests