diff --git a/crates/editor/src/git/blame.rs b/crates/editor/src/git/blame.rs index f8197d047d1cb383b304d63856fc1cc3029d3745..147989f984deda46ed7f7905df89add73439be25 100644 --- a/crates/editor/src/git/blame.rs +++ b/crates/editor/src/git/blame.rs @@ -619,6 +619,7 @@ impl GitBlame { cx.emit(project::Event::Toast { notification_id: "git-blame".into(), message: notification, + link: None, }); } else { // If we weren't triggered by a user, we just log errors in the background, instead of sending @@ -786,7 +787,8 @@ mod tests { project::Event::Toast { notification_id: "git-blame".into(), message: "Failed to blame \"file.txt\": failed to get blame for \"file.txt\"" - .to_string() + .to_string(), + link: None } ); diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 3cb275d4dd98c5dc195f7fca7655c66f640c1b20..f8ee84a6394a1ba1454f1f24c3c77d2036dda262 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -305,6 +305,13 @@ enum ProjectClientState { }, } +/// A link to display in a toast notification, useful to point to documentation. +#[derive(PartialEq, Debug, Clone)] +pub struct ToastLink { + pub label: &'static str, + pub url: &'static str, +} + #[derive(Clone, Debug, PartialEq)] pub enum Event { LanguageServerAdded(LanguageServerId, LanguageServerName, Option), @@ -326,6 +333,8 @@ pub enum Event { Toast { notification_id: SharedString, message: String, + /// Optional link to display as a button in the toast. + link: Option, }, HideToast { notification_id: SharedString, @@ -3215,6 +3224,7 @@ impl Project { cx.emit(Event::Toast { notification_id: "dap".into(), message: message.clone(), + link: None, }); } } @@ -3345,6 +3355,7 @@ impl Project { LspStoreEvent::Notification(message) => cx.emit(Event::Toast { notification_id: "lsp".into(), message: message.clone(), + link: None, }), LspStoreEvent::SnippetEdit { buffer_id, @@ -3395,6 +3406,7 @@ impl Project { let message = format!("Failed to set local settings in {path:?}:\n{message}"); cx.emit(Event::Toast { notification_id: format!("local-settings-{path:?}").into(), + link: None, message, }); } @@ -3408,6 +3420,10 @@ impl Project { let message = format!("Failed to set local tasks in {path:?}:\n{message}"); cx.emit(Event::Toast { notification_id: format!("local-tasks-{path:?}").into(), + link: Some(ToastLink { + label: "Open Tasks Documentation", + url: "https://zed.dev/docs/tasks", + }), message, }); } @@ -3422,6 +3438,7 @@ impl Project { format!("Failed to set local debug scenarios in {path:?}:\n{message}"); cx.emit(Event::Toast { notification_id: format!("local-debug-scenarios-{path:?}").into(), + link: None, message, }); } @@ -4888,6 +4905,7 @@ impl Project { cx.emit(Event::Toast { notification_id: envelope.payload.notification_id.into(), message: envelope.payload.message, + link: None, }); Ok(()) }) diff --git a/crates/project/src/task_inventory.rs b/crates/project/src/task_inventory.rs index e7eea5b158c6108b57ee3cd2f8d6dd9a14771c10..c52bd0d9056f1fd7b84ee46f6a563a0e90299507 100644 --- a/crates/project/src/task_inventory.rs +++ b/crates/project/src/task_inventory.rs @@ -512,6 +512,7 @@ impl Inventory { let new_resolved_tasks = worktree_tasks .flat_map(|(kind, task)| { let id_base = kind.to_id_base(); + if let TaskSourceKind::Worktree { id, .. } = &kind { None.or_else(|| { let (_, _, item_context) = @@ -660,8 +661,31 @@ impl Inventory { }); } }; + + let mut validation_errors = Vec::new(); let new_templates = raw_tasks.into_iter().filter_map(|raw_template| { - serde_json::from_value::(raw_template).log_err() + let template = serde_json::from_value::(raw_template).log_err()?; + + // Validate the variable names used in the `TaskTemplate`. + let unknown_variables = template.unknown_variables(); + if !unknown_variables.is_empty() { + let variables_list = unknown_variables + .iter() + .map(|variable| format!("${variable}")) + .collect::>() + .join(", "); + + validation_errors.push(format!( + "Task '{}' uses unknown variables: {}", + template.label, variables_list + )); + + // Skip this template, since it uses unknown variable names, but + // continue processing others. + return None; + } + + Some(template) }); let parsed_templates = &mut self.templates_from_settings; @@ -710,6 +734,18 @@ impl Inventory { } } + if !validation_errors.is_empty() { + return Err(InvalidSettingsError::Tasks { + path: match &location { + TaskSettingsLocation::Global(path) => path.to_path_buf(), + TaskSettingsLocation::Worktree(location) => { + location.path.as_std_path().join(task_file_name()) + } + }, + message: validation_errors.join("\n"), + }); + } + Ok(()) } diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index 0196e731ae10588615dbc25356997a9efa2838ba..f6ce89f7e675206a3452c9ba5471f3ccb371c28e 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -68,10 +68,12 @@ use settings::SettingsStore; #[cfg(not(windows))] use std::os; use std::{ + cell::RefCell, env, mem, num::NonZeroU32, ops::Range, path::{Path, PathBuf}, + rc::Rc, str::FromStr, sync::{Arc, OnceLock}, task::Poll, @@ -1112,6 +1114,64 @@ async fn test_managing_project_specific_settings(cx: &mut gpui::TestAppContext) ); } +#[gpui::test] +async fn test_invalid_local_tasks_shows_toast_with_doc_link(cx: &mut gpui::TestAppContext) { + init_test(cx); + TaskStore::init(None); + + // We need to start with a valid `.zed/tasks.json` file as otherwise the + // event is emitted before we havd a chance to setup the event subscription. + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/dir"), + json!({ + ".zed": { + "tasks.json": r#"[{ "label": "valid task", "command": "echo" }]"#, + }, + "file.rs": "" + }), + ) + .await; + + let project = Project::test(fs.clone(), [path!("/dir").as_ref()], cx).await; + let saw_toast = Rc::new(RefCell::new(false)); + + // Update the `.zed/tasks.json` file with an invalid variable, so we can + // later assert that the `Event::Toast` even is emitted. + fs.save( + path!("/dir/.zed/tasks.json").as_ref(), + &r#"[{ "label": "test $ZED_FOO", "command": "echo" }]"#.into(), + Default::default(), + ) + .await + .unwrap(); + + project.update(cx, |_, cx| { + let saw_toast = saw_toast.clone(); + + cx.subscribe(&project, move |_, _, event: &Event, _| match event { + Event::Toast { + notification_id, + message, + link: Some(ToastLink { url, .. }), + } => { + assert!(notification_id.starts_with("local-tasks-")); + assert!(message.contains("ZED_FOO")); + assert_eq!(*url, "https://zed.dev/docs/tasks"); + *saw_toast.borrow_mut() = true; + } + _ => {} + }) + .detach(); + }); + + cx.run_until_parked(); + assert!( + *saw_toast.borrow(), + "Expected `Event::Toast` was never emitted" + ); +} + #[gpui::test] async fn test_fallback_to_single_worktree_tasks(cx: &mut gpui::TestAppContext) { init_test(cx); diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index e96b15ce2e5936a4d09ae1c1f5741b33050aba6d..4d0d919e8c6a21accb1c9aa211127de1f1ea57e3 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -1834,6 +1834,7 @@ impl ProjectPanel { ), abs_path ), + link: None, }) }); None diff --git a/crates/task/src/task_template.rs b/crates/task/src/task_template.rs index 0c319db0616862489b7b7d21912142a01ee89fcb..539b2779cc85b5830af90aeb4ffd28596c2c29c3 100644 --- a/crates/task/src/task_template.rs +++ b/crates/task/src/task_template.rs @@ -128,8 +128,6 @@ impl TaskTemplates { impl TaskTemplate { /// Replaces all `VariableName` task variables in the task template string fields. - /// If any replacement fails or the new string substitutions still have [`ZED_VARIABLE_NAME_PREFIX`], - /// `None` is returned. /// /// Every [`ResolvedTask`] gets a [`TaskId`], based on the `id_base` (to avoid collision with various task sources), /// and hashes of its template and [`TaskContext`], see [`ResolvedTask`] fields' documentation for more details. @@ -275,6 +273,53 @@ impl TaskTemplate { }, }) } + + /// Validates that all `$ZED_*` variables used in this template are known + /// variable names, returning a vector with all of the unique unknown + /// variables. + /// + /// Note that `$ZED_CUSTOM_*` variables are never considered to be invalid + /// since those are provided dynamically by extensions. + pub fn unknown_variables(&self) -> Vec { + let mut variables = HashSet::default(); + + Self::collect_unknown_variables(&self.label, &mut variables); + Self::collect_unknown_variables(&self.command, &mut variables); + + self.args + .iter() + .for_each(|arg| Self::collect_unknown_variables(arg, &mut variables)); + + self.env + .values() + .for_each(|value| Self::collect_unknown_variables(value, &mut variables)); + + if let Some(cwd) = &self.cwd { + Self::collect_unknown_variables(cwd, &mut variables); + } + + variables.into_iter().collect() + } + + fn collect_unknown_variables(template: &str, unknown: &mut HashSet) { + shellexpand::env_with_context_no_errors(template, |variable| { + // It's possible that the variable has a default defined, which is + // separated by a `:`, for example, `${ZED_FILE:default_value} so we + // ensure that we're only looking at the variable name itself. + let colon_position = variable.find(':').unwrap_or(variable.len()); + let variable_name = &variable[..colon_position]; + + if variable_name.starts_with(ZED_VARIABLE_NAME_PREFIX) + && let without_prefix = &variable_name[ZED_VARIABLE_NAME_PREFIX.len()..] + && !without_prefix.starts_with("CUSTOM_") + && variable_name.parse::().is_err() + { + unknown.insert(variable_name.to_string()); + } + + None::<&str> + }); + } } const MAX_DISPLAY_VARIABLE_LENGTH: usize = 15; @@ -327,7 +372,9 @@ fn substitute_all_template_variables_in_str>( substituted_variables: &mut HashSet, ) -> Option { let substituted_string = shellexpand::env_with_context(template_str, |var| { - // Colons denote a default value in case the variable is not set. We want to preserve that default, as otherwise shellexpand will substitute it for us. + // Colons denote a default value in case the variable is not set. We + // want to preserve that default, as otherwise shellexpand will + // substitute it for us. let colon_position = var.find(':').unwrap_or(var.len()); let (variable_name, default) = var.split_at(colon_position); if let Some(name) = task_variables.get(variable_name) { @@ -346,15 +393,19 @@ fn substitute_all_template_variables_in_str>( } } // This is an unknown variable. - // We should not error out, as they may come from user environment (e.g. $PATH). That means that the variable substitution might not be perfect. - // If there's a default, we need to return the string verbatim as otherwise shellexpand will apply that default for us. + // We should not error out, as they may come from user environment (e.g. + // $PATH). That means that the variable substitution might not be + // perfect. If there's a default, we need to return the string verbatim + // as otherwise shellexpand will apply that default for us. if !default.is_empty() { return Ok(Some(format!("${{{var}}}"))); } + // Else we can just return None and that variable will be left as is. Ok(None) }) .ok()?; + Some(substituted_string.into_owned()) } @@ -374,6 +425,7 @@ fn substitute_all_template_variables_in_vec( )?; expanded.push(new_value); } + Some(expanded) } @@ -424,6 +476,7 @@ fn substitute_all_template_variables_in_map( )?; new_map.insert(new_key, new_value); } + Some(new_map) } @@ -696,8 +749,8 @@ mod tests { project_env: HashMap::default(), }, ); - assert_eq!( - resolved_task_attempt, None, + assert!( + matches!(resolved_task_attempt, None), "If any of the Zed task variables is not substituted, the task should not be resolved, but got some resolution without the variable {removed_variable:?} (index {i})" ); } @@ -971,4 +1024,54 @@ mod tests { "Should fail when ZED variable has no default and doesn't exist" ); } + + #[test] + fn test_unknown_variables() { + // Variable names starting with `ZED_` that are not valid should be + // reported. + let label = "test unknown variables".to_string(); + let command = "$ZED_UNKNOWN".to_string(); + let task = TaskTemplate { + label, + command, + ..TaskTemplate::default() + }; + + assert_eq!(task.unknown_variables(), vec!["ZED_UNKNOWN".to_string()]); + + // Variable names starting with `ZED_CUSTOM_` should never be reported, + // as those are dynamically provided by extensions. + let label = "test custom variables".to_string(); + let command = "$ZED_CUSTOM_UNKNOWN".to_string(); + let task = TaskTemplate { + label, + command, + ..TaskTemplate::default() + }; + + assert!(task.unknown_variables().is_empty()); + + // Unknown variable names with defaults should still be reported, + // otherwise the default would always be silently used. + let label = "test custom variables".to_string(); + let command = "${ZED_UNKNOWN:default_value}".to_string(); + let task = TaskTemplate { + label, + command, + ..TaskTemplate::default() + }; + + assert_eq!(task.unknown_variables(), vec!["ZED_UNKNOWN".to_string()]); + + // Valid variable names are not reported. + let label = "test custom variables".to_string(); + let command = "$ZED_FILE".to_string(); + let task = TaskTemplate { + label, + command, + ..TaskTemplate::default() + }; + + assert!(task.unknown_variables().is_empty()); + } } diff --git a/crates/tasks_ui/src/modal.rs b/crates/tasks_ui/src/modal.rs index 644f82285b26f02a6011d59141b94de14a0e2bbf..f554253a0ef436cad985b5a7cbbb486cf8acbca8 100644 --- a/crates/tasks_ui/src/modal.rs +++ b/crates/tasks_ui/src/modal.rs @@ -124,7 +124,7 @@ impl TasksModalDelegate { pub struct TasksModal { pub picker: Entity>, - _subscription: [Subscription; 2], + _subscriptions: [Subscription; 2], } impl TasksModal { @@ -139,13 +139,18 @@ impl TasksModal { ) -> Self { let picker = cx.new(|cx| { Picker::uniform_list( - TasksModalDelegate::new(task_store, task_contexts, task_overrides, workspace), + TasksModalDelegate::new( + task_store.clone(), + task_contexts, + task_overrides, + workspace.clone(), + ), window, cx, ) .modal(is_modal) }); - let _subscription = [ + let mut _subscriptions = [ cx.subscribe(&picker, |_, _, _: &DismissEvent, cx| { cx.emit(DismissEvent); }), @@ -155,9 +160,10 @@ impl TasksModal { }); }), ]; + Self { picker, - _subscription, + _subscriptions, } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 9c3e2cb44fcdfe9a9caa861a5e27fc2bf37ad168..bcb34e65cd98645113e17a539a36e4fb5fd95995 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -1346,10 +1346,20 @@ impl Workspace { project::Event::Toast { notification_id, message, + link, } => this.show_notification( NotificationId::named(notification_id.clone()), cx, - |cx| cx.new(|cx| MessageNotification::new(message.clone(), cx)), + |cx| { + let mut notification = MessageNotification::new(message.clone(), cx); + if let Some(link) = link { + notification = notification + .more_info_message(link.label) + .more_info_url(link.url); + } + + cx.new(|_| notification) + }, ), project::Event::HideToast { notification_id } => {