fix(ui): handle React 18 batching for "Submit" button on details page. Fixes #13453

[MasonM]

Fixes #13453

Motivation

The React 18 upgrade in #13069 introduced a bug on the details page where the "Submit" button is disabled immediately on page load. I believe this is happening due to batching changes in React 18 that affect the following two useEffect() calls, which are common to all the components modified in this PR:

argo-workflows/ui/src/app/event-sources/event-source-details.tsx

Lines 68 to 81 in 729ac17

	useEffect(() => {
	(async () => {
	try {
	const newEventSource = await services.eventSource.get(name, namespace);
	setEventSource(newEventSource);
	setEdited(false); // set back to false
	setError(null);
	} catch (err) {
	setError(err);
	}
	})();
	}, [name, namespace]);
	useEffect(() => setEdited(true), [eventSource]);

The first useEffect() call runs immediately and invokes setEventSource(newEventSource), which causes the second useEffect() call to be fired, since the eventSource dependency has changed. Since both are invoking setEdited(), this is basically a race condition, since the state of edited is going to depend on whether these calls are batched together (the behavior in React 18) or fired separately (the old behavior).

Modifications

This PR turns the edited variable into derived state that dynamically compares objects using JSON.stringify(), which eliminates the need for useEffect(() => setEdited(true), [template]) call, and therefore fixes the race condition.

This will have a slight performance hit, which we could solve using useMemo(), but I wasn't sure if that was worth it.

Verification

Tested locally by editing a template at http://localhost:8081/workflow-templates?namespace=argo:

Screen.Recording.2024-09-12.at.4.53.59.PM.mp4

[agilgur5]

Hmm, so per #13410 (comment) and #13069 (comment), I was planning to revert the React upgrade on main (and did not cherry-pick it).

That being said, the difference in behavior should be handled regardless, but this may be lower priority as a result

[agilgur5]

Nice root cause analysis! We might want to double-check some other places for batching bugs too 🤔 seems like React didn't have any auto-detection for this unfortunately

I left some mostly stylistic comments below, with one behavior correction.

This will have a slight performance hit, which we could solve using useMemo(), but I wasn't sure if that was worth it.

Regarding useMemo, in this case I think it would be worth it, since these are all editor components, so they are latency sensitive to user input

[MasonM]

@agilgur5 Thanks for the quick and thorough review! I pushed a couple commits to fix the stylistic issues, switch to useMemo(), and fix the bug on sensor-details.tsx

[agilgur5]

Was thinking about the useMemo's dependencies list, and I'm thinking that we could simplify it even further if we only wanted to match this logic: given that the dependencies are object refs, this is pure ref equality, so we could technically simplify edited to template !== initialTemplate.

The current memoization logic is almost that, just that when the refs are equal, it will do the JSON comparison. But because we use setTemplate on any edit, I think it is effectively exactly the same. So I'm pretty sure the simplification would be equivalent.

What do you think?

(something something "react compiler" something something "dependencies list are too easy to make mistakes with" something something)

[MasonM]

I tried that initially, but there's a couple problems with relying on object equality:

1. If you make an edit, then undo that edit, edited will remain true.
2. If you click "Update", edited will remain true. I think this is happening because of how the <ObjectEditor> serializes/deserializes the value. When "Update" is fired, the <ObjectEditor> component will be re-rendered with the new value, which gets serialized here:

   argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 25 in 6cac182

   const [text, setText] = useState<string>(stringify(value, lang));

   
   Then, the onChange handler is fired (which is bound to setTemplate()), which parses it back to an object:

   argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 132 in 6cac182

   onChange(parse(v));

   
   Even though the object is going to have the same properties, it's still considered distinct.

[agilgur5]

Yes, that's correct; this would've been the previous behavior (prior to this bug) as well if I'm not mistaken.
So while the JSON equality is more accurate than the previous behavior, it does have a performance trade-off, which is perhaps not worthwhile solely for the sake of UI button accuracy.

just that when the refs are equal, it will do the JSON comparison. But because we use setTemplate on any edit,

I did make a mistake in my previous statement^ though, "equal" -> "unequal". Meaning the JSON comparison will pretty much happen on every change, so the useMemo is effectively not used as the refs are never equal.

Since this is an input box that is latency sensitive, I'm thinking that simplifying it to the previous behavior, even if it's not entirely accurate, would be worthwhile compared to a JSON comparison on each change, as the useMemo is effectively ignored.

[MasonM]

I checked, and on the main branch, the stringify() function is called 4 times for every change made in <ObjectEditor>:

1. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 25 in 5a2fa4e

   const [text, setText] = useState<string>(stringify(value, lang));

2. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 29 in 5a2fa4e

   useEffect(() => setText(stringify(value, lang)), [value]);

3. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 29 in 5a2fa4e

   useEffect(() => setText(stringify(value, lang)), [value]);

   (again)

4. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 38 in 5a2fa4e

   const editorText = stringify(parse(editor.current.editor.getValue()), lang);

It'd surprise me if adding a couple more calls would have a significant performance impact, but you're right that making performance worse might not be worth fixing this bug. I pushed 24c0206 to delete the isEqual() logic and go back to direct object comparisons.

To fix this without affecting performance, I think we need to refactor <ObjectEditor> to only work with strings, not parsed objects. Parsing will have to be lifted up to the details component (<ClusterWorkflowTemplateDetails> in this case), which can then easily compare strings to see what's changed. That's not hard, but it does involve quite a lot of changes, so it's probably best done in a separate PR.

[agilgur5]

Yea that first one could be made into a callback function instead very simply. The last one has that comment around it regarding whitespace

argo-workflows/ui/src/app/shared/components/object-editor.tsx

Lines 36 to 37 in 5a2fa4e

	// we ONLY want to change the text, if the normalized version has changed, this prevents white-space changes
	// from resulting in a significant change

I've optimized the rest of the UI by code-splitting this component before in #12150 so this component is a bit of a hassle 😕

but you're right that making performance worse might not be worth fixing this bug.

yea the buttons working even though there has been no semantic change is pretty tiny and fairly harmless. I'd argue it's maybe even worth further simplifying if possible just to remove more complex code.

Otherwise if we wanted to keep a bunch of features while being latency sensitive, a separate thread / web worker might be the way to go to keep the UI thread light.

[MasonM]

Here's a POC of what I was suggesting: main...MasonM:argo-workflows:poc-fix-13453

The <ObjectEditorPlain> component is a copy of <ObjectEditor> for POC purposes. The parsing and language handling code has been lifted up to the details component, where it's encapsulated in the useEditableObject() hook.

This completely eliminates all the unnecessary calls to stringify() when there's a change, and also fixes the bugs with the "Update" button. It does call parse() once on a change, but that's an improvement from the main branch, where it's called twice.

If that looks good to you, I can clean that up and enter a PR.

[agilgur5]

Just some stylistic comments below

[agilgur5]

Since this is JSDoc and not GoDoc, we can simplify this to be less repetitive / verbose. There's also a typo "that be edited"

We should also mention the ref comparison same as we did the JSON comparison in the isEqual function before. And can one-line it

Suggested change

	/**
	* useEditableResource is a hook to manage the state of a resource that be edited and updated.
	*/
	/** Hook that manages the state of a resource that can be edited. Uses ref comparison */

[agilgur5]

It also could make sense to rename this function/file use-editable-object to match object-editor.

Maybe even makes sense to have as part of the ObjectEditor component? 🤔 EDIT: I wrote that confusingly; I meant as an exported function in the same file, see below comment

[MasonM]

The other hooks that have documentation follow the <x> is a React hook format:

argo-workflows/ui/src/app/shared/use-resizable-width.ts

Line 27 in 5f70eee

* useResizableWidth is a React hook containing the requisite logic for allowing a user to resize an element's width by

argo-workflows/ui/src/app/shared/use-resizable-width.ts

Line 27 in 5f70eee

* useResizableWidth is a React hook containing the requisite logic for allowing a user to resize an element's width by

I updated the comment in cf9a3f3 to match, and renamed it to use-editable-object.

I don't see how this could be part of <ObjectEditor> because the edited variable is needed by the details components.

[agilgur5]

The other hooks that have documentation follow the <x> is a React hook format:

Ah that works for consistency then I suppose. Some of the original ones written were almost certainly bringing GoDoc conventions into JSDoc (there's a handful of non-JS idioms in the UI here and there).

I don't see how this could be part of <ObjectEditor> because the edited variable is needed by the details components.

I meant as another exported function inside the same file, not directly integrated into the component. Sorry I can see how my wording above could be confusing.

As in, there are some conventions in the ecosystem of import {useComponent, Component} that we could follow. Although the usage is a tad different here with regard to the tracked state and props (they are not quite matching)

[agilgur5]

@MasonM any thoughts on the above? Otherwise this LGTM and I can merge

[MasonM]

I don't think it makes sense to couple this to <ObjectEditor>, because that will make it more difficult to move forward with the approach I posted above in #13593 (comment). That POC fully decouples <ObjectEditorPlain> from all serialization/deseralization and moves that logic into useEditableComponent(), which significantly improves performance and is a bit cleaner (IMO). Of course, that's a fairly significant change, and probably should come after this PR, but I think we'll want to do this eventually.

But if you feel strongly, I can go ahead and make that change.

[agilgur5]

I don't feel strongly.
I didn't read that change too closely yet so from a quick look I actually thought putting the imports/files together might help move in that direction, but I might've been mistaken. Since the performance change may make it even more likely that any consumer of the component would want to also use the hook (and it is currently used like that) and so combining the imports makes it more obvious how to use.

useEditableComponent()

I guess your intent with this naming is that the hook could in the future be extended to other editable components as well? As in that it's not specific to ObjectEditor per se

[MasonM]

I guess your intent with this naming is that the hook could in the future be extended to other editable components as well?

Honestly, that was a typo. I meant useEditableObject(). It could theoretically be extended to other editable components, but I can't think of anything that would benefit.

[agilgur5]

Oh ok 😅 Then I think it would make sense to combine the imports / exports so that it's more obvious the hook is supposed to be used together with the component: import {ObjectEditor, useEditableObject}

Technically doesn't have to be in the same file, could have an import/export stub instead

[agilgur5]

LGTM, thanks for iterating on this and ironing out the performance too!