fix: infinite loop and freeze (#5622)

Co-authored-by: Jakob Schott <jakob@formbricks.com> Co-authored-by: Jakob Schott <154420406+jakobsitory@users.noreply.github.com> Co-authored-by: pandeymangg <anshuman.pandey9999@gmail.com>
2025-12-30 10:19:51 -06:00 · 2025-05-07 07:19:26 -07:00
parent f0be6de0b3
commit faabd371f5
3 changed files with 253 additions and 49 deletions
--- a/apps/web/modules/survey/editor/components/survey-variables-card-item.test.tsx
+++ b/apps/web/modules/survey/editor/components/survey-variables-card-item.test.tsx
@@ -1,14 +1,37 @@
+import * as utils from "@/modules/survey/editor/lib/utils";
 import { cleanup, render, screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import React from "react";
 import { FormProvider, useForm } from "react-hook-form";
+import toast from "react-hot-toast";
 import { afterEach, describe, expect, test, vi } from "vitest";
 import { TSurvey, TSurveyVariable } from "@formbricks/types/surveys/types";
 import { SurveyVariablesCardItem } from "./survey-variables-card-item";

+vi.mock("@/modules/survey/editor/lib/utils", () => {
+  return {
+    findVariableUsedInLogic: vi.fn(),
+    getVariableTypeFromValue: vi.fn().mockImplementation((value) => {
+      if (typeof value === "number") return "number";
+      if (typeof value === "boolean") return "boolean";
+      return "text";
+    }),
+    translateOptions: vi.fn().mockReturnValue([]),
+    validateLogic: vi.fn(),
+  };
+});
+
+vi.mock("react-hot-toast", () => ({
+  default: {
+    error: vi.fn(),
+    success: vi.fn(),
+  },
+}));
+
 describe("SurveyVariablesCardItem", () => {
  afterEach(() => {
    cleanup();
+    vi.resetAllMocks();
  });

  const TestWrapper: React.FC<{ children: React.ReactNode }> = ({ children }) => {
@@ -88,6 +111,62 @@ describe("SurveyVariablesCardItem", () => {
    );
  });

+  test("should not create a new survey variable when mode is 'create' and the form input is invalid", async () => {
+    const mockSetLocalSurvey = vi.fn();
+    const initialSurvey = {
+      id: "survey123",
+      createdAt: new Date(),
+      updatedAt: new Date(),
+      name: "Test Survey",
+      status: "draft",
+      environmentId: "env123",
+      type: "app",
+      welcomeCard: {
+        enabled: true,
+        timeToFinish: false,
+        headline: { default: "Welcome" },
+        buttonLabel: { default: "Start" },
+        showResponseCount: false,
+      },
+      autoClose: null,
+      closeOnDate: null,
+      delay: 0,
+      displayOption: "displayOnce",
+      recontactDays: null,
+      displayLimit: null,
+      runOnDate: null,
+      questions: [],
+      endings: [],
+      hiddenFields: {
+        enabled: true,
+        fieldIds: ["field1", "field2"],
+      },
+      variables: [],
+    } as unknown as TSurvey;
+
+    render(
+      <TestWrapper>
+        <SurveyVariablesCardItem
+          mode="create"
+          localSurvey={initialSurvey}
+          setLocalSurvey={mockSetLocalSurvey}
+        />
+      </TestWrapper>
+    );
+
+    const nameInput = screen.getByPlaceholderText("environments.surveys.edit.field_name_eg_score_price");
+    const valueInput = screen.getByPlaceholderText("environments.surveys.edit.initial_value");
+    const addButton = screen.getByRole("button", { name: "environments.surveys.edit.add_variable" });
+
+    await userEvent.type(nameInput, "1invalidvariablename");
+    await userEvent.type(valueInput, "10");
+    await userEvent.click(addButton);
+
+    const errorMessage = screen.getByText("environments.surveys.edit.variable_name_must_start_with_a_letter");
+    expect(errorMessage).toBeVisible();
+    expect(mockSetLocalSurvey).not.toHaveBeenCalled();
+  });
+
  test("should display an error message when the variable name is invalid", async () => {
    const mockSetLocalSurvey = vi.fn();
    const initialSurvey = {
@@ -244,4 +323,142 @@ describe("SurveyVariablesCardItem", () => {
      screen.getByText("environments.surveys.edit.variable_name_is_already_taken_please_choose_another")
    ).toBeVisible();
  });
+
+  test("should show error toast if trying to delete a variable used in logic and not call setLocalSurvey", async () => {
+    const variableUsedInLogic = {
+      id: "logicVarId",
+      name: "logic_variable",
+      type: "text",
+      value: "test_value",
+    } as TSurveyVariable;
+
+    const mockSetLocalSurvey = vi.fn();
+
+    // Mock findVariableUsedInLogic to return 2, indicating the variable is used in logic
+    const findVariableUsedInLogicMock = vi.fn().mockReturnValue(2);
+    vi.spyOn(utils, "findVariableUsedInLogic").mockImplementation(findVariableUsedInLogicMock);
+
+    const initialSurvey = {
+      id: "survey123",
+      createdAt: new Date(),
+      updatedAt: new Date(),
+      name: "Test Survey",
+      status: "draft",
+      environmentId: "env123",
+      type: "app",
+      welcomeCard: {
+        enabled: true,
+        timeToFinish: false,
+        headline: { default: "Welcome" },
+        buttonLabel: { default: "Start" },
+        showResponseCount: false,
+      },
+      autoClose: null,
+      closeOnDate: null,
+      delay: 0,
+      displayOption: "displayOnce",
+      recontactDays: null,
+      displayLimit: null,
+      runOnDate: null,
+      questions: [
+        {
+          id: "q1WithLogic",
+          type: "openText",
+          headline: { default: "Question with logic" },
+          required: false,
+          logic: [{ condition: "equals", value: "logicVarId", destination: "q2" }],
+        },
+        { id: "q2", type: "openText", headline: { default: "Q2" }, required: false },
+      ],
+      endings: [],
+      hiddenFields: {
+        enabled: true,
+        fieldIds: ["field1", "field2"],
+      },
+      variables: [variableUsedInLogic],
+    } as unknown as TSurvey;
+
+    render(
+      <SurveyVariablesCardItem
+        mode="edit"
+        localSurvey={initialSurvey}
+        setLocalSurvey={mockSetLocalSurvey}
+        variable={variableUsedInLogic}
+      />
+    );
+
+    const deleteButton = screen.getByRole("button");
+    await userEvent.click(deleteButton);
+
+    expect(utils.findVariableUsedInLogic).toHaveBeenCalledWith(initialSurvey, variableUsedInLogic.id);
+    expect(mockSetLocalSurvey).not.toHaveBeenCalled();
+  });
+
+  test("should delete variable when it's not used in logic", async () => {
+    const variableToDelete = {
+      id: "recallVarId",
+      name: "recall_variable",
+      type: "text",
+      value: "recall_value",
+    } as TSurveyVariable;
+
+    const mockSetLocalSurvey = vi.fn();
+
+    const findVariableUsedInLogicMock = vi.fn().mockReturnValue(-1);
+    vi.spyOn(utils, "findVariableUsedInLogic").mockImplementation(findVariableUsedInLogicMock);
+
+    const initialSurvey = {
+      id: "survey123",
+      createdAt: new Date(),
+      updatedAt: new Date(),
+      name: "Test Survey",
+      status: "draft",
+      environmentId: "env123",
+      type: "app",
+      welcomeCard: {
+        enabled: true,
+        timeToFinish: false,
+        headline: { default: "Welcome" },
+        buttonLabel: { default: "Start" },
+        showResponseCount: false,
+      },
+      autoClose: null,
+      closeOnDate: null,
+      delay: 0,
+      displayOption: "displayOnce",
+      recontactDays: null,
+      displayLimit: null,
+      runOnDate: null,
+      questions: [
+        {
+          id: "q1",
+          type: "openText",
+          headline: { default: "Question with recall:recallVarId in it" },
+          required: false,
+        },
+      ],
+      endings: [],
+      hiddenFields: {
+        enabled: true,
+        fieldIds: ["field1", "field2"],
+      },
+      variables: [variableToDelete],
+    } as unknown as TSurvey;
+
+    render(
+      <SurveyVariablesCardItem
+        mode="edit"
+        localSurvey={initialSurvey}
+        setLocalSurvey={mockSetLocalSurvey}
+        variable={variableToDelete}
+      />
+    );
+
+    const deleteButton = screen.getByRole("button");
+    await userEvent.click(deleteButton);
+
+    expect(utils.findVariableUsedInLogic).toHaveBeenCalledWith(initialSurvey, variableToDelete.id);
+    expect(mockSetLocalSurvey).toHaveBeenCalledTimes(1);
+    expect(mockSetLocalSurvey).toHaveBeenCalledWith(expect.any(Function));
+  });
 });
--- a/apps/web/modules/survey/editor/components/survey-variables-card-item.tsx
+++ b/apps/web/modules/survey/editor/components/survey-variables-card-item.tsx
@@ -16,7 +16,7 @@ import {
 import { createId } from "@paralleldrive/cuid2";
 import { useTranslate } from "@tolgee/react";
 import { TrashIcon } from "lucide-react";
-import React, { useCallback, useEffect } from "react";
+import React, { useCallback } from "react";
 import { useForm } from "react-hook-form";
 import toast from "react-hot-toast";
 import { TSurvey, TSurveyVariable } from "@formbricks/types/surveys/types";
@@ -64,7 +64,6 @@ export const SurveyVariablesCardItem = ({
      ...localSurvey,
      variables: [...localSurvey.variables, data],
    });
-
    form.reset({
      id: createId(),
      name: "",
@@ -73,26 +72,18 @@ export const SurveyVariablesCardItem = ({
    });
  };

-  useEffect(() => {
-    if (mode === "create") {
-      return;
-    }
+  // Removed auto-submit effect

-    const subscription = form.watch(() => form.handleSubmit(editSurveyVariable)());
-    return () => subscription.unsubscribe();
-  }, [form, mode, editSurveyVariable]);
-
-  const onVariableDelete = (variable: TSurveyVariable) => {
+  const onVariableDelete = (variableToDelete: TSurveyVariable) => {
    const questions = [...localSurvey.questions];
-
-    const quesIdx = findVariableUsedInLogic(localSurvey, variable.id);
+    const quesIdx = findVariableUsedInLogic(localSurvey, variableToDelete.id);

    if (quesIdx !== -1) {
      toast.error(
        t(
          "environments.surveys.edit.variable_is_used_in_logic_of_question_please_remove_it_from_logic_first",
          {
-            variable: variable.name,
+            variable: variableToDelete.name,
            questionIndex: quesIdx + 1,
          }
        )
@@ -100,10 +91,10 @@ export const SurveyVariablesCardItem = ({
      return;
    }

-    // find if this variable is used in any question's recall and remove it for every language
+    // remove recall references
    questions.forEach((question) => {
      for (const [languageCode, headline] of Object.entries(question.headline)) {
-        if (headline.includes(`recall:${variable.id}`)) {
+        if (headline.includes(`recall:${variableToDelete.id}`)) {
          const recallInfo = extractRecallInfo(headline);
          if (recallInfo) {
            question.headline[languageCode] = headline.replace(recallInfo, "");
@@ -113,7 +104,7 @@ export const SurveyVariablesCardItem = ({
    });

    setLocalSurvey((prevSurvey) => {
-      const updatedVariables = prevSurvey.variables.filter((v) => v.id !== variable.id);
+      const updatedVariables = prevSurvey.variables.filter((v) => v.id !== variableToDelete.id);
      return { ...prevSurvey, variables: updatedVariables, questions };
    });
  };
@@ -139,6 +130,7 @@ export const SurveyVariablesCardItem = ({
          )}

          <div className="mt-2 flex w-full items-center gap-2">
+            {/* Name field: update on blur */}
            <FormField
              control={form.control}
              name="name"
@@ -150,25 +142,18 @@ export const SurveyVariablesCardItem = ({
                  ),
                },
                validate: (value) => {
-                  // if the variable name is already taken
-                  if (
-                    mode === "create" &&
-                    localSurvey.variables.find((variable) => variable.name === value)
-                  ) {
+                  if (mode === "create" && localSurvey.variables.find((v) => v.name === value)) {
                    return t(
                      "environments.surveys.edit.variable_name_is_already_taken_please_choose_another"
                    );
                  }
-
                  if (mode === "edit" && variable && variable.name !== value) {
-                    if (localSurvey.variables.find((variable) => variable.name === value)) {
+                    if (localSurvey.variables.find((v) => v.name === value)) {
                      return t(
                        "environments.surveys.edit.variable_name_is_already_taken_please_choose_another"
                      );
                    }
                  }
-
-                  // if it does not start with a letter
                  if (!/^[a-z]/.test(value)) {
                    return t("environments.surveys.edit.variable_name_must_start_with_a_letter");
                  }
@@ -180,8 +165,8 @@ export const SurveyVariablesCardItem = ({
                    <Input
                      {...field}
                      isInvalid={isNameError}
-                      type="text"
                      placeholder={t("environments.surveys.edit.field_name_eg_score_price")}
+                      onBlur={mode === "edit" ? () => form.handleSubmit(editSurveyVariable)() : undefined}
                    />
                  </FormControl>
                </FormItem>
@@ -192,28 +177,29 @@ export const SurveyVariablesCardItem = ({
              control={form.control}
              name="type"
              render={({ field }) => (
-                <Select
-                  {...field}
-                  onValueChange={(value) => {
-                    form.setValue("value", value === "number" ? 0 : "");
-                    field.onChange(value);
-                  }}>
-                  <SelectTrigger className="w-24">
-                    <SelectValue
-                      placeholder={t("environments.surveys.edit.select_type")}
-                      className="text-sm"
-                    />
-                  </SelectTrigger>
-                  <SelectContent>
-                    <SelectItem value={"number"}>{t("common.number")}</SelectItem>
-                    <SelectItem value={"text"}>{t("common.text")}</SelectItem>
-                  </SelectContent>
-                </Select>
+                <FormItem
+                  onBlur={mode === "edit" ? () => form.handleSubmit(editSurveyVariable)() : undefined}>
+                  <Select
+                    {...field}
+                    onValueChange={(value) => {
+                      form.setValue("value", value === "number" ? 0 : "");
+                      field.onChange(value);
+                    }}>
+                    <SelectTrigger className="w-24">
+                      <SelectValue placeholder={t("environments.surveys.edit.select_type")} />
+                    </SelectTrigger>
+                    <SelectContent>
+                      <SelectItem value="number">{t("common.number")}</SelectItem>
+                      <SelectItem value="text">{t("common.text")}</SelectItem>
+                    </SelectContent>
+                  </Select>
+                </FormItem>
              )}
            />

            <p className="text-slate-600">=</p>

+            {/* Value field: update on blur */}
            <FormField
              control={form.control}
              name="value"
@@ -222,23 +208,24 @@ export const SurveyVariablesCardItem = ({
                  <FormControl>
                    <Input
                      {...field}
-                      onChange={(e) => {
-                        field.onChange(variableType === "number" ? Number(e.target.value) : e.target.value);
-                      }}
+                      onChange={(e) =>
+                        field.onChange(variableType === "number" ? Number(e.target.value) : e.target.value)
+                      }
                      placeholder={t("environments.surveys.edit.initial_value")}
                      type={variableType === "number" ? "number" : "text"}
+                      onBlur={mode === "edit" ? () => form.handleSubmit(editSurveyVariable)() : undefined}
                    />
                  </FormControl>
                </FormItem>
              )}
            />

+            {/* Create / Delete buttons */}
            {mode === "create" && (
              <Button variant="secondary" type="submit" className="h-10 whitespace-nowrap">
                {t("environments.surveys.edit.add_variable")}
              </Button>
            )}
-
            {mode === "edit" && variable && (
              <Button
                variant="ghost"
--- a/apps/web/vite.config.mts
+++ b/apps/web/vite.config.mts
@@ -78,4 +78,4 @@ export default defineConfig({
    },
  },
  plugins: [tsconfigPaths(), react() as PluginOption],
-});
+});