fix: improve formbuilder logic#1135
Conversation
…nt does not 'duck' under existing ones
| searchParams: Promise<{ | ||
| unsavedChanges: boolean; | ||
| }>; | ||
| }) { | ||
| const searchParams = await props.searchParams; | ||
|
|
||
| const { unsavedChanges } = searchParams; | ||
|
|
There was a problem hiding this comment.
this logic does not need to happen server side, we can just do it client side. this is much faster
| if (JSON.stringify(defaultElement) === JSON.stringify(elementWithoutUpdated)) { | ||
| return acc; | ||
| } |
There was a problem hiding this comment.
very sophisticated diffing. please recommend a better approach if you know one
| const form = useForm<FormBuilderSchema>({ | ||
| resolver: zodResolver(formBuilderSchema), | ||
| values: { | ||
| const [isChanged, setIsChanged] = useIsChanged(); |
There was a problem hiding this comment.
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
| type="submit" | ||
| data-testid="save-form-button" | ||
| disabled={disabled} | ||
| disabled={disabled != null ? disabled : !isChanged} |
There was a problem hiding this comment.
maybe i should just remov the disabled prop entirely and make it !isChanged by default
| if ( | ||
| element.relatedPubTypes.length === 0 && | ||
| defaultElement.relatedPubTypes?.length | ||
| ) { |
There was a problem hiding this comment.
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
allisonking
left a comment
allisonking
left a comment
There was a problem hiding this comment.
woo this is awesome! approving, though I don't know about the ranking part either though, so maybe wait for Kalil to weigh in on that part?
3 participants
Issue(s) Resolved
Resolves #1143
High-level Explanation of PR
Roughly does 3 things:
accessif that is the only thing changed. Previously also the formelements needed to have been moved/deleted/added in order for a change in access to go throughMoving diff to client
This was put as TODOs in the code in some places, and it made my life easier, so i did.
The main change to the diffing algorithm is here:
https://github.com/pubpub/platform/blob/23a047bdf927b2c8a4d10f42a41626b6976a66b1/core/app/components/FormBuilder/FormBuilder.tsx#L155-L164
May be a better way of doing this but i thought this was fine for now (i expect the order of keys to be relatively stable).
Basically, if nothing except maybe the id/updatedAt of the form element has changed, we do not consider it a change.
Note
Caveat: A "rank" change is still considered a change. If two elements are swapped back and forth, their rank may still have been changed. I find it hard to check for this case as i'll admit i don't fully understand the ranking implementation, maybe @kalilsn you have some insight on how to ignore swapping back and forth cases.
Warning
Moving the diffs to the client now does carry slightly more risk for weird stuff happening, but not really that much more than before
Update logic changes
Since all the changes now arrive as separate entities, we can more easily only update form accesstype if that has changed.
QoL changes
cursor-dragrather thancursor-pointercursor-dragging, rather thancursor-pointerDemo:
(slightly hard to see bc recording my screen apparently makes the cursor almost always be the default one)
Screen.Recording.2025-04-14.at.12.14.28.mov
Test Plan
Screenshots (if applicable)
Notes