Prevent keybindings from triggering requests that should be disabled (#30221)

Ben Brandt created 7 months ago

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

Change summary

crates/assistant_context_editor/src/context_editor.rs | 44 ++++++------
1 file changed, 22 insertions(+), 22 deletions(-)

Detailed changes

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<Self>) {
+        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<Self>) {
+        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<Self>) -> 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())