From b4109a2376a9393693a7b95886091d48820e1628 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Thu, 8 May 2025 12:03:58 +0200 Subject: [PATCH] Prevent keybindings from triggering requests that should be disabled (#30221) Extracts authorization logic to a single method and add early returns in message handlers to prevent sending requests when the model configuration is invalid or terms haven't been accepted. This was allowing for the TOS popup to show up even for logged out users because they could bypass the disabled button with the keybinding. Now the behavior should be the same either way, that the request isn't made unless they can send it. The text thread already has a banner to tell the user to configure a model provider, so I don't think we need to pop up a separate modal, since the button is disabled anyway. Release Notes: - N/A --- .../src/context_editor.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/crates/assistant_context_editor/src/context_editor.rs b/crates/assistant_context_editor/src/context_editor.rs index c78e09464aac9bd9fd8d1da02b9a07a08b700699..229fcee0a3bebc2cfb2f2238e0aecdc7995fcd62 100644 --- a/crates/assistant_context_editor/src/context_editor.rs +++ b/crates/assistant_context_editor/src/context_editor.rs @@ -367,10 +367,16 @@ impl ContextEditor { } fn assist(&mut self, _: &Assist, window: &mut Window, cx: &mut Context) { + if self.sending_disabled(cx) { + return; + } self.send_to_model(RequestType::Chat, window, cx); } fn edit(&mut self, _: &Edit, window: &mut Window, cx: &mut Context) { + if self.sending_disabled(cx) { + return; + } self.send_to_model(RequestType::SuggestEdits, window, cx); } @@ -2438,17 +2444,8 @@ impl ContextEditor { None => (ButtonStyle::Filled, None), }; - let model = LanguageModelRegistry::read_global(cx).default_model(); - - let has_configuration_error = configuration_error(cx).is_some(); - let needs_to_accept_terms = self.show_accept_terms - && model - .as_ref() - .map_or(false, |model| model.provider.must_accept_terms(cx)); - let disabled = has_configuration_error || needs_to_accept_terms; - ButtonLike::new("send_button") - .disabled(disabled) + .disabled(self.sending_disabled(cx)) .style(style) .when_some(tooltip, |button, tooltip| { button.tooltip(move |_, _| tooltip.clone()) @@ -2470,6 +2467,20 @@ impl ContextEditor { }) } + /// Whether or not we should allow messages to be sent. + /// Will return false if the selected provided has a configuration error or + /// if the user has not accepted the terms of service for this provider. + fn sending_disabled(&self, cx: &mut Context<'_, ContextEditor>) -> bool { + let model = LanguageModelRegistry::read_global(cx).default_model(); + + let has_configuration_error = configuration_error(cx).is_some(); + let needs_to_accept_terms = self.show_accept_terms + && model + .as_ref() + .map_or(false, |model| model.provider.must_accept_terms(cx)); + has_configuration_error || needs_to_accept_terms + } + fn render_edit_button(&self, window: &mut Window, cx: &mut Context) -> impl IntoElement { let focus_handle = self.focus_handle(cx).clone(); @@ -2497,19 +2508,8 @@ impl ContextEditor { None => (ButtonStyle::Filled, None), }; - let provider = LanguageModelRegistry::read_global(cx) - .default_model() - .map(|default| default.provider); - - let has_configuration_error = configuration_error(cx).is_some(); - let needs_to_accept_terms = self.show_accept_terms - && provider - .as_ref() - .map_or(false, |provider| provider.must_accept_terms(cx)); - let disabled = has_configuration_error || needs_to_accept_terms; - ButtonLike::new("edit_button") - .disabled(disabled) + .disabled(self.sending_disabled(cx)) .style(style) .when_some(tooltip, |button, tooltip| { button.tooltip(move |_, _| tooltip.clone())