Skip to content

fix: improve formbuilder logic #1135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
9 changes: 1 addition & 8 deletions core/app/c/[communitySlug]/forms/[formSlug]/edit/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,7 @@ export default async function Page(props: {
formSlug: string;
communitySlug: string;
}>;
searchParams: Promise<{
unsavedChanges: boolean;
}>;
}) {
const searchParams = await props.searchParams;

const { unsavedChanges } = searchParams;

Comment on lines -31 to -38
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic does not need to happen server side, we can just do it client side. this is much faster

const params = await props.params;

const { formSlug, communitySlug } = params;
Expand Down Expand Up @@ -104,7 +97,7 @@ export default async function Page(props: {
<div className="flex items-center gap-2">
<FormCopyButton formSlug={formSlug} />
{/* <ArchiveFormButton id={form.id} className="border border-gray-950 px-4" />{" "} */}
<SaveFormButton form={formBuilderId} disabled={!unsavedChanges} />
<SaveFormButton form={formBuilderId} />
</div>
}
>
Expand Down
8 changes: 6 additions & 2 deletions core/app/components/FormBuilder/ElementPanel/SelectAccess.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,19 @@ export const SelectAccess = () => {
<hr />
<Select onValueChange={field.onChange} defaultValue={field.value}>
<FormControl>
<SelectTrigger>
<SelectTrigger data-testid="select-form-access">
<SelectValue placeholder="Select a type" />
</SelectTrigger>
</FormControl>
<SelectContent>
{Object.values(FormAccessType).map((t) => {
const { Icon, description, name } = iconsAndCopy[t];
return (
<SelectItem key={t} value={t.toString()}>
<SelectItem
key={t}
value={t.toString()}
data-testid={`select-form-access-${t}`}
>
<div className="flex h-auto flex-1 flex-shrink-0 items-center gap-2">
<Icon size={16} />
<div className="flex flex-col items-start">
Expand Down
142 changes: 121 additions & 21 deletions core/app/components/FormBuilder/FormBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
import type { DragEndEvent } from "@dnd-kit/core";

import * as React from "react";
import { useCallback, useReducer, useRef } from "react";
import { usePathname, useRouter, useSearchParams } from "next/navigation";
import { useCallback, useMemo, useReducer, useRef } from "react";
import { DndContext, KeyboardSensor, PointerSensor, useSensor, useSensors } from "@dnd-kit/core";
import { restrictToParentElement, restrictToVerticalAxis } from "@dnd-kit/modifiers";
import {
Expand All @@ -15,7 +14,14 @@ import {
import { zodResolver } from "@hookform/resolvers/zod";
import { useFieldArray, useForm } from "react-hook-form";

import type { Stages } from "db/public";
import type {
FormElementsId,
FormsId,
NewFormElements,
NewFormElementToPubType,
Stages,
} from "db/public";
import { formElementsInitializerSchema } from "db/public";
import { logger } from "logger";
import { Form, FormControl, FormField, FormItem } from "ui/form";
import { useUnsavedChangesWarning } from "ui/hooks";
Expand All @@ -35,6 +41,7 @@ import { ElementPanel } from "./ElementPanel";
import { FormBuilderProvider } from "./FormBuilderContext";
import { FormElement } from "./FormElement";
import { formBuilderSchema, isButtonElement } from "./types";
import { useIsChanged } from "./useIsChanged";

const elementPanelReducer: React.Reducer<PanelState, PanelEvent> = (prevState, event) => {
const { eventName } = event;
Expand Down Expand Up @@ -107,13 +114,98 @@ type Props = {
stages: Stages[];
};

/**
* Only sends the dirty fields to the server
*/
const preparePayload = ({
formValues,
defaultValues,
}: {
defaultValues: FormBuilderSchema;
formValues: FormBuilderSchema;
}) => {
const { upserts, deletes, relatedPubTypes, deletedRelatedPubTypes } =
formValues.elements.reduce<{
upserts: NewFormElements[];
deletes: FormElementsId[];
relatedPubTypes: NewFormElementToPubType[];
deletedRelatedPubTypes: FormElementsId[];
}>(
(acc, element, index) => {
if (element.deleted) {
if (element.elementId) {
acc.deletes.push(element.elementId);
}
} else if (!element.elementId) {
// Newly created elements have no elementId, so generate an id to use
const id = crypto.randomUUID() as FormElementsId;
acc.upserts.push(
formElementsInitializerSchema.parse({
formId: formValues.formId,
...element,
id,
})
);
if (element.relatedPubTypes) {
for (const pubTypeId of element.relatedPubTypes) {
acc.relatedPubTypes.push({ A: id, B: pubTypeId });
}
}
} else if (element.updated) {
// check whether the element is reeeaally updated minus the updated field
const { updated: _, id: _id, ...elementWithoutUpdated } = element;
const { updated, id, ...rest } =
defaultValues.elements.find((e) => e.elementId === element.elementId) ?? {};

const defaultElement = rest as Omit<FormElementData, "updated" | "id">;

if (JSON.stringify(defaultElement) === JSON.stringify(elementWithoutUpdated)) {
return acc;
}
Comment on lines +162 to +164
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very sophisticated diffing. please recommend a better approach if you know one


acc.upserts.push(
formElementsInitializerSchema.parse({
...element,
formId: formValues.formId,
id: element.elementId,
})
); // TODO: only update changed columns
if (element.relatedPubTypes) {
// If we are updating to an empty array and there were related pub types before, we should clear out all related pub types
if (
element.relatedPubTypes.length === 0 &&
defaultElement.relatedPubTypes?.length
) {
Comment on lines +175 to +178
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added this defaultElement.relatedPubTypes?.length check, bc otherwise if you swapped a relation element back and forth it would assume something had to be deleted, which didn't make sense

acc.deletedRelatedPubTypes.push(element.elementId);
} else {
for (const pubTypeId of element.relatedPubTypes) {
acc.relatedPubTypes.push({ A: element.elementId, B: pubTypeId });
}
}
}
}
return acc;
},
{ upserts: [], deletes: [], relatedPubTypes: [], deletedRelatedPubTypes: [] }
);

const access = formValues.access !== defaultValues.access ? formValues.access : undefined;

return {
formId: formValues.formId,
upserts,
deletes,
access,
relatedPubTypes,
deletedRelatedPubTypes,
};
};

export function FormBuilder({ pubForm, id, stages }: Props) {
const router = useRouter();
const pathname = usePathname();
const params = useSearchParams();
const form = useForm<FormBuilderSchema>({
resolver: zodResolver(formBuilderSchema),
values: {
const [isChanged, setIsChanged] = useIsChanged();
Copy link
Member Author

@tefkah tefkah Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a small custom nuqs hook reads from/updates the isChanged query param. using nuqs we can do this without a round trip to the server, which we want. this is because shallow is set to true by default. maybe i should add that option to make it slightly more clear that these are client-side only url updates

import { parseAsBoolean, useQueryState } from "nuqs";
export const useIsChanged = () => {
const [isChanged, setIsChanged] = useQueryState(
"unsavedChanges",
parseAsBoolean.withDefault(false).withOptions({
history: "replace",
scroll: false,
})
);
return [isChanged, setIsChanged] as const;
};


const defaultValues = useMemo(() => {
return {
elements: pubForm.elements.map((e) => {
// Do not include extra fields here
const { slug, id, fieldName, ...rest } = e;
Expand All @@ -122,7 +214,12 @@ export function FormBuilder({ pubForm, id, stages }: Props) {
}),
access: pubForm.access,
formId: pubForm.id,
},
};
}, [pubForm]);

const form = useForm<FormBuilderSchema>({
resolver: zodResolver(formBuilderSchema),
values: defaultValues,
});

const sidebarRef = useRef(null);
Expand All @@ -145,22 +242,25 @@ export function FormBuilder({ pubForm, id, stages }: Props) {
control: form.control,
});

const formValues = form.getValues();

useUnsavedChangesWarning(form.formState.isDirty);

const payload = useMemo(
() => preparePayload({ formValues, defaultValues }),
[formValues, defaultValues]
);

React.useEffect(() => {
const newParams = new URLSearchParams(params);
if (form.formState.isDirty) {
newParams.set("unsavedChanges", "true");
} else {
newParams.delete("unsavedChanges");
}
router.replace(`${pathname}?${newParams.toString()}`, { scroll: false });
}, [form.formState.isDirty, params]);
setIsChanged(
payload.upserts.length > 0 || payload.deletes.length > 0 || payload.access != null
);
}, [payload]);

const runSaveForm = useServerAction(saveForm);

const onSubmit = async (formData: FormBuilderSchema) => {
//TODO: only submit dirty fields
const result = await runSaveForm(formData);
const result = await runSaveForm(payload);
if (didSucceed(result)) {
toast({
className: "rounded border-emerald-100 bg-emerald-50",
Expand Down Expand Up @@ -250,7 +350,7 @@ export function FormBuilder({ pubForm, id, stages }: Props) {
dispatch={dispatch}
slug={pubForm.slug}
stages={stages}
isDirty={form.formState.isDirty}
isDirty={isChanged}
>
<Tabs defaultValue="builder" className="pr-[380px]">
<div className="px-6">
Expand Down
10 changes: 7 additions & 3 deletions core/app/components/FormBuilder/FormElement.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type FormElementProps = {
};

export const FormElement = ({ element, index, isEditing, isDisabled }: FormElementProps) => {
const { attributes, listeners, setNodeRef, transform, transition } = useSortable({
const { attributes, listeners, isDragging, setNodeRef, transform, transition } = useSortable({
id: element.id,
disabled: isDisabled,
});
Expand Down Expand Up @@ -78,7 +78,8 @@ export const FormElement = ({ element, index, isEditing, isDisabled }: FormEleme
"group flex min-h-[76px] flex-1 flex-shrink-0 items-center justify-between gap-3 self-stretch rounded border border-l-[12px] border-solid border-gray-200 border-l-emerald-100 bg-white p-3 pr-4",
isEditing && "border-sky-500 border-l-blue-500",
isDisabled && "cursor-auto opacity-50",
element.deleted && "border-l-red-200"
element.deleted && "border-l-red-200",
isDragging && "z-10 cursor-grabbing"
)}
>
<div className="flex flex-1 flex-shrink-0 flex-wrap justify-start gap-0.5">
Expand Down Expand Up @@ -110,7 +111,10 @@ export const FormElement = ({ element, index, isEditing, isDisabled }: FormEleme
aria-label="Drag handle"
disabled={isDisabled || element.deleted}
variant="ghost"
className="p-1.5 opacity-0 group-focus-within:opacity-100 group-hover:opacity-100"
className={cn(
"p-1.5 opacity-0 group-focus-within:opacity-100 group-hover:opacity-100",
isDragging ? "cursor-grabbing" : "cursor-grab"
)}
{...listeners}
{...attributes}
tabIndex={0}
Expand Down
5 changes: 4 additions & 1 deletion core/app/components/FormBuilder/SaveFormButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
import { Button } from "ui/button";
import { cn } from "utils";

import { useIsChanged } from "./useIsChanged";

type Props = {
form: string;
className?: string;
disabled?: boolean;
};

export const SaveFormButton = ({ form, className, disabled }: Props) => {
const [isChanged] = useIsChanged();
return (
<Button
variant="default"
Expand All @@ -18,7 +21,7 @@ export const SaveFormButton = ({ form, className, disabled }: Props) => {
form={form}
type="submit"
data-testid="save-form-button"
disabled={disabled}
disabled={disabled != null ? disabled : !isChanged}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i should just remov the disabled prop entirely and make it !isChanged by default

>
Save
</Button>
Expand Down
Loading
Loading