From b0f80cafc361ffd96ffbba0ff1726f90afeffbdf Mon Sep 17 00:00:00 2001
From: mbecker20 <becker.maxh@gmail.com>
Date: Sun, 11 Aug 2024 14:59:34 -0700
Subject: [PATCH] improve action responsiveness by improving when update is
 sent out rel to action state set

---
 bin/core/src/api/execute/build.rs             |  26 ++--
 bin/core/src/api/execute/deployment.rs        | 147 +++++++++++++-----
 bin/core/src/api/execute/procedure.rs         |   2 +
 bin/core/src/api/execute/repo.rs              |  20 +--
 bin/core/src/api/execute/server.rs            |   8 +
 bin/core/src/api/execute/stack.rs             |   2 +
 bin/core/src/api/execute/sync.rs              |   3 +
 bin/core/src/helpers/stack/execute.rs         |   3 +
 bin/core/src/helpers/update.rs                |  20 ++-
 .../components/resources/stack/actions.tsx    |  26 ++--
 .../src/components/resources/stack/info.tsx   |  12 +-
 11 files changed, 188 insertions(+), 81 deletions(-)

diff --git a/bin/core/src/api/execute/build.rs b/bin/core/src/api/execute/build.rs
index 27ca6b391..200a80b89 100644
--- a/bin/core/src/api/execute/build.rs
+++ b/bin/core/src/api/execute/build.rs
@@ -70,6 +70,19 @@ impl Resolve<RunBuild, (User, Update)> for State {
       return Err(anyhow!("Must attach builder to RunBuild"));
     }
 
+    // get the action state for the build (or insert default).
+    let action_state =
+      action_states().build.get_or_insert_default(&build.id).await;
+
+    // This will set action state back to default when dropped.
+    // Will also check to ensure build not already busy before updating.
+    let _action_guard =
+      action_state.update(|state| state.building = true)?;
+
+    build.config.version.increment();
+    update.version = build.config.version;
+    update_update(update.clone()).await?;
+
     let git_token = git_token(
       &build.config.git_provider,
       &build.config.git_account,
@@ -80,22 +93,9 @@ impl Resolve<RunBuild, (User, Update)> for State {
       || format!("Failed to get git token in call to db. This is a database error, not a token exisitence error. Stopping run. | {} | {}", build.config.git_provider, build.config.git_account),
     )?;
 
-    // get the action state for the build (or insert default).
-    let action_state =
-      action_states().build.get_or_insert_default(&build.id).await;
-
-    // This will set action state back to default when dropped.
-    // Will also check to ensure build not already busy before updating.
-    let _action_guard =
-      action_state.update(|state| state.building = true)?;
-
     let (registry_token, aws_ecr) =
       validate_account_extract_registry_token_aws_ecr(&build).await?;
 
-    build.config.version.increment();
-    update.version = build.config.version;
-    update_update(update.clone()).await?;
-
     let cancel = CancellationToken::new();
     let cancel_clone = cancel.clone();
     let mut cancel_recv =
diff --git a/bin/core/src/api/execute/deployment.rs b/bin/core/src/api/execute/deployment.rs
index 079674e3c..871c21956 100644
--- a/bin/core/src/api/execute/deployment.rs
+++ b/bin/core/src/api/execute/deployment.rs
@@ -6,8 +6,7 @@ use monitor_client::{
     build::{Build, ImageRegistry},
     config::core::AwsEcrConfig,
     deployment::{
-      extract_registry_domain, Deployment, DeploymentActionState,
-      DeploymentImage,
+      extract_registry_domain, Deployment, DeploymentImage,
     },
     get_image_name,
     permission::PermissionLevel,
@@ -36,7 +35,6 @@ use crate::{
 async fn setup_deployment_execution(
   deployment: &str,
   user: &User,
-  set_in_progress: impl Fn(&mut DeploymentActionState),
 ) -> anyhow::Result<(Deployment, Server)> {
   let deployment = resource::get_check_permissions::<Deployment>(
     deployment,
@@ -49,16 +47,6 @@ async fn setup_deployment_execution(
     return Err(anyhow!("deployment has no server configured"));
   }
 
-  // get the action state for the deployment (or insert default).
-  let action_state = action_states()
-    .deployment
-    .get_or_insert_default(&deployment.id)
-    .await;
-
-  // Will check to ensure deployment not already busy before updating, and return Err if so.
-  // The returned guard will set the action state back to default when dropped.
-  let _action_guard = action_state.update(set_in_progress)?;
-
   let (server, status) =
     get_server_with_status(&deployment.config.server_id).await?;
   if status != ServerState::Ok {
@@ -82,10 +70,21 @@ impl Resolve<Deploy, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (mut deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.deploying = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.deploying = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -234,10 +233,21 @@ impl Resolve<StartContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.starting = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.starting = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -271,10 +281,21 @@ impl Resolve<RestartContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.restarting = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.restarting = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -310,10 +331,21 @@ impl Resolve<PauseContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.pausing = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.pausing = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -347,10 +379,21 @@ impl Resolve<UnpauseContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.unpausing = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.unpausing = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -390,10 +433,21 @@ impl Resolve<StopContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.stopping = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.stopping = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
@@ -437,10 +491,21 @@ impl Resolve<RemoveContainer, (User, Update)> for State {
     (user, mut update): (User, Update),
   ) -> anyhow::Result<Update> {
     let (deployment, server) =
-      setup_deployment_execution(&deployment, &user, |state| {
-        state.removing = true
-      })
-      .await?;
+      setup_deployment_execution(&deployment, &user).await?;
+
+    // get the action state for the deployment (or insert default).
+    let action_state = action_states()
+      .deployment
+      .get_or_insert_default(&deployment.id)
+      .await;
+
+    // Will check to ensure deployment not already busy before updating, and return Err if so.
+    // The returned guard will set the action state back to default when dropped.
+    let _action_guard =
+      action_state.update(|state| state.removing = true)?;
+
+    // Send update after setting action state, this way frontend gets correct state.
+    update_update(update.clone()).await?;
 
     let periphery = periphery_client(&server)?;
 
diff --git a/bin/core/src/api/execute/procedure.rs b/bin/core/src/api/execute/procedure.rs
index 4769a215b..975e078a7 100644
--- a/bin/core/src/api/execute/procedure.rs
+++ b/bin/core/src/api/execute/procedure.rs
@@ -69,6 +69,8 @@ fn resolve_inner(
     let _action_guard =
       action_state.update(|state| state.running = true)?;
 
+    update_update(update.clone()).await?;
+
     let update = Mutex::new(update);
 
     let res = execute_procedure(&procedure, &update).await;
diff --git a/bin/core/src/api/execute/repo.rs b/bin/core/src/api/execute/repo.rs
index ce00be504..cbf5b3870 100644
--- a/bin/core/src/api/execute/repo.rs
+++ b/bin/core/src/api/execute/repo.rs
@@ -54,6 +54,17 @@ impl Resolve<CloneRepo, (User, Update)> for State {
     )
     .await?;
 
+    // get the action state for the repo (or insert default).
+    let action_state =
+      action_states().repo.get_or_insert_default(&repo.id).await;
+
+    // This will set action state back to default when dropped.
+    // Will also check to ensure repo not already busy before updating.
+    let _action_guard =
+      action_state.update(|state| state.cloning = true)?;
+
+    update_update(update.clone()).await?;
+
     let git_token = git_token(
       &repo.config.git_provider,
       &repo.config.git_account,
@@ -64,15 +75,6 @@ impl Resolve<CloneRepo, (User, Update)> for State {
       || format!("Failed to get git token in call to db. This is a database error, not a token exisitence error. Stopping run. | {} | {}", repo.config.git_provider, repo.config.git_account),
     )?;
 
-    // get the action state for the repo (or insert default).
-    let action_state =
-      action_states().repo.get_or_insert_default(&repo.id).await;
-
-    // This will set action state back to default when dropped.
-    // Will also check to ensure repo not already busy before updating.
-    let _action_guard =
-      action_state.update(|state| state.cloning = true)?;
-
     if repo.config.server_id.is_empty() {
       return Err(anyhow!("repo has no server attached"));
     }
diff --git a/bin/core/src/api/execute/server.rs b/bin/core/src/api/execute/server.rs
index 4a73dce7c..da383ccc3 100644
--- a/bin/core/src/api/execute/server.rs
+++ b/bin/core/src/api/execute/server.rs
@@ -47,6 +47,8 @@ impl Resolve<StopAllContainers, (User, Update)> for State {
     let _action_guard = action_state
       .update(|state| state.stopping_containers = true)?;
 
+    update_update(update.clone()).await?;
+
     let logs = periphery_client(&server)?
       .request(api::container::StopAllContainers {})
       .await
@@ -90,6 +92,8 @@ impl Resolve<PruneContainers, (User, Update)> for State {
     let _action_guard =
       action_state.update(|state| state.pruning_containers = true)?;
 
+    update_update(update.clone()).await?;
+
     let periphery = periphery_client(&server)?;
 
     let log = match periphery
@@ -144,6 +148,8 @@ impl Resolve<PruneNetworks, (User, Update)> for State {
     let _action_guard =
       action_state.update(|state| state.pruning_networks = true)?;
 
+    update_update(update.clone()).await?;
+
     let periphery = periphery_client(&server)?;
 
     let log = match periphery
@@ -196,6 +202,8 @@ impl Resolve<PruneImages, (User, Update)> for State {
     let _action_guard =
       action_state.update(|state| state.pruning_images = true)?;
 
+    update_update(update.clone()).await?;
+
     let periphery = periphery_client(&server)?;
 
     let log =
diff --git a/bin/core/src/api/execute/stack.rs b/bin/core/src/api/execute/stack.rs
index d7d86654d..fd0c88fb7 100644
--- a/bin/core/src/api/execute/stack.rs
+++ b/bin/core/src/api/execute/stack.rs
@@ -48,6 +48,8 @@ impl Resolve<DeployStack, (User, Update)> for State {
     let _action_guard =
       action_state.update(|state| state.deploying = true)?;
 
+    update_update(update.clone()).await?;
+
     let git_token = crate::helpers::git_token(
       &stack.config.git_provider,
       &stack.config.git_account,
diff --git a/bin/core/src/api/execute/sync.rs b/bin/core/src/api/execute/sync.rs
index a0b03ce9d..22c0398f3 100644
--- a/bin/core/src/api/execute/sync.rs
+++ b/bin/core/src/api/execute/sync.rs
@@ -58,6 +58,9 @@ impl Resolve<RunSync, (User, Update)> for State {
       return Err(anyhow!("resource sync repo not configured"));
     }
 
+    // Send update here for FE to recheck action state
+    update_update(update.clone()).await?;
+
     let (res, logs, hash, message) =
       crate::helpers::sync::remote::get_remote_resources(&sync)
         .await
diff --git a/bin/core/src/helpers/stack/execute.rs b/bin/core/src/helpers/stack/execute.rs
index 2ec955544..140f81095 100644
--- a/bin/core/src/helpers/stack/execute.rs
+++ b/bin/core/src/helpers/stack/execute.rs
@@ -48,6 +48,9 @@ pub async fn execute_compose<T: ExecuteCompose>(
   // The returned guard will set the action state back to default when dropped.
   let _action_guard = action_state.update(set_in_progress)?;
 
+  // Send update here for frontend to recheck action state
+  update_update(update.clone()).await?;
+
   let periphery = periphery_client(&server)?;
 
   if let Some(service) = &service {
diff --git a/bin/core/src/helpers/update.rs b/bin/core/src/helpers/update.rs
index 2539476f7..f10182fb2 100644
--- a/bin/core/src/helpers/update.rs
+++ b/bin/core/src/helpers/update.rs
@@ -59,6 +59,23 @@ pub async fn add_update(
   Ok(id)
 }
 
+#[instrument(level = "debug")]
+pub async fn add_update_without_send(
+  update: &Update,
+) -> anyhow::Result<String> {
+  let id = db_client()
+    .await
+    .updates
+    .insert_one(update)
+    .await
+    .context("failed to insert update into db")?
+    .inserted_id
+    .as_object_id()
+    .context("inserted_id is not object id")?
+    .to_string();
+  Ok(id)
+}
+
 #[instrument(level = "debug")]
 pub async fn update_update(update: Update) -> anyhow::Result<()> {
   update_one_by_id(&db_client().await.updates, &update.id, mungos::update::Update::Set(to_document(&update)?), None)
@@ -312,6 +329,7 @@ pub async fn init_execution_update(
   };
   let mut update = make_update(target, operation, user);
   update.in_progress();
-  update.id = add_update(update.clone()).await?;
+  // Don't actually send it here, let the handlers send it after they can set action state.
+  update.id = add_update_without_send(&update).await?;
   Ok(update)
 }
diff --git a/frontend/src/components/resources/stack/actions.tsx b/frontend/src/components/resources/stack/actions.tsx
index 74937f079..7050096c7 100644
--- a/frontend/src/components/resources/stack/actions.tsx
+++ b/frontend/src/components/resources/stack/actions.tsx
@@ -1,6 +1,14 @@
 import { ActionWithDialog, ConfirmButton } from "@components/util";
 import { useExecute, useInvalidate, useRead, useWrite } from "@lib/hooks";
-import { Pause, Pen, Play, RefreshCcw, Rocket, Square, Trash2 } from "lucide-react";
+import {
+  Pause,
+  Pen,
+  Play,
+  RefreshCcw,
+  Rocket,
+  Square,
+  Trash2,
+} from "lucide-react";
 import { useStack } from ".";
 import { Types } from "@monitor/client";
 import { useToast } from "@ui/use-toast";
@@ -20,8 +28,6 @@ export const DeployStack = ({ id }: { id: string }) => {
   if (!stack || state === Types.StackState.Unknown) {
     return null;
   }
-
-  const pending = isPending || deploying;
   const deployed =
     state !== undefined &&
     [
@@ -39,8 +45,8 @@ export const DeployStack = ({ id }: { id: string }) => {
         title="Redeploy"
         icon={<Rocket className="h-4 w-4" />}
         onClick={() => deploy({ stack: id })}
-        disabled={pending}
-        loading={pending}
+        disabled={isPending}
+        loading={isPending || deploying}
       />
     );
   }
@@ -50,8 +56,8 @@ export const DeployStack = ({ id }: { id: string }) => {
       title="Deploy"
       icon={<Rocket className="w-4 h-4" />}
       onClick={() => deploy({ stack: id })}
-      disabled={pending}
-      loading={pending}
+      disabled={isPending}
+      loading={isPending || deploying}
     />
   );
 };
@@ -74,8 +80,6 @@ export const DestroyStack = ({ id }: { id: string }) => {
     return null;
   }
 
-  const pending = isPending || destroying;
-
   if (!stack) {
     return null;
   }
@@ -86,8 +90,8 @@ export const DestroyStack = ({ id }: { id: string }) => {
       title="Destroy"
       icon={<Trash2 className="h-4 w-4" />}
       onClick={() => destroy({ stack: id })}
-      disabled={pending}
-      loading={pending}
+      disabled={isPending}
+      loading={isPending || destroying}
     />
   );
 };
diff --git a/frontend/src/components/resources/stack/info.tsx b/frontend/src/components/resources/stack/info.tsx
index d0f7c8bc3..09048b5af 100644
--- a/frontend/src/components/resources/stack/info.tsx
+++ b/frontend/src/components/resources/stack/info.tsx
@@ -22,8 +22,8 @@ export const StackInfo = ({
         <Card>
           <CardHeader>
             deployed contents:{" "}
-            {stack?.info?.deployed_contents?.map((content) => (
-              <pre className="flex flex-col gap-2">
+            {stack?.info?.deployed_contents?.map((content, i) => (
+              <pre key={i} className="flex flex-col gap-2">
                 path: {content.path}
                 <pre>{content.contents}</pre>
               </pre>
@@ -48,8 +48,8 @@ export const StackInfo = ({
           <Card>
             <CardHeader>
               latest contents:{" "}
-              {stack?.info?.remote_contents?.map((content) => (
-                <pre className="flex flex-col gap-2">
+              {stack?.info?.remote_contents?.map((content, i) => (
+                <pre key={i} className="flex flex-col gap-2">
                   path: {content.path}
                   <pre>{content.contents}</pre>
                 </pre>
@@ -62,8 +62,8 @@ export const StackInfo = ({
         <Card>
           <CardHeader>
             remote errors:{" "}
-            {stack?.info?.remote_errors?.map((content) => (
-              <pre className="flex flex-col gap-2">
+            {stack?.info?.remote_errors?.map((content, i) => (
+              <pre key={i} className="flex flex-col gap-2">
                 path: {content.path}
                 <pre>{content.contents}</pre>
               </pre>