From 648d0054de1075863f22f00521b1d4ec5a92193d Mon Sep 17 00:00:00 2001 From: Shardul Vaidya <31039336+5herlocked@users.noreply.github.com> Date: Thu, 8 May 2025 18:09:18 -0400 Subject: [PATCH] bedrock: Fix UX bug (#28350) Closes #29072, #28390, Release Notes: - AWS Bedrock: Fixed case where user couldn't delete manually added AWS credentials. --------- Co-authored-by: Marshall Bowers Co-authored-by: Peter Tripp --- .../language_models/src/provider/bedrock.rs | 82 +++++++------------ 1 file changed, 31 insertions(+), 51 deletions(-) diff --git a/crates/language_models/src/provider/bedrock.rs b/crates/language_models/src/provider/bedrock.rs index 76d3f4346b..f675250609 100644 --- a/crates/language_models/src/provider/bedrock.rs +++ b/crates/language_models/src/provider/bedrock.rs @@ -76,8 +76,6 @@ pub struct AmazonBedrockSettings { pub enum BedrockAuthMethod { #[serde(rename = "named_profile")] NamedProfile, - #[serde(rename = "static_credentials")] - StaticCredentials, #[serde(rename = "sso")] SingleSignOn, /// IMDSv2, PodIdentity, env vars, etc. @@ -186,47 +184,18 @@ impl State { }) } - fn is_authenticated(&self) -> Option { - match self + fn is_authenticated(&self) -> bool { + let derived = self .settings .as_ref() - .and_then(|s| s.authentication_method.as_ref()) - { - Some(BedrockAuthMethod::StaticCredentials) => Some(String::from( - "You are authenticated using Static Credentials.", - )), - Some(BedrockAuthMethod::NamedProfile) | Some(BedrockAuthMethod::SingleSignOn) => { - match self.settings.as_ref() { - None => Some(String::from( - "You are authenticated using a Named Profile, but no profile is set.", - )), - Some(settings) => match settings.clone().profile_name { - None => Some(String::from( - "You are authenticated using a Named Profile, but no profile is set.", - )), - Some(profile_name) => Some(format!( - "You are authenticated using a Named Profile: {profile_name}", - )), - }, - } - } - Some(BedrockAuthMethod::Automatic) => Some(String::from( - "You are authenticated using Automatic Credentials.", - )), - None => { - if self.credentials.is_some() { - Some(String::from( - "You are authenticated using Static Credentials.", - )) - } else { - None - } - } - } + .and_then(|s| s.authentication_method.as_ref()); + let creds = self.credentials.as_ref(); + + derived.is_some() || creds.is_some() } fn authenticate(&self, cx: &mut Context) -> Task> { - if self.is_authenticated().is_some() { + if self.is_authenticated() { return Task::ready(Ok(())); } @@ -328,6 +297,7 @@ impl LanguageModelProvider for BedrockLanguageModelProvider { for model in bedrock::Model::iter() { if !matches!(model, bedrock::Model::Custom { .. }) { + // TODO: Sonnet 3.7 vs. 3.7 Thinking bug is here. models.insert(model.id().to_string(), model); } } @@ -357,7 +327,7 @@ impl LanguageModelProvider for BedrockLanguageModelProvider { } fn is_authenticated(&self, cx: &App) -> bool { - self.state.read(cx).is_authenticated().is_some() + self.state.read(cx).is_authenticated() } fn authenticate(&self, cx: &mut App) -> Task> { @@ -402,8 +372,7 @@ impl BedrockModel { let auth_method = state .settings .as_ref() - .and_then(|s| s.authentication_method.clone()) - .unwrap_or(BedrockAuthMethod::Automatic); + .and_then(|s| s.authentication_method.clone()); let endpoint = state.settings.as_ref().and_then(|s| s.endpoint.clone()); @@ -438,7 +407,7 @@ impl BedrockModel { } match auth_method { - BedrockAuthMethod::StaticCredentials => { + None => { if let Some(creds) = credentials { let aws_creds = Credentials::new( creds.access_key_id, @@ -450,7 +419,8 @@ impl BedrockModel { config_builder = config_builder.credentials_provider(aws_creds); } } - BedrockAuthMethod::NamedProfile | BedrockAuthMethod::SingleSignOn => { + Some(BedrockAuthMethod::NamedProfile) + | Some(BedrockAuthMethod::SingleSignOn) => { // Currently NamedProfile and SSO behave the same way but only the instructions change // Until we support BearerAuth through SSO, this will not change. let profile_name = settings @@ -461,7 +431,7 @@ impl BedrockModel { config_builder = config_builder.profile_name(profile_name); } } - BedrockAuthMethod::Automatic => { + Some(BedrockAuthMethod::Automatic) => { // Use default credential provider chain } } @@ -1211,7 +1181,7 @@ impl ConfigurationView { .rounded_sm() } - fn should_render_editor(&self, cx: &mut Context) -> Option { + fn should_render_editor(&self, cx: &Context) -> bool { self.state.read(cx).is_authenticated() } } @@ -1219,13 +1189,16 @@ impl ConfigurationView { impl Render for ConfigurationView { fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { let env_var_set = self.state.read(cx).credentials_from_env; - let creds_type = self.should_render_editor(cx).is_some(); + let bedrock_settings = self.state.read(cx).settings.as_ref(); + let bedrock_method = bedrock_settings + .as_ref() + .and_then(|s| s.authentication_method.clone()); if self.load_credentials_task.is_some() { return div().child(Label::new("Loading credentials...")).into_any(); } - if let Some(auth) = self.should_render_editor(cx) { + if self.should_render_editor(cx) { return h_flex() .mt_1() .p_1() @@ -1241,7 +1214,14 @@ impl Render for ConfigurationView { .child(Label::new(if env_var_set { format!("Access Key ID is set in {ZED_BEDROCK_ACCESS_KEY_ID_VAR}, Secret Key is set in {ZED_BEDROCK_SECRET_ACCESS_KEY_VAR}, Region is set in {ZED_BEDROCK_REGION_VAR} environment variables.") } else { - auth.clone() + match bedrock_method { + Some(BedrockAuthMethod::Automatic) => "You are using automatic credentials".into(), + Some(BedrockAuthMethod::NamedProfile) => { + "You are using named profile".into() + }, + Some(BedrockAuthMethod::SingleSignOn) => "You are using a single sign on profile".into(), + None => "You are using static credentials".into(), + } })), ) .child( @@ -1249,12 +1229,12 @@ impl Render for ConfigurationView { .icon(Some(IconName::Trash)) .icon_size(IconSize::Small) .icon_position(IconPosition::Start) - // .disabled(env_var_set || creds_type) + .disabled(env_var_set || bedrock_method.is_some()) .when(env_var_set, |this| { this.tooltip(Tooltip::text(format!("To reset your credentials, unset the {ZED_BEDROCK_ACCESS_KEY_ID_VAR}, {ZED_BEDROCK_SECRET_ACCESS_KEY_VAR}, and {ZED_BEDROCK_REGION_VAR} environment variables."))) }) - .when(creds_type, |this| { - this.tooltip(Tooltip::text("You cannot reset credentials as they're being derived, check Zed settings to understand how.")) + .when(bedrock_method.is_some(), |this| { + this.tooltip(Tooltip::text("You cannot reset credentials as they're being derived, check Zed settings to understand how")) }) .on_click(cx.listener(|this, _, window, cx| this.reset_credentials(window, cx))), )