diff --git a/crates/app/src/session/message.rs b/crates/app/src/session/message.rs index a5601623a4..51fb1f447a 100644 --- a/crates/app/src/session/message.rs +++ b/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, + /// The newly stored attachments. + attachments: Vec, + /// The total count of the attachments stored in the session so far. len: u64, }, diff --git a/crates/app/src/session/service/mod.rs b/crates/app/src/session/service/mod.rs index 00584436d1..1eb04decbe 100644 --- a/crates/app/src/session/service/mod.rs +++ b/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 => { diff --git a/crates/app/src/session/ui/mod.rs b/crates/app/src/session/ui/mod.rs index 1603f80949..6d4fcf57bb 100644 --- a/crates/app/src/session/ui/mod.rs +++ b/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 {}.", diff --git a/crates/app/src/session/ui/shared/attachments.rs b/crates/app/src/session/ui/shared/attachments.rs index 64a174956e..4d203d5d65 100644 --- a/crates/app/src/session/ui/shared/attachments.rs +++ b/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,6 +99,14 @@ impl AttachmentsState { self.attachments.push(attachment); } + /// Add all the given attachments, keeping their order. + pub fn add_all(&mut self, attachments: Vec) { + 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, @@ -92,18 +114,26 @@ impl AttachmentsState { }; } + /// 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) } diff --git a/crates/core/session/src/handlers/observing/logs_writer.rs b/crates/core/session/src/handlers/observing/logs_writer.rs index 3a4f3ede16..ef87388ff7 100644 --- a/crates/core/session/src/handlers/observing/logs_writer.rs +++ b/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(()) } diff --git a/crates/core/session/src/state/api.rs b/crates/core/session/src/state/api.rs index b82218e102..de4e40ecbe 100644 --- a/crates/core/session/src/state/api.rs +++ b/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), GetAttachments(oneshot::Sender>), // 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, + ) -> 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}", + )) }) } diff --git a/crates/core/session/src/state/attachments.rs b/crates/core/session/src/state/attachments.rs index 28872426bc..97804c9cb9 100644 --- a/crates/core/session/src/state/attachments.rs +++ b/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 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 } } +/// 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 { + 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, + /// Descriptions of the stored attachments in the order they have been stored. + attachments: Vec, dest: Option, } 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 { - 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 { - 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 { - self.attachments - .values() - .cloned() - .collect::>() + /// All the stored attachments in the order they have been stored. + pub fn attachments(&self) -> &[stypes::AttachmentInfo] { + &self.attachments } } diff --git a/crates/core/session/src/state/mod.rs b/crates/core/session/src/state/mod.rs index 710a8f6db6..53a7e2b637 100644 --- a/crates/core/session/src/state/mod.rs +++ b/crates/core/session/src/state/mod.rs @@ -538,15 +538,30 @@ impl SessionState { })? } - fn handle_add_attachment( + /// Stores the given attachments and notifies the clients about the stored ones with a + /// single event. Attachments which can't be stored are logged and skipped, and no event + /// is sent when none of them could be stored. + fn handle_add_attachments( &mut self, - origin: parsers::Attachment, + origins: Vec, tx_callback_events: UnboundedSender, ) -> Result<(), stypes::NativeError> { - let attachment = self.attachments.add(origin)?; + let mut attachments = Vec::with_capacity(origins.len()); + for origin in origins { + // Failing on a single attachment must not drop the remaining ones. + match self.attachments.add(origin) { + Ok(attachment) => attachments.push(attachment), + Err(err) => error!("Fail to process attachment; error: {err}"), + } + } + + if attachments.is_empty() { + return Ok(()); + } + tx_callback_events.send(stypes::CallbackEvent::AttachmentsUpdated { len: self.attachments.len() as u64, - attachment, + attachments, })?; Ok(()) } @@ -962,14 +977,12 @@ async fn handle_api_msg( Api::NotifyCanceledOperation(uuid) => { state.cancelling_operations.remove(&uuid); } - Api::AddAttachment(attachment) => { - let at_name = attachment.name.clone(); - if let Err(err) = state.handle_add_attachment(attachment, tx_callback_events.clone()) { - error!("Fail to process attachment {at_name:?}; error: {err:?}"); - } + Api::AddAttachments(attachments) => { + state.handle_add_attachments(attachments, tx_callback_events.clone())?; } Api::GetAttachments(tx_response) => { - tx_response.send(state.attachments.get()).map_err(|_| { + let attachments = state.attachments.attachments().to_vec(); + tx_response.send(attachments).map_err(|_| { stypes::NativeError::channel("Failed to respond to Api::GetAttachments") })?; } diff --git a/crates/stypes/src/callback/formating.rs b/crates/stypes/src/callback/formating.rs index 007ae8847e..c9140eef19 100644 --- a/crates/stypes/src/callback/formating.rs +++ b/crates/stypes/src/callback/formating.rs @@ -13,7 +13,7 @@ impl std::fmt::Display for CallbackEvent { /// - `IndexedMapUpdated(len)` - Displays the number of indexed map entries. /// - `SearchMapUpdated` - Indicates that the search map has been updated. /// - `SearchValuesUpdated` - Indicates that search values have been updated. - /// - `AttachmentsUpdated: {len}` - Displays the size of the updated attachment. + /// - `AttachmentsUpdated: {len}` - Displays the total count of the session attachments. /// - `Progress` - Indicates progress for an operation. /// - `SessionError: {err}` - Displays details of a session error. /// - `OperationError: {uuid}: {error}` - Displays the UUID of the operation and the error details. @@ -29,7 +29,10 @@ impl std::fmt::Display for CallbackEvent { Self::IndexedMapUpdated { len } => write!(f, "IndexedMapUpdated({len})"), Self::SearchMapUpdated(_) => write!(f, "SearchMapUpdated"), Self::SearchValuesUpdated(_) => write!(f, "SearchValuesUpdated"), - Self::AttachmentsUpdated { len, attachment: _ } => { + Self::AttachmentsUpdated { + len, + attachments: _, + } => { write!(f, "AttachmentsUpdated: {len}") } Self::Progress { diff --git a/crates/stypes/src/callback/mod.rs b/crates/stypes/src/callback/mod.rs index c1a5c8b1b0..c78f817aff 100644 --- a/crates/stypes/src/callback/mod.rs +++ b/crates/stypes/src/callback/mod.rs @@ -53,12 +53,12 @@ pub enum CallbackEvent { /// - `Option>`: The value map. SearchValuesUpdated(Option>), - /// Triggered whenever a new attachment is detected in the logs. + /// Triggered whenever new attachments are detected in the logs. AttachmentsUpdated { - /// The size of the attachment in bytes. + /// The total count of the attachments stored in the session so far. len: u64, - /// The description of the attachment. - attachment: AttachmentInfo, + /// The descriptions of the newly stored attachments. + attachments: Vec, }, /// Triggered when progress is made during an operation.