Skip to content

Conversation

@josefinalliende
Copy link
Contributor

@josefinalliende josefinalliende commented Oct 21, 2025

Description

We are adding media support, starting by images. This PR updates the whitenoise crate and adds the media_files.rs 🦀 to add the upload chat media method, that is needed in #728 .

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore
  • 🧪 Tests

Checklist

  • Run just precommit to ensure that formatting and linting are correct
  • Run just check-flutter-coverage to ensure that flutter coverage rules are passing
  • Updated the CHANGELOG.md file with your changes (if they affect the user experience)

Summary by CodeRabbit

  • New Features
    • Chat media upload for group conversations — users can upload files directly to group chats.
    • Uploads now return a structured media record including file ID, access URL, MIME type, media type, upload timestamp, and optional metadata (original filename, dimensions, blurhash) for richer display and handling across platforms.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Adds end-to-end chat media upload: new Rust API and types (MediaFile, FileMetadata), Rust↔Dart bridge and FFI wiring, SSE/DCO (de)serializers for new types, dispatcher/funcId updates, and a whitenoise dependency revision plus a small groups call change.

Changes

Cohort / File(s) Summary
Dart API
lib/src/rust/api/media_files.dart
Adds FileMetadata and MediaFile data classes and Future<MediaFile> uploadChatMedia({required String accountPubkey, required String groupId, required String filePath}) delegating to Rust.
Dart generated FFI surface
lib/src/rust/frb_generated.dart, lib/src/rust/frb_generated.io.dart
Imports api/media_files.dart; adds crateApiMediaFilesUploadChatMedia API and TaskConstMeta; updates rustContentHash; adds DCO/SSE encode/decode helpers and opt/box variants for MediaFile and FileMetadata.
Rust API module
rust/src/api/media_files.rs, rust/src/api/mod.rs
New public structs FileMetadata and MediaFile, From conversions from Whitenoise types, and async upload_chat_media(account_pubkey, group_id, file_path) returning MediaFile; module exported in mod.rs.
Rust FFI & generated
rust/src/frb_generated.rs
Adds wire function wire__crate__api__media_files__upload_chat_media_impl; registers funcId mapping (61) in dispatcher; implements SSE encode/decode and IntoDart for FileMetadata, MediaFile, and Option<FileMetadata>.
Rust deps & minor call change
rust/Cargo.toml, rust/src/api/groups.rs
Bumps whitenoise git revision in Cargo.toml; changes upload_group_image call to pass Some(server) (Option) instead of Url.

Sequence Diagram(s)

sequenceDiagram
    participant Dart as Dart App
    participant Bridge as Dart API Bridge
    participant FFI as FFI Layer
    participant Rust as Rust API
    participant WN as WhiteNoise

    Dart->>Bridge: uploadChatMedia(accountPubkey, groupId, filePath)
    Bridge->>FFI: crateApiMediaFilesUploadChatMedia (encode args)
    FFI->>Rust: wire__crate__api__media_files__upload_chat_media_impl
    Rust->>WN: upload_chat_media(account_pubkey, group_id, file_path)
    WN-->>Rust: WhitenoiseMediaFile
    Rust->>Rust: Convert WhitenoiseMediaFile -> MediaFile (+FileMetadata)
    Rust-->>FFI: Result<MediaFile, ApiError> (SSE)
    FFI->>Bridge: decode MediaFile
    Bridge-->>Dart: Future<MediaFile>
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • erskingardner
  • Quwaysim
  • untreu2

Poem

🐰
I hopped through bytes and stitched a thread,
From Dart to Rust a pathway spread,
Tiny metadata tucked in store,
Files now cross the meadow's door,
Hooray — uploads bounce evermore!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.03% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat: add rust method to upload chat media" accurately describes the primary change in the changeset. The pull request's core objective is to introduce a new Rust function upload_chat_media that handles media file uploads, with corresponding Dart bridge code and FFI integration added as supporting implementation details. The title is concise, specific, and uses clear commit convention semantics. A teammate reviewing the repository history would immediately understand that this PR introduces media upload functionality at the Rust layer.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/upload-chat-media-method

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@josefinalliende josefinalliende marked this pull request as ready for review October 21, 2025 21:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
rust/src/api/groups.rs (1)

370-372: Validate server_url (https-only) and allow defaulting to None.

Currently any scheme parses. Consider rejecting non-https and treating empty string as “use default server” to preserve old behavior and avoid misuse.

Example:

-    let server = Url::parse(&server_url)?;
-    let (encrypted_hash, image_key, image_nonce) = whitenoise
-        .upload_group_image(&account, &group_id, &file_path, Some(server), None)
+    let server_opt = if server_url.trim().is_empty() {
+        None
+    } else {
+        let server = Url::parse(&server_url)?;
+        if server.scheme() != "https" {
+            return Err(ApiError::from(anyhow::anyhow!("server_url must use https")));
+        }
+        Some(server)
+    };
+    let (encrypted_hash, image_key, image_nonce) = whitenoise
+        .upload_group_image(&account, &group_id, &file_path, server_opt, None)
         .await?;

Please confirm whether server_url can originate from untrusted UI input. If yes, the https guard should be prioritized.

lib/src/rust/api/media_files.dart (1)

47-55: Consider modeling fileBlossomUrl as nullable to reflect absence.

Rust maps an optional blossom_url; representing it as String? here would avoid conflating “unset” with “empty string” and simplifies UI checks.

rust/src/api/media_files.rs (2)

27-35: Preserve optional blossom_url; avoid defaulting to empty string.

Expose file_blossom_url as Option and keep None when absent.

Apply:

 pub struct MediaFile {
     pub mls_group_id: String,
     pub file_path: String,
     pub file_mime_type: String,
     pub file_media_type: String,
-    pub file_blossom_url: String,
+    pub file_blossom_url: Option<String>,
     pub file_metadata: Option<FileMetadata>,
     pub original_file_path: Option<String>,
 }
         Self {
             mls_group_id: group_id_to_string(&media_file.mls_group_id),
             file_path: media_file.file_path.to_string_lossy().to_string(),
             file_mime_type: media_file.mime_type.to_string(),
             file_media_type: media_file.media_type.to_string(),
-            file_blossom_url: media_file.blossom_url.unwrap_or_default(),
+            file_blossom_url: media_file.blossom_url,
             file_metadata: media_file.file_metadata.map(|metadata| metadata.into()),
             original_file_path: None,
         }

Pair with the Dart change to make fileBlossomUrl nullable.

Also applies to: 37-49


51-69: Optional: early file existence check for clearer errors.

Validate file_path before calling into whitenoise to fail fast with a clear message.

Example:

use std::path::Path;
if !Path::new(&file_path).exists() {
    return Err(ApiError::from(std::io::Error::new(
        std::io::ErrorKind::NotFound,
        format!("File not found: {file_path}"),
    )));
}

Adjust error mapping to your ApiError variants as needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20b638a and 2fca643.

⛔ Files ignored due to path filters (2)
  • ios/Podfile.lock is excluded by !**/*.lock
  • rust/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • lib/src/rust/api/media_files.dart (1 hunks)
  • lib/src/rust/frb_generated.dart (20 hunks)
  • lib/src/rust/frb_generated.io.dart (13 hunks)
  • rust/Cargo.toml (1 hunks)
  • rust/src/api/groups.rs (1 hunks)
  • rust/src/api/media_files.rs (1 hunks)
  • rust/src/api/mod.rs (2 hunks)
  • rust/src/frb_generated.rs (11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)

Use the whitenoise Rust crate (via flutter_rust_bridge) as the sole data source and change trigger

Files:

  • rust/src/api/mod.rs
  • rust/src/api/media_files.rs
  • rust/src/api/groups.rs
  • rust/src/frb_generated.rs
**/*.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/src/rust/api/media_files.dart
  • lib/src/rust/frb_generated.io.dart
  • lib/src/rust/frb_generated.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/src/rust/api/media_files.dart
  • lib/src/rust/frb_generated.io.dart
  • lib/src/rust/frb_generated.dart
🧬 Code graph analysis (2)
rust/src/api/media_files.rs (1)
rust/src/api/utils.rs (2)
  • group_id_from_string (56-59)
  • group_id_to_string (51-53)
rust/src/frb_generated.rs (1)
rust/src/api/media_files.rs (1)
  • upload_chat_media (52-69)
🔇 Additional comments (5)
rust/src/api/mod.rs (1)

61-61: Module wiring LGTM.

media_files is properly added and re-exported; this keeps the API surface consistent.

Also applies to: 74-74

rust/Cargo.toml (1)

32-32: No issues found. Lockfile and APIs confirmed in sync.

Cargo.lock is already updated to the exact rev (5c79a9b66cb358146113dd485e3cf5987b6371a1), both upload_chat_media (rust/src/api/media_files.rs:63) and upload_group_image (rust/src/api/groups.rs:371) are actively used in the codebase, confirming the APIs exist in the pinned commit. No action needed.

rust/src/frb_generated.rs (1)

1-5124: LGTM! Auto-generated bridge code correctly reflects the new media upload API.

This is auto-generated code by flutter_rust_bridge v2.11.1 that adds the FFI bindings for the new upload_chat_media function. The implementation follows the established patterns consistently:

  • Wire function correctly decodes three string parameters and calls crate::api::media_files::upload_chat_media
  • Dispatch table properly routes funcId 61 to the new function and shifts subsequent IDs
  • Complete serialization support added for FileMetadata and MediaFile (SSE decode/encode, IntoDart, optional variants)
  • Function signature matches the Rust API from rust/src/api/media_files.rs
lib/src/rust/frb_generated.dart (1)

1-2: LGTM: Auto-generated FFI bindings are consistent.

The auto-generated FFI bindings for the new uploadChatMedia method follow the established patterns correctly. The method signature, encoding/decoding paths, and metadata are properly generated.

However, ensure:

  • The source Rust implementation exists and is correct
  • The MediaFile and FileMetadata types are properly defined in api/media_files.dart
  • Run just precommit as noted in the PR checklist

Also applies to: 15-15, 81-81, 317-321, 2359-2393

lib/src/rust/frb_generated.io.dart (1)

1-2: LGTM: Platform-specific FFI abstractions properly generated.

The abstract method declarations for FileMetadata and MediaFile serialization follow the correct patterns for platform-specific implementations. All necessary encode/decode methods for both SSE and DCO codecs are present.

Also applies to: 16-16, 152-153, 178-179, 256-257, 268-269

Copy link
Contributor

@jgmontoya jgmontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment! Looks good though :D

Comment on lines 26 to 58
#[derive(Debug, Clone)]
pub struct MediaFile {
pub mls_group_id: String,
pub file_path: String,
pub file_mime_type: String,
pub file_media_type: String,
pub file_blossom_url: String,
pub file_metadata: Option<FileMetadata>,
pub original_file_path: Option<String>,
}

impl From<WhitenoiseMediaFile> for MediaFile {
fn from(media_file: WhitenoiseMediaFile) -> Self {
Self {
mls_group_id: group_id_to_string(&media_file.mls_group_id),
file_path: media_file.file_path.to_string_lossy().to_string(),
file_mime_type: media_file.mime_type.to_string(),
file_media_type: media_file.media_type.to_string(),
file_blossom_url: media_file.blossom_url.unwrap_or_default(),
file_metadata: media_file.file_metadata.map(|metadata| metadata.into()),
original_file_path: None,
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual type on the Whitenoise crate:

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct MediaFile {
    pub id: Option<i64>,
    pub mls_group_id: GroupId,
    pub account_pubkey: PublicKey,
    pub file_path: PathBuf,
    pub file_hash: Vec<u8>,
    pub mime_type: String,
    pub media_type: String,
    pub blossom_url: Option<String>,
    pub nostr_key: Option<String>,
    pub file_metadata: Option<FileMetadata>,
    pub created_at: DateTime<Utc>,
}

I'm not sure if here we need to replicate it 100% though (I see you're adding original_filepath for example) but I'm leaving it here because it could be useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's not strictly necessary, I think I would try and mimic the original type as much as possible unless you have very good reason not to.

What's the purpose of the original_filepath field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I added it is that when uploading, there is the original file path, after uploaded, the file path changes to a new one. In the preview of the image, if the path changes, the image component is re rendered and it looks like it blinks and it bothers the sight

Copy link
Contributor Author

@josefinalliende josefinalliende Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I could just move that original path to the object of MediaFileUpload class (a draft PR #740 that goes next that besides the media file considers the state of the upload: uploading, uploaded or failed)... that would make more sense to you? @jgmontoya @erskingardner

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good alternative 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the original file path and added the missing fields with the same name as the rust type in a fixup commit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no I missed some

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's ok

codeswot
codeswot previously approved these changes Oct 22, 2025
Copy link
Contributor

@codeswot codeswot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@erskingardner erskingardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same small comment as @jgmontoya, but looks great! We'll make a rust dev of you yet @josefinalliende!

Comment on lines 26 to 58
#[derive(Debug, Clone)]
pub struct MediaFile {
pub mls_group_id: String,
pub file_path: String,
pub file_mime_type: String,
pub file_media_type: String,
pub file_blossom_url: String,
pub file_metadata: Option<FileMetadata>,
pub original_file_path: Option<String>,
}

impl From<WhitenoiseMediaFile> for MediaFile {
fn from(media_file: WhitenoiseMediaFile) -> Self {
Self {
mls_group_id: group_id_to_string(&media_file.mls_group_id),
file_path: media_file.file_path.to_string_lossy().to_string(),
file_mime_type: media_file.mime_type.to_string(),
file_media_type: media_file.media_type.to_string(),
file_blossom_url: media_file.blossom_url.unwrap_or_default(),
file_metadata: media_file.file_metadata.map(|metadata| metadata.into()),
original_file_path: None,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's not strictly necessary, I think I would try and mimic the original type as much as possible unless you have very good reason not to.

What's the purpose of the original_filepath field?

@josefinalliende josefinalliende force-pushed the feat/upload-chat-media-method branch from b46f5fc to 7b18b3b Compare October 22, 2025 15:06
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
rust/src/api/media_files.rs (1)

62-78: Document the None parameters for clarity.

Line 74 passes two None arguments to whitenoise.upload_chat_media, but their purpose isn't clear from the code. Consider adding a comment explaining what these optional parameters represent (e.g., server URL, metadata overrides) for future maintainability. Otherwise, the function structure is sound.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b46f5fc and 7b18b3b.

⛔ Files ignored due to path filters (1)
  • rust/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • lib/src/rust/api/media_files.dart (1 hunks)
  • lib/src/rust/frb_generated.dart (20 hunks)
  • lib/src/rust/frb_generated.io.dart (13 hunks)
  • rust/Cargo.toml (1 hunks)
  • rust/src/api/groups.rs (1 hunks)
  • rust/src/api/media_files.rs (1 hunks)
  • rust/src/api/mod.rs (2 hunks)
  • rust/src/frb_generated.rs (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • rust/src/api/groups.rs
  • lib/src/rust/api/media_files.dart
  • rust/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)

Use the whitenoise Rust crate (via flutter_rust_bridge) as the sole data source and change trigger

Files:

  • rust/src/api/media_files.rs
  • rust/src/api/mod.rs
  • rust/src/frb_generated.rs
**/*.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/src/rust/frb_generated.dart
  • lib/src/rust/frb_generated.io.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/src/rust/frb_generated.dart
  • lib/src/rust/frb_generated.io.dart
🧬 Code graph analysis (2)
rust/src/api/media_files.rs (1)
rust/src/api/utils.rs (2)
  • group_id_from_string (56-59)
  • group_id_to_string (51-53)
rust/src/frb_generated.rs (1)
rust/src/api/media_files.rs (1)
  • upload_chat_media (63-78)
⏰ 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 (20)
rust/src/api/mod.rs (1)

61-61: LGTM! Clean module integration.

The media_files module is properly declared and re-exported, following the established pattern for other API modules in the crate.

Also applies to: 74-74

rust/src/api/media_files.rs (3)

9-25: LGTM! Clean metadata structure and conversion.

The FileMetadata struct and its From implementation provide a straightforward mapping from the WhiteNoise crate's FileMetadata type.


26-41: MediaFile struct looks comprehensive.

The struct appropriately bridges WhiteNoise's MediaFile type to Flutter, using String representations for cross-platform compatibility. Based on learnings, the original_file_path field was discussed and accepted as part of the design.


43-60: Verify the id field's default value semantics.

Line 46 converts Option<i64> to String using unwrap_or_default(), which maps None to "0". This could be ambiguous on the Dart side—a newly created file with None would appear identical to one with an actual ID of 0. Consider whether the Dart MediaFile should use String? for id to preserve the None semantics, or verify that this default is acceptable for your use case.

lib/src/rust/frb_generated.dart (1)

1-5197: LGTM! Generated FFI bindings are correct.

The flutter_rust_bridge codegen has properly created the Dart bindings for the new MediaFile API, including both DCO and SSE serialization paths, API method declarations, and type conversions. As per coding guidelines, this correctly uses flutter_rust_bridge to access core app functionality.

Based on coding guidelines

rust/src/frb_generated.rs (11)

44-44: Generated content hash updated — no action

Auto-generated drift only.


2353-2395: New wire handler for upload_chat_media — LGTM

Decodes 3 strings, forwards to crate::api::media_files::upload_chat_media, returns MediaFile via transform_result_sse. Matches API snippet.


2847-2859: SSE decode for FileMetadata — LGTM

Field order and optionals look consistent.


3279-3290: SSE decode for Option — LGTM


3947-3967: IntoDart for FileMetadata — LGTM

Order matches decode/encode.


4131-4160: IntoDart for MediaFile — LGTM

Array order matches SSE (decode/encode).


4590-4597: SSE encode for FileMetadata — LGTM


4861-4877: SSE encode for MediaFile — LGTM


4911-4919: SSE encode for Option<FileMetadata) — LGTM


3203-3234: SSE decode for MediaFile — Dart model parity verified

All 12 fields are present in identical order with matching types:

  • MediaFile: id, mlsGroupId, accountPubkey, filePath, fileHash, fileMimeType, fileMediaType, fileBlossomUrl, nostrKey, fileMetadata, originalFilePath, createdAt
  • Nested FileMetadata class properly defined with originalFilename, dimensions, blurhash

Type mapping is correct (Rust Option ↔ Dart T?, DateTime ↔ DateTime). No mismatches detected.


3715-3721: Dart side regeneration verified—funcId mapping is correct

The Dart implementation of crateApiMediaFilesUploadChatMedia confirms funcId 61 is properly set and matches the Rust dispatcher mapping. The function correctly serializes arguments and invokes the FFI layer with the new ID.

lib/src/rust/frb_generated.io.dart (4)

16-16: Add import for media_files.dart — LGTM


152-154: DCO decoders for FileMetadata/MediaFile — LGTM

Presence of box_autoadd, plain, and optional variants looks complete.

Also applies to: 178-180, 256-258, 268-270


445-449: SSE decoders for FileMetadata/MediaFile — LGTM

Matches Rust side additions.

Also applies to: 479-481, 571-573, 585-589


788-793: SSE encoders for FileMetadata/MediaFile — LGTM

Symmetric with decoders; ready for round‑trip.

Also applies to: 827-829, 938-940, 956-961

@josefinalliende josefinalliende force-pushed the feat/upload-chat-media-method branch 2 times, most recently from cc9c48a to 21b4de5 Compare October 22, 2025 15:35
@josefinalliende josefinalliende force-pushed the feat/upload-chat-media-method branch from 21b4de5 to 99df9dc Compare October 22, 2025 15:38
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
rust/src/api/media_files.rs (2)

28-40: Preserve nullability parity with Whitenoise for id/blossom_url/nostr_key.

Current Dart-facing struct forces non-null Strings, but Whitenoise exposes Option for these. This loses “unknown/absent” semantics and invites sentinel values. Recommend exposing these as Option on Rust (String?) across FRB, or document invariants if you must keep non-null.


1-7: Tighten imports (optional).

nostr_sdk::prelude::* pulls a lot; import nostr_sdk::prelude::PublicKey to reduce surface. No behavior change.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b18b3b and 667108a.

📒 Files selected for processing (4)
  • lib/src/rust/api/media_files.dart (1 hunks)
  • lib/src/rust/frb_generated.dart (20 hunks)
  • rust/src/api/media_files.rs (1 hunks)
  • rust/src/frb_generated.rs (11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)

Use the whitenoise Rust crate (via flutter_rust_bridge) as the sole data source and change trigger

Files:

  • rust/src/api/media_files.rs
  • rust/src/frb_generated.rs
**/*.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/src/rust/api/media_files.dart
  • lib/src/rust/frb_generated.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/src/rust/api/media_files.dart
  • lib/src/rust/frb_generated.dart
🧠 Learnings (1)
📚 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 flutter_rust_bridge to access core app functionality

Applied to files:

  • lib/src/rust/api/media_files.dart
🧬 Code graph analysis (2)
rust/src/api/media_files.rs (1)
rust/src/api/utils.rs (2)
  • group_id_from_string (56-59)
  • group_id_to_string (51-53)
rust/src/frb_generated.rs (1)
rust/src/api/media_files.rs (1)
  • upload_chat_media (61-76)
⏰ 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 (6)
lib/src/rust/api/media_files.dart (1)

13-21: Wrapper aligns with FRB surface; OK.

Signature mirrors Rust upload_chat_media and routes through RustLib.

lib/src/rust/frb_generated.dart (4)

317-322: API surface wiring looks consistent.

New RustLibApi method crateApiMediaFilesUploadChatMedia matches wrapper and args order.


2359-2394: FFI call setup and funcId are coherent.

Serializer encodes (accountPubkey, groupId, filePath) and uses funcId 61; constMeta debugName matches Rust fn. Downstream shifts (62–65) handled.
Please run your app smoke test for any stale call sites relying on hardcoded funcIds (if any custom bindings exist).


2849-2858: Codec for FileMetadata/MediaFile matches Rust field order and counts.

  • FileMetadata: 3 fields (originalFilename, dimensions, blurhash) — OK.
  • MediaFile: 11 fields — OK and consistent with Rust struct order.
    If you change Rust field optionality (e.g., make id/blossom_url/nostr_key optional), regenerate FRB to update these codecs.

Also applies to: 3062-3079, 3658-3669, 3998-4026, 4920-4934


3573-3580: Optional FileMetadata encode/decode paths added correctly.

opt-box codecs match non-opaque usage and align with the new types.

Also applies to: 4581-4589, 4973-4984

rust/src/frb_generated.rs (1)

1-5246: LGTM! Auto-generated FFI bindings for media upload are correct.

The flutter_rust_bridge code generation correctly reflects the new upload_chat_media API and types (MediaFile, FileMetadata) from the upstream Rust module. The wire function, SSE codec implementations, and Dart interop all follow established patterns consistently.

@josefinalliende josefinalliende merged commit 127d0e9 into master Oct 24, 2025
2 checks passed
@josefinalliende josefinalliende deleted the feat/upload-chat-media-method branch October 24, 2025 13:18
This was referenced Oct 28, 2025
@coderabbitai coderabbitai bot mentioned this pull request Nov 5, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants