Restructure verification code prompting to open a window instead

Antonio Scandurra created

Also, prevent multiple calls to `sign_in` from racing with each other.

Change summary

crates/copilot/src/auth_modal.rs       |  84 ---------
crates/copilot/src/copilot.rs          | 238 ++++++++++++----------------
crates/copilot/src/request.rs          |   2 
crates/copilot/src/sign_in.rs          |  98 +++++++++++
crates/gpui/src/platform/mac/window.rs |   1 
5 files changed, 202 insertions(+), 221 deletions(-)

Detailed changes

crates/copilot/src/auth_modal.rs 🔗

@@ -1,84 +0,0 @@
-use gpui::{
-    elements::{Flex, Label, MouseEventHandler, ParentElement, Stack},
-    Axis, Element, Entity, View, ViewContext,
-};
-use settings::Settings;
-
-use crate::{Copilot, PromptingUser};
-
-pub enum Event {
-    Dismiss,
-}
-
-pub struct AuthModal {}
-
-impl Entity for AuthModal {
-    type Event = Event;
-}
-
-impl View for AuthModal {
-    fn ui_name() -> &'static str {
-        "AuthModal"
-    }
-
-    fn render(&mut self, cx: &mut gpui::RenderContext<'_, Self>) -> gpui::ElementBox {
-        let style = cx.global::<Settings>().theme.copilot.clone();
-
-        let user_code_and_url = Copilot::global(cx).read(cx).prompting_user().cloned();
-        let auth_text = style.auth_text.clone();
-        MouseEventHandler::<AuthModal>::new(0, cx, move |_state, cx| {
-            Stack::new()
-                .with_child(match user_code_and_url {
-                    Some(PromptingUser {
-                        user_code,
-                        verification_uri,
-                    }) => Flex::new(Axis::Vertical)
-                        .with_children([
-                            Label::new(user_code, auth_text.clone())
-                                .constrained()
-                                .with_width(540.)
-                                .boxed(),
-                            MouseEventHandler::<AuthModal>::new(1, cx, move |_state, _cx| {
-                                Label::new("Click here to open github!", auth_text.clone())
-                                    .constrained()
-                                    .with_width(540.)
-                                    .boxed()
-                            })
-                            .on_click(gpui::MouseButton::Left, move |_click, cx| {
-                                cx.platform().open_url(&verification_uri)
-                            })
-                            .with_cursor_style(gpui::CursorStyle::PointingHand)
-                            .boxed(),
-                        ])
-                        .boxed(),
-                    None => Label::new("Not signing in", style.auth_text.clone())
-                        .constrained()
-                        .with_width(540.)
-                        .boxed(),
-                })
-                .contained()
-                .with_style(style.auth_modal)
-                .constrained()
-                .with_max_width(540.)
-                .with_max_height(420.)
-                .named("Copilot Authentication status modal")
-        })
-        .on_hover(|_, _| {})
-        .on_click(gpui::MouseButton::Left, |_, _| {})
-        .on_click(gpui::MouseButton::Left, |_, _| {})
-        .boxed()
-    }
-
-    fn focus_out(&mut self, _: gpui::AnyViewHandle, cx: &mut ViewContext<Self>) {
-        cx.emit(Event::Dismiss)
-    }
-}
-
-impl AuthModal {
-    pub fn new(cx: &mut ViewContext<Self>) -> Self {
-        cx.observe(&Copilot::global(cx), |_, _, cx| cx.notify())
-            .detach();
-
-        AuthModal {}
-    }
-}

crates/copilot/src/copilot.rs 🔗

@@ -1,10 +1,10 @@
-mod auth_modal;
 mod request;
+mod sign_in;
 
 use anyhow::{anyhow, Result};
 use async_compression::futures::bufread::GzipDecoder;
-use auth_modal::AuthModal;
 use client::Client;
+use futures::{future::Shared, FutureExt, TryFutureExt};
 use gpui::{actions, AppContext, Entity, ModelContext, ModelHandle, MutableAppContext, Task};
 use language::{point_from_lsp, point_to_lsp, Anchor, Bias, Buffer, BufferSnapshot, ToPointUtf16};
 use lsp::LanguageServer;
@@ -18,61 +18,25 @@ use std::{
 use util::{
     fs::remove_matching, github::latest_github_release, http::HttpClient, paths, ResultExt,
 };
-use workspace::Workspace;
 
-actions!(copilot, [SignIn, SignOut, ToggleAuthStatus]);
+actions!(copilot, [SignIn, SignOut]);
 
 pub fn init(client: Arc<Client>, cx: &mut MutableAppContext) {
     let copilot = cx.add_model(|cx| Copilot::start(client.http_client(), cx));
     cx.set_global(copilot.clone());
-    cx.add_action(|_workspace: &mut Workspace, _: &SignIn, cx| {
+    cx.add_global_action(|_: &SignIn, cx| {
         let copilot = Copilot::global(cx);
-        if copilot.read(cx).status() == Status::Authorized {
-            return;
-        }
-
-        if !copilot.read(cx).has_subscription() {
-            let display_subscription =
-                cx.subscribe(&copilot, |workspace, _copilot, e, cx| match e {
-                    Event::PromptUserDeviceFlow => {
-                        workspace.toggle_modal(cx, |_workspace, cx| build_auth_modal(cx));
-                    }
-                });
-
-            copilot.update(cx, |copilot, _cx| {
-                copilot.set_subscription(display_subscription)
-            })
-        }
-
         copilot
             .update(cx, |copilot, cx| copilot.sign_in(cx))
             .detach_and_log_err(cx);
     });
-    cx.add_action(|workspace: &mut Workspace, _: &SignOut, cx| {
+    cx.add_global_action(|_: &SignOut, cx| {
         let copilot = Copilot::global(cx);
-
         copilot
             .update(cx, |copilot, cx| copilot.sign_out(cx))
             .detach_and_log_err(cx);
-
-        if workspace.modal::<AuthModal>().is_some() {
-            workspace.dismiss_modal(cx)
-        }
     });
-    cx.add_action(|workspace: &mut Workspace, _: &ToggleAuthStatus, cx| {
-        workspace.toggle_modal(cx, |_workspace, cx| build_auth_modal(cx))
-    })
-}
-
-fn build_auth_modal(cx: &mut gpui::ViewContext<Workspace>) -> gpui::ViewHandle<AuthModal> {
-    let modal = cx.add_view(|cx| AuthModal::new(cx));
-
-    cx.subscribe(&modal, |workspace, _, e: &auth_modal::Event, cx| match e {
-        auth_modal::Event::Dismiss => workspace.dismiss_modal(cx),
-    })
-    .detach();
-
-    modal
+    sign_in::init(cx);
 }
 
 enum CopilotServer {
@@ -84,40 +48,33 @@ enum CopilotServer {
     },
 }
 
-#[derive(Clone, Debug, PartialEq, Eq)]
-struct PromptingUser {
-    user_code: String,
-    verification_uri: String,
-}
-
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug)]
 enum SignInStatus {
-    Authorized { user: String },
-    Unauthorized { user: String },
-    PromptingUser(PromptingUser),
+    Authorized {
+        user: String,
+    },
+    Unauthorized {
+        user: String,
+    },
+    SigningIn {
+        prompt: Option<request::PromptUserDeviceFlow>,
+        task: Shared<Task<Result<(), Arc<anyhow::Error>>>>,
+    },
     SignedOut,
 }
 
-#[derive(Debug)]
-pub enum Event {
-    PromptUserDeviceFlow,
-}
-
 #[derive(Debug, PartialEq, Eq)]
 pub enum Status {
     Downloading,
     Error(Arc<str>),
     SignedOut,
+    SigningIn {
+        prompt: Option<request::PromptUserDeviceFlow>,
+    },
     Unauthorized,
     Authorized,
 }
 
-impl Status {
-    fn is_authorized(&self) -> bool {
-        matches!(self, Status::Authorized)
-    }
-}
-
 #[derive(Debug)]
 pub struct Completion {
     pub position: Anchor,
@@ -126,11 +83,10 @@ pub struct Completion {
 
 struct Copilot {
     server: CopilotServer,
-    _display_subscription: Option<gpui::Subscription>,
 }
 
 impl Entity for Copilot {
-    type Event = Event;
+    type Event = ();
 }
 
 impl Copilot {
@@ -138,15 +94,6 @@ impl Copilot {
         cx.global::<ModelHandle<Self>>().clone()
     }
 
-    fn has_subscription(&self) -> bool {
-        self._display_subscription.is_some()
-    }
-
-    fn set_subscription(&mut self, display_subscription: gpui::Subscription) {
-        debug_assert!(self._display_subscription.is_none());
-        self._display_subscription = Some(display_subscription);
-    }
-
     fn start(http: Arc<dyn HttpClient>, cx: &mut ModelContext<Self>) -> Self {
         // TODO: Don't eagerly download the LSP
         cx.spawn(|this, mut cx| async move {
@@ -184,57 +131,99 @@ impl Copilot {
 
         Self {
             server: CopilotServer::Downloading,
-            _display_subscription: None,
         }
     }
 
     fn sign_in(&mut self, cx: &mut ModelContext<Self>) -> Task<Result<()>> {
-        if let CopilotServer::Started { server, .. } = &self.server {
-            let server = server.clone();
-            cx.spawn(|this, mut cx| async move {
-                let sign_in = server
-                    .request::<request::SignInInitiate>(request::SignInInitiateParams {})
-                    .await?;
-                if let request::SignInInitiateResult::PromptUserDeviceFlow(flow) = sign_in {
-                    this.update(&mut cx, |this, cx| {
-                        this.update_prompting_user(
-                            flow.user_code.clone(),
-                            flow.verification_uri,
-                            cx,
-                        );
-
-                        cx.emit(Event::PromptUserDeviceFlow)
-                    });
-                    // TODO: catch an error here and clear the corresponding user code
-                    let response = server
-                        .request::<request::SignInConfirm>(request::SignInConfirmParams {
-                            user_code: flow.user_code,
+        if let CopilotServer::Started { server, status } = &mut self.server {
+            let task = match status {
+                SignInStatus::Authorized { .. } | SignInStatus::Unauthorized { .. } => {
+                    Task::ready(Ok(())).shared()
+                }
+                SignInStatus::SigningIn { task, .. } => task.clone(),
+                SignInStatus::SignedOut => {
+                    let server = server.clone();
+                    let task = cx
+                        .spawn(|this, mut cx| async move {
+                            let sign_in = async {
+                                let sign_in = server
+                                    .request::<request::SignInInitiate>(
+                                        request::SignInInitiateParams {},
+                                    )
+                                    .await?;
+                                match sign_in {
+                                    request::SignInInitiateResult::AlreadySignedIn { user } => {
+                                        Ok(request::SignInStatus::Ok { user })
+                                    }
+                                    request::SignInInitiateResult::PromptUserDeviceFlow(flow) => {
+                                        this.update(&mut cx, |this, cx| {
+                                            if let CopilotServer::Started { status, .. } =
+                                                &mut this.server
+                                            {
+                                                if let SignInStatus::SigningIn {
+                                                    prompt: prompt_flow,
+                                                    ..
+                                                } = status
+                                                {
+                                                    *prompt_flow = Some(flow.clone());
+                                                    cx.notify();
+                                                }
+                                            }
+                                        });
+                                        let response = server
+                                            .request::<request::SignInConfirm>(
+                                                request::SignInConfirmParams {
+                                                    user_code: flow.user_code,
+                                                },
+                                            )
+                                            .await?;
+                                        Ok(response)
+                                    }
+                                }
+                            };
+
+                            let sign_in = sign_in.await;
+                            this.update(&mut cx, |this, cx| match sign_in {
+                                Ok(status) => {
+                                    this.update_sign_in_status(status, cx);
+                                    Ok(())
+                                }
+                                Err(error) => {
+                                    this.update_sign_in_status(
+                                        request::SignInStatus::NotSignedIn,
+                                        cx,
+                                    );
+                                    Err(Arc::new(error))
+                                }
+                            })
                         })
-                        .await?;
-
-                    this.update(&mut cx, |this, cx| this.update_sign_in_status(response, cx));
+                        .shared();
+                    *status = SignInStatus::SigningIn {
+                        prompt: None,
+                        task: task.clone(),
+                    };
+                    cx.notify();
+                    task
                 }
-                anyhow::Ok(())
-            })
+            };
+
+            cx.foreground()
+                .spawn(task.map_err(|err| anyhow!("{:?}", err)))
         } else {
             Task::ready(Err(anyhow!("copilot hasn't started yet")))
         }
     }
 
     fn sign_out(&mut self, cx: &mut ModelContext<Self>) -> Task<Result<()>> {
-        if let CopilotServer::Started { server, .. } = &self.server {
+        if let CopilotServer::Started { server, status } = &mut self.server {
+            *status = SignInStatus::SignedOut;
+            cx.notify();
+
             let server = server.clone();
-            cx.spawn(|this, mut cx| async move {
+            cx.background().spawn(async move {
                 server
                     .request::<request::SignOut>(request::SignOutParams {})
                     .await?;
-                this.update(&mut cx, |this, cx| {
-                    if let CopilotServer::Started { status, .. } = &mut this.server {
-                        *status = SignInStatus::SignedOut;
-                        cx.notify();
-                    }
-                });
-
                 anyhow::Ok(())
             })
         } else {
@@ -305,38 +294,15 @@ impl Copilot {
             CopilotServer::Error(error) => Status::Error(error.clone()),
             CopilotServer::Started { status, .. } => match status {
                 SignInStatus::Authorized { .. } => Status::Authorized,
-                SignInStatus::Unauthorized { .. } | SignInStatus::PromptingUser { .. } => {
-                    Status::Unauthorized
-                }
+                SignInStatus::Unauthorized { .. } => Status::Unauthorized,
+                SignInStatus::SigningIn { prompt, .. } => Status::SigningIn {
+                    prompt: prompt.clone(),
+                },
                 SignInStatus::SignedOut => Status::SignedOut,
             },
         }
     }
 
-    pub fn prompting_user(&self) -> Option<&PromptingUser> {
-        if let CopilotServer::Started { status, .. } = &self.server {
-            if let SignInStatus::PromptingUser(prompt) = status {
-                return Some(prompt);
-            }
-        }
-        None
-    }
-
-    fn update_prompting_user(
-        &mut self,
-        user_code: String,
-        verification_uri: String,
-        cx: &mut ModelContext<Self>,
-    ) {
-        if let CopilotServer::Started { status, .. } = &mut self.server {
-            *status = SignInStatus::PromptingUser(PromptingUser {
-                user_code,
-                verification_uri,
-            });
-            cx.notify();
-        }
-    }
-
     fn update_sign_in_status(
         &mut self,
         lsp_status: request::SignInStatus,

crates/copilot/src/request.rs 🔗

@@ -26,7 +26,7 @@ pub enum SignInInitiateResult {
     PromptUserDeviceFlow(PromptUserDeviceFlow),
 }
 
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct PromptUserDeviceFlow {
     pub user_code: String,

crates/copilot/src/sign_in.rs 🔗

@@ -0,0 +1,98 @@
+use crate::{request::PromptUserDeviceFlow, Copilot};
+use gpui::{
+    elements::*,
+    geometry::{rect::RectF, vector::vec2f},
+    Axis, Element, Entity, MutableAppContext, View, ViewContext, WindowKind, WindowOptions,
+};
+use settings::Settings;
+
+pub fn init(cx: &mut MutableAppContext) {
+    let copilot = Copilot::global(cx);
+
+    let mut code_verification_window_id = None;
+    cx.observe(&copilot, move |copilot, cx| {
+        match copilot.read(cx).status() {
+            crate::Status::SigningIn {
+                prompt: Some(prompt),
+            } => {
+                if let Some(window_id) = code_verification_window_id.take() {
+                    cx.remove_window(window_id);
+                }
+
+                let screen = cx.platform().screens().pop();
+                let (window_id, _) = cx.add_window(
+                    WindowOptions {
+                        bounds: gpui::WindowBounds::Fixed(RectF::new(
+                            vec2f(100., 100.),
+                            vec2f(300., 300.),
+                        )),
+                        titlebar: None,
+                        center: false,
+                        focus: false,
+                        kind: WindowKind::Normal,
+                        is_movable: true,
+                        screen,
+                    },
+                    |_| CopilotCodeVerification::new(prompt),
+                );
+                code_verification_window_id = Some(window_id);
+            }
+            _ => {
+                if let Some(window_id) = code_verification_window_id.take() {
+                    cx.remove_window(window_id);
+                }
+            }
+        }
+    })
+    .detach();
+}
+
+pub enum Event {
+    Dismiss,
+}
+
+pub struct CopilotCodeVerification {
+    prompt: PromptUserDeviceFlow,
+}
+
+impl Entity for CopilotCodeVerification {
+    type Event = Event;
+}
+
+impl View for CopilotCodeVerification {
+    fn ui_name() -> &'static str {
+        "CopilotCodeVerification"
+    }
+
+    fn render(&mut self, cx: &mut gpui::RenderContext<'_, Self>) -> gpui::ElementBox {
+        let style = cx.global::<Settings>().theme.copilot.clone();
+
+        let auth_text = style.auth_text.clone();
+        let prompt = self.prompt.clone();
+        Flex::new(Axis::Vertical)
+            .with_child(Label::new(prompt.user_code.clone(), auth_text.clone()).boxed())
+            .with_child(
+                MouseEventHandler::<Self>::new(1, cx, move |_state, _cx| {
+                    Label::new("Click here to open GitHub!", auth_text.clone()).boxed()
+                })
+                .on_click(gpui::MouseButton::Left, move |_click, cx| {
+                    cx.platform().open_url(&prompt.verification_uri)
+                })
+                .with_cursor_style(gpui::CursorStyle::PointingHand)
+                .boxed(),
+            )
+            .contained()
+            .with_style(style.auth_modal)
+            .named("Copilot Authentication status modal")
+    }
+
+    fn focus_out(&mut self, _: gpui::AnyViewHandle, cx: &mut ViewContext<Self>) {
+        cx.emit(Event::Dismiss)
+    }
+}
+
+impl CopilotCodeVerification {
+    pub fn new(prompt: PromptUserDeviceFlow) -> Self {
+        CopilotCodeVerification { prompt }
+    }
+}

crates/gpui/src/platform/mac/window.rs 🔗

@@ -473,6 +473,7 @@ impl Window {
                 WindowBounds::Fixed(rect) => {
                     let screen_frame = screen.visibleFrame();
                     let ns_rect = rect.to_ns_rect();
+                    dbg!(screen_frame.as_CGRect(), ns_rect.as_CGRect());
                     if ns_rect.intersects(screen_frame) {
                         native_window.setFrame_display_(ns_rect, YES);
                     } else {