Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Oct 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fe7d6be
fix(ui): Submit button grayed out on details page. Fixes #13453
MasonM Sep 12, 2024
2747d17
fix(ui): stylistic fixes
MasonM Sep 14, 2024
1ef4958
fix(ui): cache using useMemo() and fix sensor-details.tsx
MasonM Sep 14, 2024
c249458
Merge branch 'main' into fix-13453
MasonM Sep 27, 2024
24c0206
refactor(ui): Extract to hook and delete deep Object comparison
MasonM Sep 28, 2024
a54bbd3
Merge branch 'main' into fix-13453
MasonM Sep 28, 2024
cf9a3f3
refactor: rename to useEditableObject and update comments
MasonM Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {ZeroState} from '../shared/components/zero-state';
import {Context} from '../shared/context';
import {historyUrl} from '../shared/history';
import {services} from '../shared/services';
import {useEditableResource} from '../shared/use-editable-resource';
import {useQueryParams} from '../shared/use-query-params';
import * as nsUtils from '../shared/namespaces';
import {WorkflowDetailsList} from '../workflows/components/workflow-details-list/workflow-details-list';
Expand All @@ -36,8 +37,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route
const [columns, setColumns] = useState<models.Column[]>([]);

const [error, setError] = useState<Error>();
const [template, setTemplate] = useState<ClusterWorkflowTemplate>();
const [edited, setEdited] = useState(false);
const [template, edited, setTemplate, resetTemplate] = useEditableResource<ClusterWorkflowTemplate>();

useEffect(
useQueryParams(history, p => {
Expand All @@ -47,7 +47,6 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route
[history]
);

useEffect(() => setEdited(true), [template]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agilgur5 agilgur5 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

useEffect(() => {
history.push(historyUrl('cluster-workflow-templates/{name}', {name, sidePanel, tab}));
}, [name, sidePanel, tab]);
Expand All @@ -56,8 +55,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route
(async () => {
try {
const newTemplate = await services.clusterWorkflowTemplate.get(name);
setTemplate(newTemplate);
setEdited(false); // set back to false
resetTemplate(newTemplate);
setError(null);
} catch (err) {
setError(err);
Expand Down Expand Up @@ -106,15 +104,14 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route
action: () => {
services.clusterWorkflowTemplate
.update(template, name)
.then(setTemplate)
.then(resetTemplate)
.then(() =>
notifications.show({
content: 'Updated',
type: NotificationType.Success
})
)
.then(() => setError(null))
.then(() => setEdited(false))
.catch(setError);
}
},
Expand Down
18 changes: 6 additions & 12 deletions ui/src/app/cron-workflows/cron-workflow-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {Context} from '../shared/context';
import {historyUrl} from '../shared/history';
import {services} from '../shared/services';
import {useQueryParams} from '../shared/use-query-params';
import {useEditableResource} from '../shared/use-editable-resource';
import {WidgetGallery} from '../widgets/widget-gallery';
import {WorkflowDetailsList} from '../workflows/components/workflow-details-list/workflow-details-list';
import {CronWorkflowEditor} from './cron-workflow-editor';
Expand All @@ -35,8 +36,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
const [workflows, setWorkflows] = useState<Workflow[]>([]);
const [columns, setColumns] = useState<models.Column[]>([]);

const [cronWorkflow, setCronWorkflow] = useState<CronWorkflow>();
const [edited, setEdited] = useState(false);
const [cronWorkflow, edited, setCronWorkflow, resetCronWorkflow] = useEditableResource<CronWorkflow>();
const [error, setError] = useState<Error>();

useEffect(
Expand All @@ -63,14 +63,11 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
useEffect(() => {
services.cronWorkflows
.get(name, namespace)
.then(setCronWorkflow)
.then(() => setEdited(false))
.then(resetCronWorkflow)
.then(() => setError(null))
.catch(setError);
}, [namespace, name]);

useEffect(() => setEdited(true), [cronWorkflow]);

useEffect(() => {
(async () => {
const workflowList = await services.workflows.list(namespace, null, [`${models.labels.cronWorkflow}=${name}`], {limit: 50});
Expand All @@ -91,8 +88,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
action: () =>
services.cronWorkflows
.suspend(name, namespace)
.then(setCronWorkflow)
.then(() => setEdited(false))
.then(resetCronWorkflow)
.then(() => setError(null))
.catch(setError),
disabled: !cronWorkflow || edited
Expand All @@ -103,8 +99,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
action: () =>
services.cronWorkflows
.resume(name, namespace)
.then(setCronWorkflow)
.then(() => setEdited(false))
.then(resetCronWorkflow)
.then(() => setError(null))
.catch(setError),
disabled: !cronWorkflow || !cronWorkflow.spec.suspend || edited
Expand Down Expand Up @@ -142,10 +137,9 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
cronWorkflow.metadata.namespace
)
)
.then(setCronWorkflow)
.then(resetCronWorkflow)
.then(() => notifications.show({content: 'Updated', type: NotificationType.Success}))
.then(() => setError(null))
.then(() => setEdited(false))
.catch(setError);
}
},
Expand Down
12 changes: 4 additions & 8 deletions ui/src/app/event-sources/event-source-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {Context} from '../shared/context';
import {historyUrl} from '../shared/history';
import {services} from '../shared/services';
import {useQueryParams} from '../shared/use-query-params';
import {useEditableResource} from '../shared/use-editable-resource';
import {EventsPanel} from '../workflows/components/events-panel';
import {EventSourceEditor} from './event-source-editor';
import {EventSourceLogsViewer} from './event-source-log-viewer';
Expand Down Expand Up @@ -52,9 +53,8 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro
[namespace, name, tab, selectedNode]
);

const [edited, setEdited] = useState(false);
const [error, setError] = useState<Error>();
const [eventSource, setEventSource] = useState<EventSource>();
const [eventSource, edited, setEventSource, resetEventSource] = useEditableResource<EventSource>();

const selected = (() => {
if (!selectedNode) {
Expand All @@ -69,17 +69,14 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro
(async () => {
try {
const newEventSource = await services.eventSource.get(name, namespace);
setEventSource(newEventSource);
setEdited(false); // set back to false
resetEventSource(newEventSource);
setError(null);
} catch (err) {
setError(err);
}
})();
}, [name, namespace]);

useEffect(() => setEdited(true), [eventSource]);

useCollectEvent('openedEventSourceDetails');

return (
Expand All @@ -100,14 +97,13 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro
action: () =>
services.eventSource
.update(eventSource, name, namespace)
.then(setEventSource)
.then(resetEventSource)
.then(() =>
notifications.show({
content: 'Updated',
type: NotificationType.Success
})
)
.then(() => setEdited(false))
.then(() => setError(null))
.catch(setError)
},
Expand Down
12 changes: 4 additions & 8 deletions ui/src/app/sensors/sensor-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {Context} from '../shared/context';
import {historyUrl} from '../shared/history';
import {services} from '../shared/services';
import {useQueryParams} from '../shared/use-query-params';
import {useEditableResource} from '../shared/use-editable-resource';
import {SensorEditor} from './sensor-editor';
import {SensorSidePanel} from './sensor-side-panel';

Expand All @@ -29,8 +30,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
const [name] = useState(match.params.name);
const [tab, setTab] = useState<string>(queryParams.get('tab'));

const [sensor, setSensor] = useState<Sensor>();
const [edited, setEdited] = useState(false);
const [sensor, edited, setSensor, resetSensor] = useEditableResource<Sensor>();
const [selectedLogNode, setSelectedLogNode] = useState<Node>(queryParams.get('selectedLogNode'));
const [error, setError] = useState<Error>();

Expand Down Expand Up @@ -58,14 +58,11 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
useEffect(() => {
services.sensor
.get(name, namespace)
.then(setSensor)
.then(() => setEdited(false))
.then(resetSensor)
.then(() => setError(null))
.catch(setError);
}, [namespace, name]);

useEffect(() => setEdited(true), [sensor]);

useCollectEvent('openedSensorDetails');

const selected = (() => {
Expand Down Expand Up @@ -94,9 +91,8 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
action: () =>
services.sensor
.update(sensor, namespace)
.then(setSensor)
.then(resetSensor)
.then(() => notifications.show({content: 'Updated', type: NotificationType.Success}))
.then(() => setEdited(false))
.then(() => setError(null))
.catch(setError)
},
Expand Down
22 changes: 22 additions & 0 deletions ui/src/app/shared/use-editable-resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import {useState} from 'react';

/**
* useEditableResource is a hook to manage the state of a resource that be edited and updated.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

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 */

Copy link
Contributor

@agilgur5 agilgur5 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agilgur5 agilgur5 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agilgur5 agilgur5 Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agilgur5 agilgur5 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

export function useEditableResource<T>(initial?: T): [T, boolean, React.Dispatch<T>, (value: T) => void] {
const [value, setValue] = useState<T>(initial);
const [initialValue, setInitialValue] = useState<T>(initial);

// TODO: Fix this so that it handles object comparison properly.
// Currently, this returns true if you make a change and immediately undo it, or if you save your changes.
// This could be solved using "const edited = JSON.stringify(value) !== JSON.stringify(initialValue)",
// but that has a performance penalty.
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
const edited = value !== initialValue;

function resetValue(value: T) {
setValue(value);
setInitialValue(value);
}

return [value, edited, setValue, resetValue];
}
12 changes: 4 additions & 8 deletions ui/src/app/workflow-templates/workflow-template-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {WorkflowTemplate, Workflow} from '../../models';
import {uiUrl} from '../shared/base';
import {ErrorNotice} from '../shared/components/error-notice';
import {Loading} from '../shared/components/loading';
import {useEditableResource} from '../shared/use-editable-resource';
import {useCollectEvent} from '../shared/use-collect-event';
import {ZeroState} from '../shared/components/zero-state';
import {Context} from '../shared/context';
Expand All @@ -34,11 +35,8 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone
const [workflows, setWorkflows] = useState<Workflow[]>([]);
const [columns, setColumns] = useState<models.Column[]>([]);

const [template, setTemplate] = useState<WorkflowTemplate>();
const [template, edited, setTemplate, resetTemplate] = useEditableResource<WorkflowTemplate>();
const [error, setError] = useState<Error>();
const [edited, setEdited] = useState(false);

useEffect(() => setEdited(true), [template]);

useEffect(
useQueryParams(history, p => {
Expand All @@ -64,8 +62,7 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone
useEffect(() => {
services.workflowTemplate
.get(name, namespace)
.then(setTemplate)
.then(() => setEdited(false)) // set back to false
.then(resetTemplate)
.then(() => setError(null))
.catch(setError);
}, [name, namespace]);
Expand Down Expand Up @@ -106,9 +103,8 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone
action: () =>
services.workflowTemplate
.update(template, name, namespace)
.then(setTemplate)
.then(resetTemplate)
.then(() => notifications.show({content: 'Updated', type: NotificationType.Success}))
.then(() => setEdited(false))
.then(() => setError(null))
.catch(setError)
},
Expand Down
Loading