Feat/images modal #768
Feat/images modal #768
Conversation
WalkthroughThis PR introduces new Flutter widgets for media display and viewing in chat messages. It adds MediaImage (renders local files or blurhash placeholders), MediaModal (full-screen media viewer with pagination and thumbnails), and MediaThumbnail (thumbnail indicators). MessageMediaGrid and MessageWidget are updated to wire tap events to the modal. WnDialog gains an optional backgroundColor property. Comprehensive test coverage is included for all new widgets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageMediaGrid
participant MessageWidget
participant MediaModal
participant PageView
participant MediaImage
User->>MessageMediaGrid: Tap media item (index)
MessageMediaGrid->>MessageWidget: onMediaTap(index)
MessageWidget->>MediaModal: showDialog(mediaFiles, initialIndex, senderInfo, timestamp)
activate MediaModal
MediaModal->>PageView: initPageController(initialIndex)
PageView->>MediaImage: request page render
MediaImage->>MediaImage: _hasLocalFile() check
alt local file exists
MediaImage-->>PageView: render Image.file(...)
else fallback
MediaImage-->>PageView: render BlurhashPlaceholder(hash)
end
User->>PageView: swipe or tap thumbnail -> PageView changes page
PageView->>MediaImage: render new page
User->>MediaModal: tap close
MediaModal-->>MessageWidget: dismiss dialog
deactivate MediaModal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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/show-images-in-messages
branch
from
b3076b1 to
cfc47d7
Compare
josefinalliende
changed the base branch from
master
to
fix/image-preview-scroll
josefinalliende
force-pushed
the
feat/images-modal
branch
from
6733f55 to
4b6ae5d
Compare
josefinalliende
force-pushed
the
feat/images-modal
branch
from
4b6ae5d to
450af5e
Compare
josefinalliende
force-pushed
the
feat/images-modal
branch
from
450af5e to
7286037
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: 1
🧹 Nitpick comments (2)
lib/ui/chat/widgets/media_thumbnail.dart (1)
63-71: Consider extracting shared file-checking logic.The
_hasLocalFile()method is duplicated betweenMediaImageandMediaThumbnail. While the duplication is minimal, consider extracting this to a shared utility function to maintain consistency and simplify future updates.lib/ui/chat/widgets/message_media_grid.dart (1)
16-16: Consider usingValueChanged<int>for callback type.The callback type
Function(int index)?is valid butValueChanged<int>?is more idiomatic in Flutter for single-parameter callbacks. Alternatively, consider defining a typedef for better clarity.Apply this diff to use the more idiomatic type:
- final Function(int index)? onMediaTap; + final ValueChanged<int>? onMediaTap;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/ui/chat/widgets/media_image.dart(1 hunks)lib/ui/chat/widgets/media_modal.dart(1 hunks)lib/ui/chat/widgets/media_thumbnail.dart(1 hunks)lib/ui/chat/widgets/message_media_grid.dart(3 hunks)lib/ui/chat/widgets/message_widget.dart(3 hunks)lib/ui/core/ui/wn_dialog.dart(2 hunks)test/ui/chat/widgets/media_image_test.dart(1 hunks)test/ui/chat/widgets/media_modal_test.dart(1 hunks)test/ui/chat/widgets/media_thumbnail_test.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:
lib/ui/core/ui/wn_dialog.darttest/ui/chat/widgets/media_modal_test.dartlib/ui/chat/widgets/media_image.dartlib/ui/chat/widgets/message_media_grid.darttest/ui/chat/widgets/media_image_test.dartlib/ui/chat/widgets/message_widget.dartlib/ui/chat/widgets/media_modal.darttest/ui/chat/widgets/media_thumbnail_test.dartlib/ui/chat/widgets/media_thumbnail.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/core/ui/wn_dialog.dartlib/ui/chat/widgets/media_image.dartlib/ui/chat/widgets/message_media_grid.dartlib/ui/chat/widgets/message_widget.dartlib/ui/chat/widgets/media_modal.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/ui/chat/widgets/media_modal_test.darttest/ui/chat/widgets/media_image_test.darttest/ui/chat/widgets/media_thumbnail_test.dart
🧠 Learnings (11)
📓 Common learnings
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 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.
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 728
File: lib/config/providers/chat_input_provider.dart:94-112
Timestamp: 2025-10-24T17:31:14.191Z
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.
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
Repo: parres-hq/whitenoise_flutter PR: 0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to test/**/*.dart : Use standard Flutter widget testing
Applied to files:
test/ui/chat/widgets/media_modal_test.darttest/ui/chat/widgets/media_image_test.darttest/ui/chat/widgets/media_thumbnail_test.dart
📚 Learning: 2025-10-15T02:08:41.843Z
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 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/ui/chat/widgets/media_modal_test.dartlib/ui/chat/widgets/media_image.dartlib/ui/chat/widgets/message_media_grid.darttest/ui/chat/widgets/media_image_test.dartlib/ui/chat/widgets/message_widget.dartlib/ui/chat/widgets/media_modal.darttest/ui/chat/widgets/media_thumbnail_test.dartlib/ui/chat/widgets/media_thumbnail.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
Repo: parres-hq/whitenoise_flutter PR: 0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to test/**/*.dart : Follow Arrange-Act-Assert in tests
Applied to files:
test/ui/chat/widgets/media_modal_test.darttest/ui/chat/widgets/media_image_test.darttest/ui/chat/widgets/media_thumbnail_test.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
Repo: parres-hq/whitenoise_flutter PR: 0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to test/**/*.dart : Write unit tests for each public function; use test doubles for dependencies (except cheap third-party)
Applied to files:
test/ui/chat/widgets/media_modal_test.darttest/ui/chat/widgets/media_image_test.darttest/ui/chat/widgets/media_thumbnail_test.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
Repo: parres-hq/whitenoise_flutter PR: 0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to integration_test/**/*.dart : Use integration tests for each API module
Applied to files:
test/ui/chat/widgets/media_modal_test.dart
📚 Learning: 2025-10-24T13:53:05.455Z
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 740
File: lib/domain/models/media_file_upload.dart:30-34
Timestamp: 2025-10-24T13:53:05.455Z
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_image.dartlib/ui/chat/widgets/media_thumbnail.dart
📚 Learning: 2025-10-24T17:31:14.191Z
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 728
File: lib/config/providers/chat_input_provider.dart:94-112
Timestamp: 2025-10-24T17:31:14.191Z
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/ui/chat/widgets/media_image.dartlib/ui/chat/widgets/message_widget.dart
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
Repo: parres-hq/whitenoise_flutter PR: 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/widgets/message_widget.dart
📚 Learning: 2025-08-23T11:02:28.308Z
Learnt from: Quwaysim
Repo: parres-hq/whitenoise_flutter PR: 527
File: lib/ui/core/ui/wn_avatar.dart:1-6
Timestamp: 2025-08-23T11:02:28.308Z
Learning: In the whitenoise_flutter codebase, flutter_screenutil is re-exported through lib/ui/core/themes/src/app_theme.dart, so files that import app_theme.dart do not need to directly import flutter_screenutil to use .w, .h, .sp extensions.
Applied to files:
lib/ui/chat/widgets/message_widget.dart
📚 Learning: 2025-09-01T14:56:50.988Z
Learnt from: josefinalliende
Repo: parres-hq/whitenoise_flutter PR: 568
File: lib/ui/core/ui/wn_image.dart:1-2
Timestamp: 2025-09-01T14:56:50.988Z
Learning: The whitenoise_flutter project does not target web platforms and any future web version would be in a separate repository, so dart:io imports and file system operations are acceptable in this codebase.
Applied to files:
lib/ui/chat/widgets/message_widget.dart
🔇 Additional comments (4)
lib/ui/chat/widgets/media_modal.dart (4)
68-82: Verify thumbnail scroll positioning matches design intent.The scroll position calculation on line 71 scrolls to the left edge of the active thumbnail rather than centering it in the viewport. While this ensures visibility, it may not provide the optimal UX if the design expects centered thumbnails.
Run the app and verify that the thumbnail scrolling behavior aligns with the Figma design specifications. If centering is required, consider calculating the offset to position the active thumbnail in the center of the visible strip.
194-213: Clean thumbnail strip implementation.The thumbnail strip correctly handles horizontal scrolling, active state highlighting, and tap navigation. The conditional rendering (line 113) ensures it only displays when multiple media files are present.
42-59: Proper lifecycle management.Controllers are correctly initialized in
initStateand disposed indispose. The use ofaddPostFrameCallbackensures the initial thumbnail scroll occurs after the widget tree is built and the scroll controller has clients.
153-153: Use localized date formatting instead of hardcoded format.The date format
'dd/MM/yyyy - HH:mm'is hardcoded, which violates the guideline to useAppLocalizationsfor translations and locale-specific formatting. Different locales have different conventions for date/time display.As per coding guidelines, use Flutter's localized date formatting or define the format string in your localization files:
// Example using localized formatting Text( DateFormat.yMd().add_Hm().format(widget.timestamp.toLocal()), style: TextStyle( fontSize: 12.sp, fontWeight: FontWeight.w600, color: context.colors.mutedForeground, ), ),Alternatively, define the format pattern in your
AppLocalizationsto allow different patterns per locale.⛔ Skipped due to learnings
Learnt from: codeswot Repo: parres-hq/whitenoise_flutter PR: 595 File: lib/ui/settings/profile/share_profile_qr_scan_screen.dart:73-90 Timestamp: 2025-09-06T09:32:55.455Z Learning: The whitenoise_flutter project does not use localization (AppLocalizations) - hard-coded strings are acceptable and should not be flagged for localization.Learnt from: CR Repo: parres-hq/whitenoise_flutter PR: 0 File: .cursor/rules/flutter.mdc:0-0 Timestamp: 2025-10-12T22:44:49.606Z Learning: Applies to lib/**/*.dart : Use AppLocalizations for translations
| const MediaModal({ | ||
| super.key, | ||
| required this.mediaFiles, | ||
| required this.initialIndex, | ||
| required this.senderName, | ||
| required this.senderImagePath, | ||
| required this.timestamp, | ||
| }); | ||
|
|
||
| final List<MediaFile> mediaFiles; | ||
| final int initialIndex; | ||
| final String senderName; | ||
| final String? senderImagePath; | ||
| final DateTime timestamp; |
There was a problem hiding this comment.
Add validation for initialIndex bounds and mediaFiles non-empty constraint.
The constructor does not validate that initialIndex is within the bounds of mediaFiles or that mediaFiles is non-empty. This can lead to runtime errors when PageController is initialized with an invalid initialPage or when the PageView.builder attempts to render with an empty list.
Apply this diff to add assertions in debug mode:
class MediaModal extends StatefulWidget {
const MediaModal({
super.key,
required this.mediaFiles,
required this.initialIndex,
required this.senderName,
required this.senderImagePath,
required this.timestamp,
- });
+ }) : assert(mediaFiles.isNotEmpty, 'mediaFiles must not be empty'),
+ assert(initialIndex >= 0 && initialIndex < mediaFiles.length,
+ 'initialIndex must be within bounds of mediaFiles');
final List<MediaFile> mediaFiles;🤖 Prompt for AI Agents
In lib/ui/chat/widgets/media_modal.dart around lines 15 to 28, add constructor
assertions to validate inputs: assert mediaFiles.isNotEmpty and
assert(initialIndex >= 0 && initialIndex < mediaFiles.length) so that mediaFiles
is non-empty and initialIndex is within bounds; if using PageController with
initialPage, ensure it uses the validated initialIndex. These assertions should
be placed in the constructor parameter list (or initializer list) so they run in
debug mode and prevent runtime errors when building the PageView.
|
When I send a message with photos that have different aspect ratios, this happens. The one of them has cropped and I cannot see the uncropped version of the other one. This is about @josefinalliende's code or @vladimir-krstic's design? Because when I tap, I should see uncropped/full version, that's why I tapped on it. |
When keeping the aspect ratio by replacing the boxfit .cover by .contain it bothered me a bit that in horizontal pictures there was an empty space between the image and the thumbnails at the bottom... but it's true that is also bothering not to see the whole picture when tapping. @vladimir-krstic can you please choose which option seems better to you? Or maybe we need a change in the design? contain.MP4cover.MP4 |
I prefer option B (cover). i will suggest maybe if the image is tapped it can show a full preview with black background similar to signal |
|
Option B is useless, let’s say I sent an important paper to you, you won’t even be able to see the whole content. So why we have an image modal? |
|
This is not related to this PR so you can ignore. But this is not okay too. The main purpose of tapping on a picture is seeing the whole picture without any distractions. |
|
And I definitely don’t want to delay the release. So if you’re going to change this design, we can make a temporary decision too. We can update that in the next release. @vladimir-krstic Because if it’s a design choice instead of a bug, @josefinalliende’s PR should be approved and does the job. |
I dont't understand what do you find that is not ok about the small squares @untreu2 ? In the deign is like that, or I don't notice the difference... You don't like about the design that the small versions are over the image? |
|
Those small squares should be out of the picture so I can see the whole picture. As I said, this is the purpose of image modal. I should see full and uncropped version of the picture. |
|
This is okay for now 💯 LGTM. |
Description
This PR adds the modal that shows the images from a message when tapped.
Images.modal.MP4
Figma: https://www.figma.com/design/dGJqv6m6pchdcwxJEwxxzB/White-Noise---Design-System?node-id=3649-46190&t=GRk6JaDluufv108A-0
Commit by commit:
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