Send parsed attachments to UI in batches + Refactoring & Cleanup

crates/app/src/session/message.rs

@@ -88,9 +88,11 @@ pub enum SessionMessage {
     /// This event is not triggered for streams within a session.
     FileReadCompleted,
 
-    /// Triggered when attachments are updated
+    /// Triggered when new attachments are detected in the session.
     AttachmentsUpdated {
-        attachment: Box<AttachmentInfo>,
+        /// The newly stored attachments.
+        attachments: Vec<AttachmentInfo>,
+        /// The total count of the attachments stored in the session so far.
         len: u64,
     },
 

crates/app/src/session/service/mod.rs

@@ -911,12 +911,9 @@ impl SessionService {
                     .send_session_msg(SessionMessage::FileReadCompleted)
                     .await;
             }
-            CallbackEvent::AttachmentsUpdated { attachment, len } => {
+            CallbackEvent::AttachmentsUpdated { attachments, len } => {
                 self.senders
-                    .send_session_msg(SessionMessage::AttachmentsUpdated {
-                        attachment: Box::new(attachment),
-                        len,
-                    })
+                    .send_session_msg(SessionMessage::AttachmentsUpdated { attachments, len })
                     .await;
             }
             event => {

crates/app/src/session/ui/mod.rs

@@ -445,8 +445,8 @@ impl Session {
                     self.recent_session
                         .on_file_read_completed(&self.shared, actions, &self.cmd_tx);
                 }
-                SessionMessage::AttachmentsUpdated { attachment, len } => {
-                    self.shared.attachments.add(*attachment);
+                SessionMessage::AttachmentsUpdated { attachments, len } => {
+                    self.shared.attachments.add_all(attachments);
                     if self.shared.attachments.attachments().len() as u64 != len {
                         warn!(
                             "Unexpected internal error: Attachment count mismatch: expected {} from backend, got {}.",

crates/app/src/session/ui/shared/attachments.rs

@@ -21,18 +21,27 @@ pub struct AttachmentsState {
     modal: AttachmentModalState,
 }
 
+/// State of the modal showing the preview of a single attachment.
 #[derive(Debug, Default)]
 pub enum AttachmentModalState {
+    /// No attachment preview is open.
     #[default]
     Closed,
+    /// Preview of the attachment is open while its content is still being loaded.
     Pending {
+        /// Attachment whose content is awaited.
         attachment: AttachmentInfo,
     },
+    /// Preview of the attachment with its loaded content.
     Content {
+        /// Previewed attachment.
         attachment: AttachmentInfo,
+        /// Loaded content to render.
         content: PreviewContent,
     },
+    /// Preview of an attachment whose type can't be rendered.
     NotSupported {
+        /// Attachment without preview support.
         attachment: AttachmentInfo,
     },
 }
@@ -47,6 +56,11 @@ pub struct AttachmentFilter {
 }
 
 impl AttachmentsState {
+    /// Add the given attachment, keeping the lookup indices, the known extensions and the
+    /// active filter up to date.
+    ///
+    /// Attachments with an already known UUID keep their existing entry, refreshing their
+    /// log-position lookups only.
     pub fn add(&mut self, attachment: AttachmentInfo) {
         let uuid = attachment.uuid;
         let index = self
@@ -85,25 +99,41 @@ impl AttachmentsState {
         self.attachments.push(attachment);
     }
 
+    /// Add all the given attachments, keeping their order.
+    pub fn add_all(&mut self, attachments: Vec<AttachmentInfo>) {
+        for attachment in attachments {
+            self.add(attachment);
+        }
+    }
+
+    /// Show the loaded `content` of the given attachment in the preview modal.
     pub fn show_preview_content(&mut self, attachment: AttachmentInfo, content: PreviewContent) {
         self.modal = AttachmentModalState::Content {
             attachment,
             content,
         };
     }
 
+    /// Open the preview modal for the given attachment while its content is still loading.
     pub fn show_preview_pending(&mut self, attachment: AttachmentInfo) {
         self.modal = AttachmentModalState::Pending { attachment };
     }
 
+    /// Open the preview modal stating that the given attachment can't be previewed.
     pub fn show_preview_unsupported(&mut self, attachment: AttachmentInfo) {
         self.modal = AttachmentModalState::NotSupported { attachment };
     }
 
+    /// Close the preview modal, dropping its current state.
     pub fn close_preview_modal(&mut self) {
         self.modal = AttachmentModalState::Closed;
     }
 
+    /// Show `content` in the preview modal.
+    ///
+    /// The content is dropped unless the modal is still pending for the attachment with the
+    /// given ID, since the user may have closed the modal or opened another attachment while
+    /// the content was loading.
     pub fn handle_modal_preview(&mut self, attachment_id: Uuid, content: PreviewContent) {
         let AttachmentModalState::Pending { attachment } = &self.modal else {
             return;
@@ -119,6 +149,7 @@ impl AttachmentsState {
         };
     }
 
+    /// Close the preview modal if it is still pending for the attachment with the given ID.
     pub fn close_pending_modal(&mut self, attachment_id: Uuid) {
         let AttachmentModalState::Pending { attachment } = &self.modal else {
             return;
@@ -131,6 +162,7 @@ impl AttachmentsState {
         self.close_preview_modal();
     }
 
+    /// Current state of the attachment preview modal.
     pub fn preview_modal(&self) -> &AttachmentModalState {
         &self.modal
     }
@@ -202,6 +234,7 @@ impl AttachmentsState {
 }
 
 impl AttachmentModalState {
+    /// Returns `true` when no attachment preview is open.
     pub fn closed(&self) -> bool {
         matches!(self, AttachmentModalState::Closed)
     }

crates/core/session/src/handlers/observing/logs_writer.rs

@@ -54,9 +54,10 @@ impl LogsWriter {
             self.text_buffer.clear();
             self.state.write_session_file(self.id, msgs).await?;
         }
-        for attachment in self.attachments.drain(..) {
-            // TODO: send all attachments with 1 call
-            self.state.add_attachment(attachment)?;
+        if !self.attachments.is_empty() {
+            // Draining into a new vector preserves the capacity of the internal buffer.
+            let attachments = self.attachments.drain(..).collect();
+            self.state.add_attachments(attachments)?;
         }
         Ok(())
     }

crates/core/session/src/state/api.rs

@@ -195,7 +195,7 @@ pub enum Api {
     SetDebugMode((bool, oneshot::Sender<()>)),
     NotifyCancelingOperation(Uuid),
     NotifyCanceledOperation(Uuid),
-    AddAttachment(parsers::Attachment),
+    AddAttachments(Vec<parsers::Attachment>),
     GetAttachments(oneshot::Sender<Vec<stypes::AttachmentInfo>>),
     // Used for tests of error handeling
     ShutdownWithError,
@@ -250,7 +250,7 @@ impl Display for Api {
                 Self::SetDebugMode(_) => "SetDebugMode",
                 Self::NotifyCancelingOperation(_) => "NotifyCancelingOperation",
                 Self::NotifyCanceledOperation(_) => "NotifyCanceledOperation",
-                Self::AddAttachment(_) => "AddAttachment",
+                Self::AddAttachments(_) => "AddAttachments",
                 Self::GetAttachments(_) => "GetAttachments",
                 Self::Shutdown => "Shutdown",
                 Self::ShutdownWithError => "ShutdownWithError",
@@ -686,11 +686,21 @@ impl SessionStateAPI {
         })
     }
 
-    pub fn add_attachment(&self, origin: parsers::Attachment) -> Result<(), stypes::NativeError> {
-        self.tx_api.send(Api::AddAttachment(origin)).map_err(|e| {
-            stypes::NativeError::channel(
-                &format!("fail to send to Api::AddAttachment; error: {e}",),
-            )
+    /// Sends the given attachments to the session state to be stored and delivered to the
+    /// clients within a single event.
+    ///
+    /// # Note:
+    ///
+    /// This call doesn't wait for the attachments to be processed. Success means the request
+    /// has been queued only.
+    pub fn add_attachments(
+        &self,
+        origins: Vec<parsers::Attachment>,
+    ) -> Result<(), stypes::NativeError> {
+        self.tx_api.send(Api::AddAttachments(origins)).map_err(|e| {
+            stypes::NativeError::channel(&format!(
+                "fail to send to Api::AddAttachments; error: {e}",
+            ))
         })
     }
 

crates/core/session/src/state/attachments.rs

@@ -1,7 +1,6 @@
 use mime_guess;
 use parsers::{self};
 use std::{
-    collections::HashMap,
     fs::{File, create_dir},
     io,
     io::Write,
@@ -12,24 +11,12 @@ use uuid::Uuid;
 
 #[derive(Error, Debug)]
 pub enum AttachmentsError {
-    #[error("Save error: {0}")]
-    Save(String),
-    #[error("IO error: {0:?}")]
-    Io(#[from] std::io::Error),
+    #[error("Failed to store attachment {name:?}: {source}")]
+    Store { name: String, source: io::Error },
     #[error("Session isn't created")]
     SessionNotCreated,
 }
 
-impl From<AttachmentsError> for stypes::NativeError {
-    fn from(err: AttachmentsError) -> Self {
-        stypes::NativeError {
-            severity: stypes::Severity::ERROR,
-            kind: stypes::NativeErrorKind::Io,
-            message: Some(err.to_string()),
-        }
-    }
-}
-
 const FILE_NAME_INDEXES_LIMIT: usize = 1000;
 const ALLOWED_FILENAME_CHARS: &[char] = &['-', '_'];
 
@@ -85,47 +72,39 @@ fn get_valid_file_path(dest: &Path, origin: &str) -> Result<PathBuf, io::Error>
     }
 }
 
+/// Writes the payload of `origin` into `store_folder`, creating the folder when it's missing.
+///
+/// # Return:
+/// The path of the created file.
+fn write_attachment_file(
+    origin: &parsers::Attachment,
+    store_folder: &Path,
+) -> Result<PathBuf, io::Error> {
+    if !store_folder.exists() {
+        create_dir(store_folder)?;
+    }
+    let attachment_path = get_valid_file_path(store_folder, &origin.name)?;
+    let mut attachment_file = File::create(&attachment_path)?;
+    attachment_file.write_all(&origin.data)?;
+
+    Ok(attachment_path)
+}
+
 #[derive(Debug)]
 pub struct Attachments {
-    attachments: HashMap<Uuid, stypes::AttachmentInfo>,
+    /// Descriptions of the stored attachments in the order they have been stored.
+    attachments: Vec<stypes::AttachmentInfo>,
     dest: Option<PathBuf>,
 }
 
 impl Attachments {
     pub fn new() -> Self {
         Attachments {
-            attachments: HashMap::new(),
+            attachments: Vec::new(),
             dest: None,
         }
     }
 
-    pub fn get_attch_from(
-        origin: parsers::Attachment,
-        store_folder: &PathBuf,
-    ) -> Result<stypes::AttachmentInfo, AttachmentsError> {
-        if !store_folder.exists() {
-            create_dir(store_folder).map_err(AttachmentsError::Io)?;
-        }
-        let uuid = Uuid::new_v4();
-        let attachment_path =
-            get_valid_file_path(store_folder, &origin.name).map_err(AttachmentsError::Io)?;
-        let mut attachment_file = File::create(&attachment_path)?;
-        attachment_file.write_all(&origin.data)?;
-        Ok(stypes::AttachmentInfo {
-            uuid,
-            filepath: attachment_path,
-            name: origin.name.clone(),
-            ext: Path::new(&origin.name)
-                .extension()
-                .map(|ex| ex.to_string_lossy().to_string()),
-            size: origin.size,
-            mime: mime_guess::from_path(origin.name)
-                .first()
-                .map(|guess| guess.to_string()),
-            messages: origin.messages,
-        })
-    }
-
     pub fn set_dest_path(&mut self, dest: PathBuf) -> bool {
         if let (Some(parent), Some(file_stem)) = (dest.parent(), dest.file_stem()) {
             let dest = parent.join(file_stem);
@@ -145,25 +124,45 @@ impl Attachments {
         self.len() == 0
     }
 
+    /// Stores the payload of the given attachment on disk and keeps its description.
     pub fn add(
         &mut self,
-        attachment: parsers::Attachment,
+        origin: parsers::Attachment,
     ) -> Result<stypes::AttachmentInfo, AttachmentsError> {
-        if let Some(dest) = self.dest.as_ref() {
-            let uuid = Uuid::new_v4();
-            let a = Self::get_attch_from(attachment, dest)?;
-            self.attachments.insert(uuid, a.clone());
-            Ok(a)
-        } else {
-            Err(AttachmentsError::SessionNotCreated)
-        }
+        let Some(dest) = self.dest.as_ref() else {
+            return Err(AttachmentsError::SessionNotCreated);
+        };
+
+        // The attachment name is cloned in the error path only, keeping the success path
+        // free of extra allocations.
+        let attachment_path =
+            write_attachment_file(&origin, dest).map_err(|source| AttachmentsError::Store {
+                name: origin.name.clone(),
+                source,
+            })?;
+
+        let attachment = stypes::AttachmentInfo {
+            uuid: Uuid::new_v4(),
+            filepath: attachment_path,
+            name: origin.name.clone(),
+            ext: Path::new(&origin.name)
+                .extension()
+                .map(|ex| ex.to_string_lossy().to_string()),
+            size: origin.size,
+            mime: mime_guess::from_path(origin.name)
+                .first()
+                .map(|guess| guess.to_string()),
+            messages: origin.messages,
+        };
+
+        self.attachments.push(attachment.clone());
+
+        Ok(attachment)
     }
 
-    pub fn get(&self) -> Vec<stypes::AttachmentInfo> {
-        self.attachments
-            .values()
-            .cloned()
-            .collect::<Vec<stypes::AttachmentInfo>>()
+    /// All the stored attachments in the order they have been stored.
+    pub fn attachments(&self) -> &[stypes::AttachmentInfo] {
+        &self.attachments
     }
 }