Select new sub-environments and keep enabled state through bulk edit (#714)

apps/yaak-client/commands/createEnvironment.tsx

@@ -6,11 +6,12 @@ import { showDialog } from "../lib/dialog";
 import { jotaiStore } from "../lib/jotai";
 import { setWorkspaceSearchParams } from "../lib/setWorkspaceSearchParams";
 
-export const createSubEnvironmentAndActivate = createFastMutation<
-  string | null,
-  unknown,
-  Environment | null
->({
+/**
+ * Prompt for and create a sub-environment of the given base environment, resolving with the new
+ * environment's ID (or null if the dialog was dismissed). This does not change the workspace's
+ * active environment.
+ */
+export const createSubEnvironment = createFastMutation<string | null, unknown, Environment | null>({
   mutationKey: ["create_environment"],
   mutationFn: async (baseEnvironment) => {
     if (baseEnvironment == null) {
@@ -41,6 +42,17 @@ export const createSubEnvironmentAndActivate = createFastMutation<
       });
     });
   },
+});
+
+/** Same as {@link createSubEnvironment}, but also makes the new environment the active one. */
+export const createSubEnvironmentAndActivate = createFastMutation<
+  string | null,
+  unknown,
+  Environment | null
+>({
+  mutationKey: ["create_environment_and_activate"],
+  mutationFn: (baseEnvironment) => createSubEnvironment.mutateAsync(baseEnvironment),
+  disableToastError: true, // Already shown by createSubEnvironment
   onSuccess: async (environmentId) => {
     if (environmentId == null) {
       return; // Was not created

apps/yaak-client/components/EnvironmentEditDialog.tsx

@@ -4,8 +4,16 @@ import type { TreeHandle, TreeNode, TreeProps } from "@yaakapp-internal/ui";
 import { Banner, Icon, InlineCode, SplitLayout, Tree } from "@yaakapp-internal/ui";
 import { atom, useAtomValue } from "jotai";
 import { atomFamily } from "jotai-family";
-import { useCallback, useLayoutEffect, useRef, useState } from "react";
-import { createSubEnvironmentAndActivate } from "../commands/createEnvironment";
+import {
+  createContext,
+  useCallback,
+  useContext,
+  useEffect,
+  useLayoutEffect,
+  useRef,
+  useState,
+} from "react";
+import { createSubEnvironment } from "../commands/createEnvironment";
 import { activeWorkspaceAtom, activeWorkspaceIdAtom } from "../hooks/useActiveWorkspace";
 import {
   environmentsBreakdownAtom,
@@ -40,6 +48,9 @@ interface Props {
 
 type TreeModel = Environment | Workspace;
 
+/** Lets tree row slots create a sub-environment and select it in the editor. */
+const CreateSubEnvironmentContext = createContext<() => Promise<void>>(async () => {});
+
 export function EnvironmentEditDialog({ initialEnvironmentId, setRef }: Props) {
   const { allEnvironments, baseEnvironment, baseEnvironments } = useEnvironmentsBreakdown();
   const [selectedEnvironmentId, setSelectedEnvironmentId] = useState<string | null>(
@@ -115,7 +126,7 @@ function EnvironmentEditDialogSidebar({
   const activeWorkspaceId = useAtomValue(activeWorkspaceIdAtom) ?? "";
   const treeId = `environment.${activeWorkspaceId}.sidebar`;
   const treeRef = useRef<TreeHandle>(null);
-  const { baseEnvironment, baseEnvironments } = useEnvironmentsBreakdown();
+  const { allEnvironments, baseEnvironment, baseEnvironments } = useEnvironmentsBreakdown();
 
   // oxlint-disable-next-line react-hooks/exhaustive-deps -- none
   useLayoutEffect(() => {
@@ -124,6 +135,31 @@ function EnvironmentEditDialogSidebar({
     treeRef.current?.focus();
   }, []);
 
+  // A just-created environment to select once it reaches the model store. Selecting it before
+  // then would show a "failed to find" error in the editor, and the tree would drop the
+  // selection of an item it doesn't have yet.
+  const [pendingSelectionId, setPendingSelectionId] = useState<string | null>(null);
+  const tree = useAtomValue(treeAtom);
+  useEffect(() => {
+    if (pendingSelectionId == null) return;
+    if (!allEnvironments.some((e) => e.id === pendingSelectionId)) return;
+
+    setPendingSelectionId(null);
+    setSelectedEnvironmentId(pendingSelectionId);
+
+    // The tree leaves out sub-environments while there are multiple base environments
+    const node = tree == null ? null : findTreeNode(tree, pendingSelectionId);
+    if (node == null) return;
+    if (node.parent != null) {
+      // Expand the parent so the new item is visible to be selected
+      const parentId = node.parent.item.id;
+      jotaiStore.set(collapsedFamily(treeId), (prev) => ({ ...prev, [parentId]: false }));
+    }
+    // Wait a frame so the tree's own pending selection fixups, which may have been scheduled
+    // before the new item arrived, don't reset this selection
+    requestAnimationFrame(() => treeRef.current?.selectItem(pendingSelectionId));
+  }, [allEnvironments, pendingSelectionId, setSelectedEnvironmentId, tree, treeId]);
+
   const handleDeleteEnvironment = useCallback(
     async (environment: Environment) => {
       await deleteModelWithConfirm(environment);
@@ -134,6 +170,14 @@ function EnvironmentEditDialogSidebar({
     [baseEnvironment?.id, selectedEnvironmentId, setSelectedEnvironmentId],
   );
 
+  const handleCreateSubEnvironment = useCallback(async () => {
+    const { baseEnvironment } = jotaiStore.get(environmentsBreakdownAtom);
+    if (baseEnvironment == null) return;
+    const id = await createSubEnvironment.mutateAsync(baseEnvironment);
+    // Show the new environment in the editor without changing the workspace's active environment
+    if (id != null) setPendingSelectionId(id);
+  }, []);
+
   const treeHasFocus = useCallback(() => treeRef.current?.hasFocus() ?? false, []);
 
   const getSelectedTreeModels = useCallback(
@@ -195,13 +239,14 @@ function EnvironmentEditDialogSidebar({
       const addEnvironmentItem: DropdownItem = {
         label: "Create Sub Environment",
         leftSlot: <Icon icon="plus" />,
-        onSelect: async () => {
-          await createSubEnvironment();
-        },
+        onSelect: handleCreateSubEnvironment,
       };
+      // Sub-environments aren't shown while there are multiple base environments, same as the
+      // plus button on the base environment row
+      const canCreateSubEnvironment = baseEnvironments.length <= 1;
 
       if (environment == null || environment.model !== "environment") {
-        return [addEnvironmentItem];
+        return canCreateSubEnvironment ? [addEnvironmentItem] : [];
       }
 
       const singleEnvironment = items.length === 1;
@@ -257,7 +302,7 @@ function EnvironmentEditDialogSidebar({
       ];
 
       // Add sub environment to base environment
-      if (isBaseEnvironment(environment) && singleEnvironment) {
+      if (isBaseEnvironment(environment) && singleEnvironment && canCreateSubEnvironment) {
         menuItems.push({ type: "separator" });
         menuItems.push(addEnvironmentItem);
       }
@@ -266,6 +311,7 @@ function EnvironmentEditDialogSidebar({
     },
     [
       baseEnvironments.length,
+      handleCreateSubEnvironment,
       handleDeleteEnvironment,
       handleDuplicateSelected,
       handleRenameSelected,
@@ -317,34 +363,37 @@ function EnvironmentEditDialogSidebar({
   );
 
   const renderContextMenuFn = useCallback<NonNullable<TreeProps<TreeModel>["renderContextMenu"]>>(
-    ({ items, position, onClose }) => (
-      <ContextMenu items={items as DropdownItem[]} triggerPosition={position} onClose={onClose} />
-    ),
+    ({ items, position, onClose }) =>
+      // Right-clicking empty space has nothing to offer while there are multiple base environments
+      items.length === 0 ? null : (
+        <ContextMenu items={items as DropdownItem[]} triggerPosition={position} onClose={onClose} />
+      ),
     [],
   );
 
-  const tree = useAtomValue(treeAtom);
   return (
     <aside className="x-theme-sidebar h-full w-full min-w-0 grid overflow-y-auto border-r border-border-subtle ">
       {tree != null && (
-        <div className="pt-2">
-          <Tree
-            ref={treeRef}
-            treeId={treeId}
-            collapsedAtom={collapsedFamily(treeId)}
-            className="px-2 pb-10"
-            root={tree}
-            getContextMenu={getContextMenu}
-            renderContextMenu={renderContextMenuFn}
-            onDragEnd={handleDragEnd}
-            getItemKey={(i) => `${i.id}::${i.name}`}
-            ItemLeftSlotInner={ItemLeftSlotInner}
-            ItemRightSlot={ItemRightSlot}
-            ItemInner={ItemInner}
-            onActivate={handleActivate}
-            getEditOptions={getEditOptions}
-          />
-        </div>
+        <CreateSubEnvironmentContext.Provider value={handleCreateSubEnvironment}>
+          <div className="pt-2">
+            <Tree
+              ref={treeRef}
+              treeId={treeId}
+              collapsedAtom={collapsedFamily(treeId)}
+              className="px-2 pb-10"
+              root={tree}
+              getContextMenu={getContextMenu}
+              renderContextMenu={renderContextMenuFn}
+              onDragEnd={handleDragEnd}
+              getItemKey={(i) => `${i.id}::${i.name}`}
+              ItemLeftSlotInner={ItemLeftSlotInner}
+              ItemRightSlot={ItemRightSlot}
+              ItemInner={ItemInner}
+              onActivate={handleActivate}
+              getEditOptions={getEditOptions}
+            />
+          </div>
+        </CreateSubEnvironmentContext.Provider>
       )}
     </aside>
   );
@@ -384,6 +433,15 @@ const treeAtom = atom<TreeNode<TreeModel> | null>((get) => {
   return root;
 });
 
+function findTreeNode(node: TreeNode<TreeModel>, id: string): TreeNode<TreeModel> | null {
+  if (node.item.id === id) return node;
+  for (const child of node.children ?? []) {
+    const found = findTreeNode(child, id);
+    if (found != null) return found;
+  }
+  return null;
+}
+
 function ItemLeftSlotInner({ item }: { item: TreeModel }) {
   const { baseEnvironments } = useEnvironmentsBreakdown();
   return baseEnvironments.length > 1 ? (
@@ -395,6 +453,7 @@ function ItemLeftSlotInner({ item }: { item: TreeModel }) {
 
 function ItemRightSlot({ item }: { item: TreeModel }) {
   const { baseEnvironments } = useEnvironmentsBreakdown();
+  const handleCreateSubEnvironment = useContext(CreateSubEnvironmentContext);
   return (
     <>
       {item.model === "environment" && baseEnvironments.length <= 1 && isBaseEnvironment(item) && (
@@ -405,7 +464,7 @@ function ItemRightSlot({ item }: { item: TreeModel }) {
           icon="plus_circle"
           className="opacity-50 hover:opacity-100"
           title="Add Sub-Environment"
-          onClick={createSubEnvironment}
+          onClick={handleCreateSubEnvironment}
         />
       )}
     </>
@@ -425,13 +484,6 @@ function ItemInner({ item }: { item: TreeModel }) {
   );
 }
 
-async function createSubEnvironment() {
-  const { baseEnvironment } = jotaiStore.get(environmentsBreakdownAtom);
-  if (baseEnvironment == null) return;
-  const id = await createSubEnvironmentAndActivate.mutateAsync(baseEnvironment);
-  return id;
-}
-
 function getEditOptions(item: TreeModel) {
   const options: ReturnType<NonNullable<TreeProps<TreeModel>["getEditOptions"]>> = {
     defaultValue: item.name,

apps/yaak-client/components/core/BulkPairEditor.test.ts

@@ -1,5 +1,10 @@
 import { describe, expect, test } from "vite-plus/test";
-import { parseBulkPairLine } from "./BulkPairEditor";
+import {
+  formatBulkPairLine,
+  formatBulkPairs,
+  parseBulkPairLine,
+  parseBulkPairs,
+} from "./BulkPairEditor";
 
 describe("parseBulkPairLine", () => {
   test("parses colon-space pairs as name and value", () => {
@@ -33,4 +38,130 @@ describe("parseBulkPairLine", () => {
       value: "bar\nbaz",
     });
   });
+
+  test("parses commented-out pairs as disabled", () => {
+    expect(parseBulkPairLine("# foo: bar")).toMatchObject({
+      enabled: false,
+      name: "foo",
+      value: "bar",
+    });
+  });
+
+  test("allows any whitespace after the comment marker", () => {
+    expect(parseBulkPairLine("#   foo: bar")).toMatchObject({
+      enabled: false,
+      name: "foo",
+      value: "bar",
+    });
+    expect(parseBulkPairLine("#\tfoo: bar")).toMatchObject({
+      enabled: false,
+      name: "foo",
+      value: "bar",
+    });
+  });
+
+  test("treats a # without whitespace after it as part of the name", () => {
+    expect(parseBulkPairLine("#foo: bar")).toMatchObject({
+      enabled: true,
+      name: "#foo",
+      value: "bar",
+    });
+  });
+
+  test("parses commented-out pairs with an empty value as disabled", () => {
+    expect(parseBulkPairLine("# token:")).toMatchObject({
+      enabled: false,
+      name: "token",
+      value: "",
+    });
+    expect(parseBulkPairLine("# token: ")).toMatchObject({
+      enabled: false,
+      name: "token",
+      value: "",
+    });
+  });
+
+  test("drops comments that are not pairs", () => {
+    expect(parseBulkPairLine("# just a comment")).toBeNull();
+    expect(parseBulkPairLine("# see http://example.com")).toBeNull();
+    expect(parseBulkPairLine("#")).toBeNull();
+  });
+
+  test("leaves values that start with # alone", () => {
+    expect(parseBulkPairLine("color: #fff")).toMatchObject({
+      enabled: true,
+      name: "color",
+      value: "#fff",
+    });
+  });
+});
+
+describe("formatBulkPairLine", () => {
+  test("formats enabled pairs as name and value", () => {
+    expect(formatBulkPairLine({ enabled: true, name: "foo", value: "bar" })).toBe("foo: bar");
+  });
+
+  test("treats pairs without an enabled flag as enabled", () => {
+    expect(formatBulkPairLine({ name: "foo", value: "bar" })).toBe("foo: bar");
+  });
+
+  test("comments out disabled pairs", () => {
+    expect(formatBulkPairLine({ enabled: false, name: "foo", value: "bar" })).toBe("# foo: bar");
+  });
+
+  test("escapes newlines in values", () => {
+    expect(formatBulkPairLine({ enabled: false, name: "foo", value: "a\nb" })).toBe("# foo: a\\nb");
+  });
+});
+
+describe("bulk pair round trip", () => {
+  test("preserves order and enabled state", () => {
+    const pairs = [
+      { enabled: true, name: "host", value: "example.com" },
+      { enabled: false, name: "token", value: "old" },
+      { enabled: true, name: "color", value: "#fff" },
+      { enabled: false, name: "multi", value: "a\nb" },
+      { enabled: true, name: "token", value: "new" },
+    ];
+
+    const text = formatBulkPairs(pairs);
+    expect(text).toBe(
+      ["host: example.com", "# token: old", "color: #fff", "# multi: a\\nb", "token: new"].join(
+        "\n",
+      ),
+    );
+
+    const parsed = parseBulkPairs(text);
+    expect(parsed.map(({ enabled, name, value }) => ({ enabled, name, value }))).toEqual(pairs);
+  });
+
+  test("preserves enabled pairs whose name starts with #", () => {
+    const pairs = [{ enabled: true, name: "#foo", value: "bar" }];
+    const text = formatBulkPairs(pairs);
+    expect(text).toBe("#foo: bar");
+    const parsed = parseBulkPairs(text);
+    expect(parsed.map(({ enabled, name, value }) => ({ enabled, name, value }))).toEqual(pairs);
+  });
+
+  test("preserves disabled pairs whose name starts with #", () => {
+    const pairs = [{ enabled: false, name: "#foo", value: "bar" }];
+    const text = formatBulkPairs(pairs);
+    expect(text).toBe("# #foo: bar");
+    const parsed = parseBulkPairs(text);
+    expect(parsed.map(({ enabled, name, value }) => ({ enabled, name, value }))).toEqual(pairs);
+  });
+
+  test("preserves disabled pairs with empty values", () => {
+    const pairs = [{ enabled: false, name: "token", value: "" }];
+    const parsed = parseBulkPairs(formatBulkPairs(pairs));
+    expect(parsed.map(({ enabled, name, value }) => ({ enabled, name, value }))).toEqual(pairs);
+  });
+
+  test("drops free-text comments and blank lines between pairs", () => {
+    const parsed = parseBulkPairs("# Auth\nfoo: bar\n\n# baz: qux\n# see the docs");
+    expect(parsed.map(({ enabled, name, value }) => ({ enabled, name, value }))).toEqual([
+      { enabled: true, name: "foo", value: "bar" },
+      { enabled: false, name: "baz", value: "qux" },
+    ]);
+  });
 });

apps/yaak-client/components/core/BulkPairEditor.tsx

@@ -14,20 +14,11 @@ export function BulkPairEditor({
   forcedEnvironmentId,
   stateKey,
 }: Props) {
-  const pairsText = useMemo(() => {
-    return pairs
-      .filter((p) => !(p.name.trim() === "" && p.value.trim() === ""))
-      .map(formatBulkPairLine)
-      .join("\n");
-  }, [pairs]);
+  const pairsText = useMemo(() => formatBulkPairs(pairs), [pairs]);
 
   const handleChange = useCallback(
     (text: string) => {
-      const pairs = text
-        .split("\n")
-        .filter((l: string) => l.trim())
-        .map(parseBulkPairLine);
-      onChange(pairs);
+      onChange(parseBulkPairs(text));
     },
     [onChange],
   );
@@ -47,13 +38,57 @@ export function BulkPairEditor({
   );
 }
 
-export function formatBulkPairLine(pair: Pair) {
-  const value = pair.value.replaceAll("\n", "\\n");
-  return `${pair.name}: ${value}`;
+export function formatBulkPairs(pairs: Pair[]): string {
+  return pairs
+    .filter((p) => !(p.name.trim() === "" && p.value.trim() === ""))
+    .map(formatBulkPairLine)
+    .join("\n");
 }
 
-export function parseBulkPairLine(line: string): PairWithId {
-  const [, name, value] = line.match(/^([^:]+):\s+(.*)$/) ?? [];
+export function parseBulkPairs(text: string): PairWithId[] {
+  return text
+    .split("\n")
+    .filter((l: string) => l.trim())
+    .map(parseBulkPairLine)
+    .filter((p) => p != null);
+}
+
+/**
+ * Format a pair as a `name: value` line. Disabled pairs are commented out dotenv-style, as
+ * `# name: value`, so the enabled state survives a round trip through {@link parseBulkPairLine}.
+ */
+export function formatBulkPairLine(pair: Pair) {
+  const value = pair.value.replaceAll("\n", "\\n");
+  const line = `${pair.name}: ${value}`;
+  return pair.enabled === false ? `# ${line}` : line;
+}
+
+const PAIR_REGEX = /^([^:]+):\s+(.*)$/;
+// Commented-out pairs may also end at the colon, so a hand-typed `# name:` isn't dropped
+const COMMENTED_PAIR_REGEX = /^([^:]+):(?:\s+(.*))?$/;
+// A # only marks a comment when followed by whitespace (or nothing), so `#foo: bar` stays an
+// enabled pair named `#foo`
+const COMMENT_PREFIX_REGEX = /^\s*#(?:\s+|$)/;
+
+/**
+ * Parse a `name: value` line into an enabled pair. A line starting with `# ` is a disabled pair
+ * if the rest of it parses as `name: value`, otherwise it's a free-text comment and `null` is
+ * returned so it can be dropped.
+ */
+export function parseBulkPairLine(line: string): PairWithId | null {
+  const commentPrefix = line.match(COMMENT_PREFIX_REGEX);
+  if (commentPrefix != null) {
+    const uncommented = line.slice(commentPrefix[0].length);
+    const match = uncommented.match(COMMENTED_PAIR_REGEX);
+    if (match == null) return null;
+    return { ...pairFromMatch(uncommented, match), enabled: false };
+  }
+
+  return pairFromMatch(line, line.match(PAIR_REGEX));
+}
+
+function pairFromMatch(line: string, match: RegExpMatchArray | null): PairWithId {
+  const [, name, value] = match ?? [];
   return {
     enabled: true,
     name: (name ?? line).trim(),