Related pubs value components fixups

[allisonking]

Issue(s) Resolved

Closes #791 at last!

High-level Explanation of PR

#910 didn't test each value field type. This goes through and tests them all and fixes up various errors that came up.

All value fields should work now, EXCEPT for trying to delete all the related pubs in a single field (see comment)

Test Plan

Try adding related versions of every field and make sure they work 😮‍💨

Screenshots (if applicable)

Notes

[allisonking]

need to instantiate uppy with an id so that we can have multiple on one page

[allisonking]

Deleting all the related pubs for a given field does not currently work, and I'm not sure the best way to make it work. the problem is that it'll go to the updatePub endpoint as pubValues={"croccroc:authors": []} and we can't really tell from that that it is a related pub array vs a single array field. Currently, normalizePubValues returns an empty array if the user tries to delete all the related pubs of a field, which makes the update func do nothing since it thinks there is nothing to update.

I think there are a number of ways around this though no elegant ones I can think of. though I think @tefkah might be reworking this whole func?

[allisonking]

There is a bug here regarding autosaving and dirty states. To reproduce:

1. Add a related pub field. Member id is a good one.
2. Go to the form fill page and create a pub. Create should work fine 👍
3. Now go to the edit version of the page (i.e. http://localhost:3000/c/{communitySlug}/public/forms/{formSlug}/fill?pubId={pubId})
4. Add another related pub and give its value a member id.
5. During this time, autosave will likely kick in because it takes a bit of time to add a related pub. However, it'll filter out the member field because it's invalid while the field does not have a value
6. However, for some reason the dirty state is cleared now despite the member field not having been submitted since it was invalid. Perhaps because the autosave cleared the dirty state? or is this a race condition...?
7. The form will not autosave again since it does not detect any changes
8. Manually saving the form will not save the new field since it does not think that field is dirty
9. the field never gets saved!

I haven't quite figured out what's going on yet. I think I'm likely not setting the form state properly so the form is not detecting dirty states quite right. It also seems like editing one related pub field makes ALL related pub fields dirty 🤔

[allisonking]

moving the serialization here is a lot nicer—I remember I couldn't do it before because the editor would keep rendering, but I guess that's under control now. in any case, this makes it so that we don't have to serialize in preparePayload. more importantly, it means the object in react-hook-form is always a plain json which is needed for rhf to do its comparisons properly.

this only really shows up as a problem now because before the editor would always serialize/deserialize itself. in the case of related pubs, we load in a json without sending it over to an editor so we end up in a weird state where sometimes the editor data in rfh is a node and sometimes it is a json. now it is always a json!

[allisonking]

There is a bug here regarding autosaving and dirty states. To reproduce:

1. Add a related pub field. Member id is a good one.
2. Go to the form fill page and create a pub. Create should work fine 👍
3. Now go to the edit version of the page (i.e. http://localhost:3000/c/{communitySlug}/public/forms/{formSlug}/fill?pubId={pubId})
4. Add another related pub and give its value a member id.
5. During this time, autosave will likely kick in because it takes a bit of time to add a related pub. However, it'll filter out the member field because it's invalid while the field does not have a value
6. However, for some reason the dirty state is cleared now despite the member field not having been submitted since it was invalid. Perhaps because the autosave cleared the dirty state? or is this a race condition...?
7. The form will not autosave again since it does not detect any changes
8. Manually saving the form will not save the new field since it does not think that field is dirty
9. the field never gets saved!

I haven't quite figured out what's going on yet. I think I'm likely not setting the form state properly so the form is not detecting dirty states quite right. It also seems like editing one related pub field makes ALL related pub fields dirty 🤔

It appears what is happening is:

4. Add another related pub and give its value a member id
5. This will be marked as invalid while you're searching for a user. Validation is triggering from the call to handleSubmit in the form's onChange

   platform/core/app/components/pubs/PubEditor/PubEditorClient.tsx

   Lines 344 to 348 in 05a0cdf

   	onChange={
   	withAutoSave
   	? formInstance.handleSubmit(handleAutoSave, handleAutoSaveOnError)
   	: undefined
   	}

6. The form will be ready to save just the valid fields once the autosave timer hits
7. You can finish searching for a user. The value doesn't become valid though because revalidation isn't triggered
8. There is no further onChange triggered either
9. The form submits with only valid values, and the added value is lost!

One way to fix this which I've implemented is to make sure validation retriggers in step 7, when the user is done being searched for. I've done this by calling the field's onBlur method when the popover closes (our form validates onBlur)

Ideally we are also able to trigger the form's onChange on field array manipulation—append, remove, etc. However these don't trigger onChange (see react-hook-form/react-hook-form#3978). I think we'd have to redo our autosave logic to use the form's watch instead, but iirc that caused a lot of renders when I first tried to implement it. might be worth trying if we see more autosave issues, but for now, adding the onBlur call at least makes it so that, even though the autosave does not include the errored field, the next time you hit save, the value will be saved

[tefkah]

Looks really good!! Fantastic work getting all of this to work.

I'm still reviewing, but i'll add comments here as I go

[tefkah]

currently it's not possible to edit values after you've added them, which makes editing them kind of hard. would this be possible to change?

[tefkah]

works great! only have making the value editable as a suggestion, but it's not necessary to merge it.

great work with this allison!

[tefkah]

we could perhaps keep the value editable by just using it as the "label" for the popover, like so
(can't add it as a suggestion as the return showValue ? ( is not part of the diff :( )

	return (
		<Popover
			open={isPopoverOpen}
			onOpenChange={(open) => {
				if (!open && onBlur) {
					// In order to retrigger validation
					onBlur();
				}
				setPopoverIsOpen(open);
			}}
		>
			<PopoverTrigger asChild>
				<Button
					type="button"
					data-testid="add-related-value"
					variant="link"
					size="sm"
					className={cn("flex h-4 gap-1 p-0 text-blue-500", {
						"text-red-500": valueError,
					})}
				>
					{valueError && <TriangleAlert />}
					<span className="max-w-[20ch] truncate md:max-w-[60ch]">
						{showValue ? value.toString() : label}
					</span>
					{showValue ? <Pencil size={12} /> : <Plus size={12} />}
				</Button>
			</PopoverTrigger>
			<PopoverContent side="left">
				<PubFieldFormElement {...props} element={element} slug={slug} label={label} />
			</PopoverContent>
		</Popover>
	);
};

[tefkah]

looks like so

Screen.Recording.2025-02-11.at.14.10.08.mov

[allisonking]

oh that's quite nice! I had punted on it since katy and I talked briefly about how it might work but punted since it wasn't in the original scope of the ticket, but I think your solution is simple and elegant. I'll add it now!