From 96c119ed46051c79649916826cdf43e8a1a773cd Mon Sep 17 00:00:00 2001
From: Cyrus Goh <hello@cyrusgoh.com>
Date: Tue, 3 Dec 2024 15:46:34 -0800
Subject: [PATCH] Allow variable creation with different target environments 
 (#6168)

* fix value cell click target

* improve existing keys and env check, utils

* comment out client side special char validation

* clean up

* lint

* check for dups earlier, lint fix

* lint

* update regex

* reset after submission

* fix reset bug in edit dialog

* update copy

* duplicate fix in add dialog

* lint

* move check existing to submit instead of onblur

* feedback
---
 .../environment-variables/ActionsCell.svelte  |  3 +-
 .../environment-variables/ActivityCell.svelte |  2 +-
 .../environment-variables/AddDialog.svelte    | 65 ++++++++------
 .../environment-variables/EditDialog.svelte   | 89 ++++++++++---------
 .../EnvironmentVariablesTable.svelte          |  3 +-
 .../environment-variables/KeyCell.svelte      |  6 +-
 .../projects/environment-variables/types.ts   |  8 +-
 .../projects/environment-variables/utils.ts   | 77 ++++++++++++++++
 .../environment-variables/+page.svelte        |  8 +-
 9 files changed, 179 insertions(+), 82 deletions(-)
 create mode 100644 web-admin/src/features/projects/environment-variables/utils.ts

diff --git a/web-admin/src/features/projects/environment-variables/ActionsCell.svelte b/web-admin/src/features/projects/environment-variables/ActionsCell.svelte
index ad31bc22a27..2a2cf6b5ddb 100644
--- a/web-admin/src/features/projects/environment-variables/ActionsCell.svelte
+++ b/web-admin/src/features/projects/environment-variables/ActionsCell.svelte
@@ -5,12 +5,13 @@
   import { Trash2Icon, Pencil } from "lucide-svelte";
   import EditDialog from "./EditDialog.svelte";
   import DeleteDialog from "./DeleteDialog.svelte";
+  import type { VariableNames } from "./types";
 
   export let id: string;
   export let environment: string;
   export let name: string;
   export let value: string;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let isDropdownOpen = false;
   let isEditDialogOpen = false;
diff --git a/web-admin/src/features/projects/environment-variables/ActivityCell.svelte b/web-admin/src/features/projects/environment-variables/ActivityCell.svelte
index 94a3104dd96..201f9095c83 100644
--- a/web-admin/src/features/projects/environment-variables/ActivityCell.svelte
+++ b/web-admin/src/features/projects/environment-variables/ActivityCell.svelte
@@ -20,7 +20,7 @@
 </script>
 
 <Tooltip distance={8} location="top" alignment="start">
-  <div class="text-xs text-gray-500 cursor-pointer">
+  <div class="text-xs text-gray-500 cursor-pointer w-fit">
     {formattedText}
   </div>
   <TooltipContent slot="tooltip-content">
diff --git a/web-admin/src/features/projects/environment-variables/AddDialog.svelte b/web-admin/src/features/projects/environment-variables/AddDialog.svelte
index 2bb17716d62..ce5692c7466 100644
--- a/web-admin/src/features/projects/environment-variables/AddDialog.svelte
+++ b/web-admin/src/features/projects/environment-variables/AddDialog.svelte
@@ -22,13 +22,14 @@
   import { defaults, superForm } from "sveltekit-superforms";
   import { yup } from "sveltekit-superforms/adapters";
   import { object, string, array } from "yup";
-  import { EnvironmentType } from "./types";
+  import { type VariableNames } from "./types";
   import Input from "@rilldata/web-common/components/forms/Input.svelte";
   import IconButton from "@rilldata/web-common/components/button/IconButton.svelte";
   import { Trash2Icon, UploadIcon } from "lucide-svelte";
+  import { getCurrentEnvironment, isDuplicateKey } from "./utils";
 
   export let open = false;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let inputErrors: { [key: number]: boolean } = {};
   let isKeyAlreadyExists = false;
@@ -61,8 +62,9 @@
           key: string()
             .optional()
             .matches(
-              /^[a-zA-Z0-9_]+$/,
-              "Key must only contain letters, numbers, and underscores.",
+              /^[a-zA-Z_][a-zA-Z0-9_.]*$/,
+              // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12
+              "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots",
             ),
           value: string().optional(),
         }),
@@ -80,12 +82,18 @@
         if (!form.valid) return;
         const values = form.data;
 
-        // Omit draft variables that do not have a key
+        // Check for duplicates before proceeding
+        const duplicates = checkForExistingKeys();
+        if (duplicates > 0) {
+          // Early return without resetting the form
+          return;
+        }
+
+        // Only filter and process if there are no duplicates
         const filteredVariables = values.variables.filter(
           ({ key }) => key !== "",
         );
 
-        // Flatten the variables to match the schema
         const flatVariables = Object.fromEntries(
           filteredVariables.map(({ key, value }) => [key, value]),
         );
@@ -93,28 +101,13 @@
         try {
           await handleUpdateProjectVariables(flatVariables);
           open = false;
+          handleReset();
         } catch (error) {
           console.error(error);
         }
       },
     });
 
-  function getCurrentEnvironment() {
-    if (isDevelopment && isProduction) {
-      return undefined;
-    }
-
-    if (isDevelopment) {
-      return EnvironmentType.DEVELOPMENT;
-    }
-
-    if (isProduction) {
-      return EnvironmentType.PRODUCTION;
-    }
-
-    return undefined;
-  }
-
   async function handleUpdateProjectVariables(
     flatVariables: AdminServiceUpdateProjectVariablesBodyVariables,
   ) {
@@ -123,7 +116,7 @@
         organization,
         project,
         data: {
-          environment: getCurrentEnvironment(),
+          environment: getCurrentEnvironment(isDevelopment, isProduction),
           variables: flatVariables,
         },
       });
@@ -172,17 +165,29 @@
   }
 
   function checkForExistingKeys() {
-    const existingKeys = $form.variables.map((variable) => variable.key);
     inputErrors = {};
     isKeyAlreadyExists = false;
 
-    existingKeys.forEach((key, index) => {
-      // Case sensitive
-      if (variableNames.some((existingKey) => existingKey === key)) {
-        inputErrors[index] = true;
+    const existingKeys = $form.variables.map((variable) => {
+      return {
+        environment: getCurrentEnvironment(isDevelopment, isProduction),
+        name: variable.key,
+      };
+    });
+
+    let duplicateCount = 0;
+    existingKeys.forEach((key, idx) => {
+      const variableEnvironment = key.environment;
+      const variableKey = key.name;
+
+      if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) {
+        inputErrors[idx] = true;
         isKeyAlreadyExists = true;
+        duplicateCount++;
       }
     });
+
+    return duplicateCount;
   }
 
   function handleFileUpload(event) {
@@ -356,7 +361,9 @@
             {#if isKeyAlreadyExists}
               <div class="mt-1">
                 <p class="text-xs text-red-600 font-normal">
-                  These keys already exist for this project.
+                  {Object.keys(inputErrors).length > 1
+                    ? "These keys already exist for your target environment(s)"
+                    : "This key already exists for your target environment(s)"}
                 </p>
               </div>
             {/if}
diff --git a/web-admin/src/features/projects/environment-variables/EditDialog.svelte b/web-admin/src/features/projects/environment-variables/EditDialog.svelte
index 0774be048cd..f6c1f662021 100644
--- a/web-admin/src/features/projects/environment-variables/EditDialog.svelte
+++ b/web-admin/src/features/projects/environment-variables/EditDialog.svelte
@@ -21,16 +21,20 @@
   import { defaults, superForm } from "sveltekit-superforms";
   import { yup } from "sveltekit-superforms/adapters";
   import { object, string } from "yup";
-  import { EnvironmentType } from "./types";
+  import { EnvironmentType, type VariableNames } from "./types";
   import Input from "@rilldata/web-common/components/forms/Input.svelte";
-  import { onMount } from "svelte";
+  import {
+    getCurrentEnvironment,
+    getEnvironmentType,
+    isDuplicateKey,
+  } from "./utils";
 
   export let open = false;
   export let id: string;
   export let environment: string;
   export let name: string;
   export let value: string;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let initialEnvironment: {
     isDevelopment: boolean;
@@ -45,7 +49,7 @@
   $: project = $page.params.project;
 
   $: isEnvironmentSelected = isDevelopment || isProduction;
-  $: hasChanges =
+  $: hasNewChanges =
     $form.key !== initialValues.key ||
     $form.value !== initialValues.value ||
     initialEnvironment?.isDevelopment !== isDevelopment ||
@@ -67,8 +71,9 @@
       key: string()
         .optional()
         .matches(
-          /^[a-zA-Z0-9_]+$/,
-          "Key must only contain letters, numbers, and underscores",
+          /^[a-zA-Z_][a-zA-Z0-9_.]*$/,
+          // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12
+          "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots",
         ),
       value: string().optional(),
     }),
@@ -92,6 +97,9 @@
       if (!form.valid) return;
       const values = form.data;
 
+      checkForExistingKeys();
+      if (isKeyAlreadyExists) return;
+
       const flatVariable = {
         [values.key]: values.value,
       };
@@ -99,31 +107,17 @@
       try {
         await handleUpdateProjectVariables(flatVariable);
         open = false;
+        handleReset();
       } catch (error) {
         console.error(error);
       }
     },
   });
 
-  function processEnvironment() {
-    if (isDevelopment && isProduction) {
-      return undefined;
-    }
-
-    if (isDevelopment) {
-      return EnvironmentType.DEVELOPMENT;
-    }
-
-    if (isProduction) {
-      return EnvironmentType.PRODUCTION;
-    }
-
-    return undefined;
-  }
-
   async function handleUpdateProjectVariables(
     flatVariable: AdminServiceUpdateProjectVariablesBodyVariables,
   ) {
+    // Check if the key has changed, if so, check for existing keys
     if ($form.key !== initialValues.key) {
       checkForExistingKeys();
     }
@@ -146,7 +140,7 @@
           organization,
           project,
           data: {
-            environment: processEnvironment(),
+            environment: getCurrentEnvironment(isDevelopment, isProduction),
             variables: flatVariable,
           },
         });
@@ -155,7 +149,10 @@
       // If the key remains the same, update the environment or value
       if (initialValues.key === $form.key) {
         // If environment has changed, remove the old key and add the new key
-        if (initialValues.environment !== processEnvironment()) {
+        if (
+          initialValues.environment !==
+          getCurrentEnvironment(isDevelopment, isProduction)
+        ) {
           await $updateProjectVariables.mutateAsync({
             organization,
             project,
@@ -170,7 +167,7 @@
           organization,
           project,
           data: {
-            environment: processEnvironment(),
+            environment: getCurrentEnvironment(isDevelopment, isProduction),
             variables: flatVariable,
           },
         });
@@ -204,21 +201,30 @@
 
   function handleReset() {
     reset();
+    isDevelopment = false;
+    isProduction = false;
+    inputErrors = {};
     isKeyAlreadyExists = false;
   }
 
   function checkForExistingKeys() {
-    const existingKeys = [$form.key];
     inputErrors = {};
     isKeyAlreadyExists = false;
 
-    existingKeys.forEach((key, index) => {
-      // Case sensitive
-      if (variableNames.some((existingKey) => existingKey === key)) {
-        inputErrors[index] = true;
-        isKeyAlreadyExists = true;
-      }
-    });
+    const existingKey = {
+      environment: getEnvironmentType(
+        getCurrentEnvironment(isDevelopment, isProduction),
+      ),
+      name: $form.key,
+    };
+
+    const variableEnvironment = existingKey.environment;
+    const variableKey = existingKey.name;
+
+    if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) {
+      inputErrors[0] = true;
+      isKeyAlreadyExists = true;
+    }
   }
 
   function setInitialCheckboxState() {
@@ -236,13 +242,17 @@
     }
   }
 
-  onMount(() => {
+  function handleDialogOpen() {
     setInitialCheckboxState();
     initialEnvironment = {
       isDevelopment,
       isProduction,
     };
-  });
+  }
+
+  $: if (open) {
+    handleDialogOpen();
+  }
 </script>
 
 <Dialog
@@ -298,11 +308,6 @@
                   : ""}
                 placeholder="Key"
                 on:input={(e) => handleKeyChange(e)}
-                onBlur={() => {
-                  if ($form.key !== initialValues.key) {
-                    checkForExistingKeys();
-                  }
-                }}
               />
               <Input
                 bind:value={$form.value}
@@ -322,7 +327,7 @@
             {#if isKeyAlreadyExists}
               <div class="mt-1">
                 <p class="text-xs text-red-600 font-normal">
-                  This key already exists for this project.
+                  This key already exists for your target environment(s)
                 </p>
               </div>
             {/if}
@@ -343,7 +348,7 @@
         type="primary"
         form={$formId}
         disabled={$submitting ||
-          !hasChanges ||
+          !hasNewChanges ||
           !isEnvironmentSelected ||
           hasExistingKeys ||
           $allErrors.length > 0}
diff --git a/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte b/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte
index 2d6300dc235..e896e6d9222 100644
--- a/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte
+++ b/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte
@@ -8,10 +8,11 @@
   import KeyCell from "./KeyCell.svelte";
   import ValueCell from "./ValueCell.svelte";
   import ActionsCell from "./ActionsCell.svelte";
+  import type { VariableNames } from "./types";
 
   export let data: V1ProjectVariable[];
   export let emptyText: string = "No environment variables";
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   const columns: ColumnDef<V1ProjectVariable, any>[] = [
     {
diff --git a/web-admin/src/features/projects/environment-variables/KeyCell.svelte b/web-admin/src/features/projects/environment-variables/KeyCell.svelte
index 1a77da44de9..35280afbf29 100644
--- a/web-admin/src/features/projects/environment-variables/KeyCell.svelte
+++ b/web-admin/src/features/projects/environment-variables/KeyCell.svelte
@@ -5,8 +5,8 @@
   export let name: string;
 
   // NOTE: if environment is empty, the variable is shared for all environments
-  function getEnvironmentLabel(environment: string) {
-    if (environment === "") {
+  function getEnvironmentType(environment: string) {
+    if (environment === EnvironmentType.UNDEFINED) {
       return "Development, Production";
     }
     if (environment === EnvironmentType.DEVELOPMENT) {
@@ -18,7 +18,7 @@
     return "";
   }
 
-  $: environmentLabel = getEnvironmentLabel(environment);
+  $: environmentLabel = getEnvironmentType(environment);
 </script>
 
 <div class="flex flex-col">
diff --git a/web-admin/src/features/projects/environment-variables/types.ts b/web-admin/src/features/projects/environment-variables/types.ts
index b0f40cebe28..2fdaba1b836 100644
--- a/web-admin/src/features/projects/environment-variables/types.ts
+++ b/web-admin/src/features/projects/environment-variables/types.ts
@@ -7,7 +7,7 @@ export enum EnvironmentType {
 
 export type EnvironmentTypes = "dev" | "prod" | "";
 
-export type EnvironmentVariable = {
-  key: string;
-  value: string;
-};
+export type VariableNames = {
+  environment: string;
+  name: string;
+}[];
diff --git a/web-admin/src/features/projects/environment-variables/utils.ts b/web-admin/src/features/projects/environment-variables/utils.ts
new file mode 100644
index 00000000000..026a867c469
--- /dev/null
+++ b/web-admin/src/features/projects/environment-variables/utils.ts
@@ -0,0 +1,77 @@
+import { EnvironmentType, type VariableNames } from "./types";
+
+/**
+ *
+ * - Environment it belongs to ("prod" or "dev").
+ * - If empty, then the value is used as the fallback for all environments.
+ */
+export function getEnvironmentType(environment: string) {
+  return environment === EnvironmentType.UNDEFINED ? "" : environment;
+}
+
+/**
+ * Checks if a key would create a duplicate environment variable.
+ *
+ * Rules for environment variables:
+ * 1. A key can be tied to multiple environment configurations, e.g. my_key[dev] & my_key[prod]
+ * 2. If a key exists in all environments (UNDEFINED), it cannot be created in dev or prod
+ * 3. If a key exists in dev or prod, it cannot be created in all environments
+ * 4. If a key exists in dev, it cannot be created in dev but CAN be created in prod
+ *
+ * Examples:
+ * - If test[dev] exists:
+ *   - Cannot create test[dev] (same environment)
+ *   - Cannot create test[all] (key already used)
+ *   - CAN create test[prod] (different environment)
+ * - If test[all] exists:
+ *   - Cannot create test[dev] (key used in all environments)
+ *   - Cannot create test[prod] (key used in all environments)
+ *
+ * @param environment - The target environment ("development", "production", or "undefined" for all)
+ * @param key - The environment variable key to check
+ * @param variableNames - Existing environment variables
+ * @returns true if the key would create a duplicate, false otherwise
+ */
+export function isDuplicateKey(
+  environment: string,
+  key: string,
+  variableNames: VariableNames,
+): boolean {
+  return variableNames.some((variable) => {
+    // Only consider it a duplicate if the same key exists
+    if (variable.name === key) {
+      // If either the existing or new variable is for all environments, it's a duplicate
+      if (
+        variable.environment === EnvironmentType.UNDEFINED ||
+        environment === EnvironmentType.UNDEFINED
+      ) {
+        return true;
+      }
+
+      // If trying to create in the same environment as existing variable
+      if (variable.environment === environment) {
+        return true;
+      }
+    }
+    return false;
+  });
+}
+
+export function getCurrentEnvironment(
+  isDevelopment: boolean,
+  isProduction: boolean,
+) {
+  if (isDevelopment && isProduction) {
+    return EnvironmentType.UNDEFINED;
+  }
+
+  if (isDevelopment) {
+    return EnvironmentType.DEVELOPMENT;
+  }
+
+  if (isProduction) {
+    return EnvironmentType.PRODUCTION;
+  }
+
+  return EnvironmentType.UNDEFINED;
+}
diff --git a/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte b/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte
index 715a4816f93..f209f81687b 100644
--- a/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte
+++ b/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte
@@ -15,6 +15,7 @@
     EnvironmentType,
     type EnvironmentTypes,
   } from "@rilldata/web-admin/features/projects/environment-variables/types";
+  import { getEnvironmentType } from "@rilldata/web-admin/features/projects/environment-variables/utils";
 
   let open = false;
   let searchText = "";
@@ -34,7 +35,12 @@
 
   $: projectVariables = $getProjectVariables.data?.variables || [];
 
-  $: variableNames = projectVariables.map((variable) => variable.name);
+  $: variableNames = projectVariables.map((variable) => {
+    return {
+      environment: getEnvironmentType(variable.environment),
+      name: variable.name,
+    };
+  });
 
   $: searchedVariables = projectVariables.filter((variable) =>
     variable.name.toLowerCase().includes(searchText.toLowerCase()),