fix: improve cli plugin loading error messages

6 files changed, 56 insertions, 18 deletions

diff --git a/zellij-client/src/cli_client.rs b/zellij-client/src/cli_client.rs
index 7e93cc66a..5a023dee3 100644
--- a/zellij-client/src/cli_client.rs
+++ b/zellij-client/src/cli_client.rs
@@ -137,6 +137,14 @@ fn pipe_client(
                             .non_fatal();
                     }
                 },
+                Some((ServerToClientMsg::Log(log_lines), _)) => {
+                    log_lines.iter().for_each(|line| println!("{line}"));
+                    process::exit(0);
+                },
+                Some((ServerToClientMsg::LogError(log_lines), _)) => {
+                    log_lines.iter().for_each(|line| eprintln!("{line}"));
+                    process::exit(2);
+                },
                 Some((ServerToClientMsg::Exit(exit_reason), _)) => {
                     match exit_reason {
                         ExitReason::Error(e) => {
diff --git a/zellij-server/src/lib.rs b/zellij-server/src/lib.rs
index 82ad52cb0..9bbd107b5 100644
--- a/zellij-server/src/lib.rs
+++ b/zellij-server/src/lib.rs
@@ -86,6 +86,7 @@ pub enum ServerInstruction {
     ConnStatus(ClientId),
     ActiveClients(ClientId),
     Log(Vec<String>, ClientId),
+    LogError(Vec<String>, ClientId),
     SwitchSession(ConnectToSession, ClientId),
     UnblockCliPipeInput(String), // String -> Pipe name
     CliPipeOutput(String, String), // String -> Pipe name, String -> Output
@@ -107,6 +108,7 @@ impl From<&ServerInstruction> for ServerContext {
             ServerInstruction::ConnStatus(..) => ServerContext::ConnStatus,
             ServerInstruction::ActiveClients(_) => ServerContext::ActiveClients,
             ServerInstruction::Log(..) => ServerContext::Log,
+            ServerInstruction::LogError(..) => ServerContext::LogError,
             ServerInstruction::SwitchSession(..) => ServerContext::SwitchSession,
             ServerInstruction::UnblockCliPipeInput(..) => ServerContext::UnblockCliPipeInput,
             ServerInstruction::CliPipeOutput(..) => ServerContext::CliPipeOutput,
@@ -741,6 +743,14 @@ pub fn start_server(mut os_input: Box<dyn ServerOsApi>, socket_path: PathBuf) {
                     session_state
                 );
             },
+            ServerInstruction::LogError(lines_to_log, client_id) => {
+                send_to_client!(
+                    client_id,
+                    os_input,
+                    ServerToClientMsg::LogError(lines_to_log),
+                    session_state
+                );
+            },
             ServerInstruction::SwitchSession(connect_to_session, client_id) => {
                 if let Some(min_size) = session_state.read().unwrap().min_client_terminal_size() {
                     session_data
diff --git a/zellij-server/src/plugins/mod.rs b/zellij-server/src/plugins/mod.rs
index 95d30b81d..cb7f4bc30 100644
--- a/zellij-server/src/plugins/mod.rs
+++ b/zellij-server/src/plugins/mod.rs
@@ -113,6 +113,7 @@ pub enum PluginInstruction {
         pane_title: Option<String>,
         cwd: Option<PathBuf>,
         skip_cache: bool,
+        cli_client_id: ClientId,
     },
     CachePluginEvents { plugin_id: PluginId },
     MessageFromPlugin(MessageToPlugin),
@@ -212,6 +213,7 @@ pub(crate) fn plugin_thread_main(
                 cwd.clone(),
                 skip_cache,
                 Some(client_id),
+                None,
             ) {
                 Ok((plugin_id, client_id)) => {
                     drop(bus.senders.send_to_screen(ScreenInstruction::AddPlugin(
@@ -250,7 +252,7 @@ pub(crate) fn plugin_thread_main(
                             // the cli who spawned the command and is not an existing client_id
                             let skip_cache = true; // when reloading we always skip cache
                             match wasm_bridge
-                                .load_plugin(&run, Some(tab_index), size, None, skip_cache, None)
+                                .load_plugin(&run, Some(tab_index), size, None, skip_cache, None, None)
                             {
                                 Ok((plugin_id, client_id)) => {
                                     let should_be_open_in_place = false;
@@ -324,6 +326,7 @@ pub(crate) fn plugin_thread_main(
                             None,
                             skip_cache,
                             Some(client_id),
+                            None,
                         )?;
                         plugin_ids
                             .entry((run.location, run.configuration))
@@ -430,7 +433,8 @@ pub(crate) fn plugin_thread_main(
                 pane_id_to_replace,
                 pane_title,
                 cwd,
-                skip_cache
+                skip_cache,
+                cli_client_id,
             } => {
                 // TODO CONTINUE HERE(18/12):
                 // * make plugin pretty and make POC with pausing and filtering - DONE
@@ -467,19 +471,13 @@ pub(crate) fn plugin_thread_main(
                 //  plugin with --launch-new) - DONE
                 // * bring all the custo moverride stuff form the plugin messages for when
                 // launching a new plugin with a message (like we did through the cli) - DONE
-                // * add permissions <== CONTINUE HERE
+                // * add permissions - DONE
                 // * work on product side... do we need all parameters? does enforcing name make
-                // sense? now that we separated name and id? rethink (some of) the interface?
-                // * work on cli error messages, must be clearer
-
-                // TODO:
-                // * if the plugin is not running
-                // * do a wasm_bridge.load_plugin with as much defaults as possible (accept
-                // overrides in the instruction later)
-                // * make sure the below all_plugin_and_client_ids_for_plugin_location also returns
-                // pending plugins
-                // * continue as normal below (make sure to test this with a pipe, maybe even with
-                // a pipe to multiple plugins where one of them is not loaded)
+                // sense? now that we separated name and id? rethink (some of) the interface? -
+                // DONE
+                // * work on cli error messages, must be clearer - DONE
+                // * make a `zellij pipe` (aliased: zpipe) alias for the cli message that will not
+                // include all the plugin launching stuff
 
                 let should_float = floating.unwrap_or(true);
                 let size = Size::default(); // TODO: why??
@@ -500,14 +498,16 @@ pub(crate) fn plugin_thread_main(
                                     pane_id_to_replace.is_some(),
                                     pane_title,
                                     pane_id_to_replace,
+                                    Some(cli_client_id),
                                 );
                                 for (plugin_id, client_id) in all_plugin_ids {
                                     updates.push((Some(plugin_id), client_id, Event::CliMessage {input_pipe_id: input_pipe_id.clone(), name: name.clone(), payload: payload.clone(), args: args.clone() }));
                                 }
                             },
                             Err(e) => {
-                                log::error!("Failed to parse plugin url: {:?}", e);
-                                // TODO: inform cli client
+                                let _ = bus.senders.send_to_server(
+                                    ServerInstruction::LogError(vec![format!("Failed to parse plugin url: {}", e)], cli_client_id)
+                                );
                             }
                         }
                     },
@@ -553,6 +553,7 @@ pub(crate) fn plugin_thread_main(
                                     should_be_open_in_place,
                                     pane_title,
                                     pane_id_to_replace.map(|p| p.into()),
+                                    None,
                                 );
                                 for (plugin_id, client_id) in all_plugin_ids {
                                     updates.push((Some(plugin_id), client_id, Event::MessageFromPlugin {
diff --git a/zellij-server/src/plugins/wasm_bridge.rs b/zellij-server/src/plugins/wasm_bridge.rs
index 407d98c98..7660d1621 100644
--- a/zellij-server/src/plugins/wasm_bridge.rs
+++ b/zellij-server/src/plugins/wasm_bridge.rs
@@ -29,6 +29,7 @@ use crate::panes::PaneId;
 use crate::{
     background_jobs::BackgroundJob, screen::ScreenInstruction, thread_bus::ThreadSenders,
     ui::loading_indication::LoadingIndication, ClientId,
+    ServerInstruction
 };
 use zellij_utils::{
     data::{Event, EventType, PluginCapabilities},
@@ -123,6 +124,7 @@ impl WasmBridge {
         cwd: Option<PathBuf>,
         skip_cache: bool,
         client_id: Option<ClientId>,
+        cli_client_id: Option<ClientId>,
     ) -> Result<(PluginId, ClientId)> {
         // returns the plugin id
         let err_context = move || format!("failed to load plugin");
@@ -194,6 +196,7 @@ impl WasmBridge {
                             plugin_id,
                             &mut loading_indication,
                             e,
+                            cli_client_id,
                         ),
                     }
                 }
@@ -225,6 +228,7 @@ impl WasmBridge {
                         plugin_id,
                         &mut loading_indication,
                         e,
+                        cli_client_id,
                     ),
                 }
                 let _ =
@@ -331,6 +335,7 @@ impl WasmBridge {
                                     *plugin_id,
                                     &mut loading_indication,
                                     e,
+                                    None,
                                 ),
                             }
                         }
@@ -342,6 +347,7 @@ impl WasmBridge {
                                 *plugin_id,
                                 &mut loading_indication,
                                 &e,
+                                None,
                             );
                         }
                     },
@@ -873,7 +879,7 @@ impl WasmBridge {
 
     // gets all running plugins details matching this run_plugin, if none are running, loads one and
     // returns its details
-    pub fn get_or_load_plugins(&mut self, run_plugin: RunPlugin, size: Size, cwd: Option<PathBuf>, skip_cache: bool, should_float: bool, should_be_open_in_place: bool, pane_title: Option<String>, pane_id_to_replace: Option<PaneId>) -> Vec<(PluginId, Option<ClientId>)> {
+    pub fn get_or_load_plugins(&mut self, run_plugin: RunPlugin, size: Size, cwd: Option<PathBuf>, skip_cache: bool, should_float: bool, should_be_open_in_place: bool, pane_title: Option<String>, pane_id_to_replace: Option<PaneId>, cli_client_id: Option<ClientId>) -> Vec<(PluginId, Option<ClientId>)> {
         let all_plugin_ids = self.all_plugin_and_client_ids_for_plugin_location(&run_plugin.location, &run_plugin.configuration);
         if all_plugin_ids.is_empty() {
             if let Some(loading_plugin_id) = self.plugin_id_of_loading_plugin(&run_plugin.location, &run_plugin.configuration) {
@@ -886,6 +892,7 @@ impl WasmBridge {
                 cwd.clone(),
                 skip_cache,
                 None,
+                cli_client_id,
             ) {
                 Ok((plugin_id, client_id)) => {
                     drop(self.senders.send_to_screen(ScreenInstruction::AddPlugin(
@@ -903,6 +910,11 @@ impl WasmBridge {
                 },
                 Err(e) => {
                     log::error!("Failed to load plugin: {e}");
+                    if let Some(cli_client_id) = cli_client_id {
+                        let _ = self.senders.send_to_server(
+                            ServerInstruction::LogError(vec![format!("Failed to log plugin: {e}")], cli_client_id)
+                        );
+                    }
                     vec![]
                 },
             }
@@ -925,6 +937,7 @@ fn handle_plugin_loading_failure(
     plugin_id: PluginId,
     loading_indication: &mut LoadingIndication,
     error: impl std::fmt::Debug,
+    cli_client_id: Option<ClientId>,
 ) {
     log::error!("{:?}", error);
     let _ = senders.send_to_background_jobs(BackgroundJob::StopPluginLoadingAnimation(plugin_id));
@@ -933,6 +946,11 @@ fn handle_plugin_loading_failure(
         plugin_id,
         loading_indication.clone(),
     ));
+    if let Some(cli_client_id) = cli_client_id {
+        let _ = senders.send_to_server(
+            ServerInstruction::LogError(vec![format!("{:?}", error)], cli_client_id)
+        );
+    }
 }
 
 // TODO: move to permissions?
diff --git a/zellij-server/src/route.rs b/zellij-server/src/route.rs
index 375adb464..1c0a22cad 100644
--- a/zellij-server/src/route.rs
+++ b/zellij-server/src/route.rs
@@ -820,7 +820,7 @@ pub(crate) fn route_action(
                 }
                 let pane_id_to_replace = if should_open_in_place { pane_id } else { None };
                 senders
-                    .send_to_plugin(PluginInstruction::CliMessage { input_pipe_id, name, payload, plugin, args, configuration, floating, pane_id_to_replace, cwd, pane_title, skip_cache })
+                    .send_to_plugin(PluginInstruction::CliMessage { input_pipe_id, name, payload, plugin, args, configuration, floating, pane_id_to_replace, cwd, pane_title, skip_cache, cli_client_id: client_id })
                     .with_context(err_context)?;
             } else {
                 log::error!("Message must have a name");
diff --git a/zellij-utils/src/errors.rs b/zellij-utils/src/errors.rs
index 2be6d89d2..77777bcec 100644
--- a/zellij-utils/src/errors.rs
+++ b/zellij-utils/src/errors.rs
@@ -436,6 +436,7 @@ pub enum ServerContext {
     ConnStatus,
     ActiveClients,
     Log,
+    LogError,
     SwitchSession,
     UnblockCliPipeInput,
     CliPipeOutput,