Better scenario variable editing

Some users have gotten confused by the scenario variable editing interface. This change makes the interface easier to understand.

app/src/server/api/root.router.ts

@@ -3,7 +3,7 @@ import { createTRPCRouter } from "~/server/api/trpc";
 import { experimentsRouter } from "./routers/experiments.router";
 import { scenariosRouter } from "./routers/scenarios.router";
 import { scenarioVariantCellsRouter } from "./routers/scenarioVariantCells.router";
-import { templateVarsRouter } from "./routers/templateVariables.router";
+import { scenarioVarsRouter } from "./routers/scenarioVariables.router";
 import { evaluationsRouter } from "./routers/evaluations.router";
 import { worldChampsRouter } from "./routers/worldChamps.router";
 import { datasetsRouter } from "./routers/datasets.router";
@@ -22,7 +22,7 @@ export const appRouter = createTRPCRouter({
   experiments: experimentsRouter,
   scenarios: scenariosRouter,
   scenarioVariantCells: scenarioVariantCellsRouter,
-  templateVars: templateVarsRouter,
+  scenarioVars: scenarioVarsRouter,
   evaluations: evaluationsRouter,
   worldChamps: worldChampsRouter,
   datasets: datasetsRouter,

app/src/server/api/routers/promptVariants.router.ts

@@ -3,7 +3,7 @@ import { createTRPCRouter, protectedProcedure, publicProcedure } from "~/server/
 import { prisma } from "~/server/db";
 import { Prisma } from "@prisma/client";
 import { generateNewCell } from "~/server/utils/generateNewCell";
-import userError from "~/server/utils/error";
+import { error, success } from "~/utils/standardResponses";
 import { recordExperimentUpdated } from "~/server/utils/recordExperimentUpdated";
 import { reorderPromptVariants } from "~/server/utils/reorderPromptVariants";
 import { type PromptVariant } from "@prisma/client";
@@ -315,7 +315,7 @@ export const promptVariantsRouter = createTRPCRouter({
       const constructedPrompt = await parsePromptConstructor(existing.promptConstructor);
 
       if ("error" in constructedPrompt) {
-        return userError(constructedPrompt.error);
+        return error(constructedPrompt.error);
       }
 
       const model = input.newModel
@@ -353,7 +353,7 @@ export const promptVariantsRouter = createTRPCRouter({
       const parsedPrompt = await parsePromptConstructor(input.promptConstructor);
 
       if ("error" in parsedPrompt) {
-        return userError(parsedPrompt.error);
+        return error(parsedPrompt.error);
       }
 
       // Create a duplicate with only the config changed
@@ -398,7 +398,7 @@ export const promptVariantsRouter = createTRPCRouter({
         });
       }
 
-      return { status: "ok" } as const;
+      return success();
     }),
 
   reorder: protectedProcedure

app/src/server/api/routers/scenarioVariables.router.ts

@@ -0,0 +1,143 @@
+import { type TemplateVariable } from "@prisma/client";
+import { sql } from "kysely";
+import { z } from "zod";
+import { createTRPCRouter, protectedProcedure, publicProcedure } from "~/server/api/trpc";
+import { kysely, prisma } from "~/server/db";
+import { error, success } from "~/utils/standardResponses";
+import { requireCanModifyExperiment, requireCanViewExperiment } from "~/utils/accessControl";
+
+export const scenarioVarsRouter = createTRPCRouter({
+  create: protectedProcedure
+    .input(z.object({ experimentId: z.string(), label: z.string() }))
+    .mutation(async ({ input, ctx }) => {
+      await requireCanModifyExperiment(input.experimentId, ctx);
+
+      // Make sure there isn't an existing variable with the same name
+      const existingVariable = await prisma.templateVariable.findFirst({
+        where: {
+          experimentId: input.experimentId,
+          label: input.label,
+        },
+      });
+      if (existingVariable) {
+        return error(`A variable named ${input.label} already exists.`);
+      }
+
+      await prisma.templateVariable.create({
+        data: {
+          experimentId: input.experimentId,
+          label: input.label,
+        },
+      });
+
+      return success();
+    }),
+
+  rename: protectedProcedure
+    .input(z.object({ id: z.string(), label: z.string() }))
+    .mutation(async ({ input, ctx }) => {
+      const templateVariable = await prisma.templateVariable.findUniqueOrThrow({
+        where: { id: input.id },
+      });
+      await requireCanModifyExperiment(templateVariable.experimentId, ctx);
+
+      // Make sure there isn't an existing variable with the same name
+      const existingVariable = await prisma.templateVariable.findFirst({
+        where: {
+          experimentId: templateVariable.experimentId,
+          label: input.label,
+        },
+      });
+      if (existingVariable) {
+        return error(`A variable named ${input.label} already exists.`);
+      }
+
+      await renameTemplateVariable(templateVariable, input.label);
+      return success();
+    }),
+
+  delete: protectedProcedure
+    .input(z.object({ id: z.string() }))
+    .mutation(async ({ input, ctx }) => {
+      const { experimentId } = await prisma.templateVariable.findUniqueOrThrow({
+        where: { id: input.id },
+      });
+
+      await requireCanModifyExperiment(experimentId, ctx);
+
+      await prisma.templateVariable.delete({ where: { id: input.id } });
+    }),
+
+  list: publicProcedure
+    .input(z.object({ experimentId: z.string() }))
+    .query(async ({ input, ctx }) => {
+      await requireCanViewExperiment(input.experimentId, ctx);
+      return await prisma.templateVariable.findMany({
+        where: {
+          experimentId: input.experimentId,
+        },
+        orderBy: {
+          createdAt: "asc",
+        },
+        select: {
+          id: true,
+          label: true,
+        },
+      });
+    }),
+});
+
+export const renameTemplateVariable = async (
+  templateVariable: TemplateVariable,
+  newLabel: string,
+) => {
+  const { experimentId } = templateVariable;
+
+  await kysely.transaction().execute(async (trx) => {
+    await trx
+      .updateTable("TemplateVariable")
+      .set({
+        label: newLabel,
+      })
+      .where("id", "=", templateVariable.id)
+      .execute();
+
+    await sql`
+      CREATE TEMP TABLE "TempTestScenario" AS
+      SELECT *
+      FROM "TestScenario"
+      WHERE "experimentId" = ${experimentId}
+
+      -- Only copy the rows that actually have a value for the variable, no reason to churn the rest and simplifies the update.
+      AND "variableValues"->${templateVariable.label} IS NOT NULL
+      `.execute(trx);
+
+    await sql`
+      UPDATE "TempTestScenario"
+      SET "variableValues" = jsonb_set(
+        "variableValues",
+        ${`{${newLabel}}`},
+        "variableValues"->${templateVariable.label}
+      ) - ${templateVariable.label},
+      "updatedAt" = NOW(),
+      "id" = uuid_generate_v4()
+    `.execute(trx);
+
+    // Print the contents of the temp table
+    const results = await sql`SELECT * FROM "TempTestScenario"`.execute(trx);
+    console.log(results.rows);
+
+    await trx
+      .updateTable("TestScenario")
+      .set({
+        visible: false,
+      })
+      .where("experimentId", "=", experimentId)
+      .execute();
+
+    await sql`
+      INSERT INTO "TestScenario" (id, "variableValues", "uiId", visible, "sortIndex", "experimentId", "createdAt", "updatedAt")
+      SELECT * FROM "TempTestScenario";
+    `.execute(trx);
+  });
+};

app/src/server/api/routers/templateVariables.router.test.ts

@@ -0,0 +1,110 @@
+import { expect, it } from "vitest";
+import { prisma } from "~/server/db";
+import { renameTemplateVariable } from "./scenarioVariables.router";
+
+const createExperiment = async () => {
+  return await prisma.experiment.create({
+    data: {
+      label: "Test Experiment",
+      project: {
+        create: {},
+      },
+    },
+  });
+};
+
+const createTemplateVar = async (experimentId: string, label: string) => {
+  return await prisma.templateVariable.create({
+    data: {
+      experimentId,
+      label,
+    },
+  });
+};
+
+it("renames templateVariables", async () => {
+  // Create experiments concurrently
+  const [exp1, exp2] = await Promise.all([createExperiment(), createExperiment()]);
+
+  // Create template variables concurrently
+  const [exp1Var, exp2Var1, exp2Var2] = await Promise.all([
+    createTemplateVar(exp1.id, "input1"),
+    createTemplateVar(exp2.id, "input1"),
+    createTemplateVar(exp2.id, "input2"),
+  ]);
+
+  // Create test scenarios concurrently
+  const [exp1Scenario, exp2Scenario, exp2HiddenScenario] = await Promise.all([
+    prisma.testScenario.create({
+      data: {
+        experimentId: exp1.id,
+        visible: true,
+        variableValues: { input1: "test" },
+      },
+    }),
+    prisma.testScenario.create({
+      data: {
+        experimentId: exp2.id,
+        visible: true,
+        variableValues: { input1: "test1", otherInput: "otherTest" },
+      },
+    }),
+    prisma.testScenario.create({
+      data: {
+        experimentId: exp2.id,
+        visible: false,
+        variableValues: { otherInput: "otherTest2" },
+      },
+    }),
+  ]);
+
+  await renameTemplateVariable(exp2Var1, "input1-renamed");
+
+  expect(await prisma.templateVariable.findUnique({ where: { id: exp2Var1.id } })).toMatchObject({
+    label: "input1-renamed",
+  });
+
+  // It shouldn't mess with unrelated experiments
+  expect(await prisma.testScenario.findUnique({ where: { id: exp1Scenario.id } })).toMatchObject({
+    visible: true,
+    variableValues: { input1: "test" },
+  });
+
+  // Make sure there are a total of 4 scenarios for exp2
+  expect(
+    await prisma.testScenario.count({
+      where: {
+        experimentId: exp2.id,
+      },
+    }),
+  ).toBe(3);
+
+  // It shouldn't mess with the existing scenarios, except to hide them
+  expect(await prisma.testScenario.findUnique({ where: { id: exp2Scenario.id } })).toMatchObject({
+    visible: false,
+    variableValues: { input1: "test1", otherInput: "otherTest" },
+  });
+
+  // It should create a new scenario with the new variable name
+  const newScenario1 = await prisma.testScenario.findFirst({
+    where: {
+      experimentId: exp2.id,
+      variableValues: { equals: { "input1-renamed": "test1", otherInput: "otherTest" } },
+    },
+  });
+
+  expect(newScenario1).toMatchObject({
+    visible: true,
+  });
+
+  const newScenario2 = await prisma.testScenario.findFirst({
+    where: {
+      experimentId: exp2.id,
+      variableValues: { equals: { otherInput: "otherTest2" } },
+    },
+  });
+
+  expect(newScenario2).toMatchObject({
+    visible: false,
+  });
+});

app/src/server/api/routers/templateVariables.router.ts

@@ -1,49 +0,0 @@
-import { z } from "zod";
-import { createTRPCRouter, protectedProcedure, publicProcedure } from "~/server/api/trpc";
-import { prisma } from "~/server/db";
-import { requireCanModifyExperiment, requireCanViewExperiment } from "~/utils/accessControl";
-
-export const templateVarsRouter = createTRPCRouter({
-  create: protectedProcedure
-    .input(z.object({ experimentId: z.string(), label: z.string() }))
-    .mutation(async ({ input, ctx }) => {
-      await requireCanModifyExperiment(input.experimentId, ctx);
-
-      await prisma.templateVariable.create({
-        data: {
-          experimentId: input.experimentId,
-          label: input.label,
-        },
-      });
-    }),
-
-  delete: protectedProcedure
-    .input(z.object({ id: z.string() }))
-    .mutation(async ({ input, ctx }) => {
-      const { experimentId } = await prisma.templateVariable.findUniqueOrThrow({
-        where: { id: input.id },
-      });
-
-      await requireCanModifyExperiment(experimentId, ctx);
-
-      await prisma.templateVariable.delete({ where: { id: input.id } });
-    }),
-
-  list: publicProcedure
-    .input(z.object({ experimentId: z.string() }))
-    .query(async ({ input, ctx }) => {
-      await requireCanViewExperiment(input.experimentId, ctx);
-      return await prisma.templateVariable.findMany({
-        where: {
-          experimentId: input.experimentId,
-        },
-        orderBy: {
-          createdAt: "asc",
-        },
-        select: {
-          id: true,
-          label: true,
-        },
-      });
-    }),
-});