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 1 commit
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
15 changes: 9 additions & 6 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 @@ -10,6 +10,7 @@ import {ClusterWorkflowTemplate, Workflow} from '../../models';
import {uiUrl} from '../shared/base';
import {ErrorNotice} from '../shared/components/error-notice';
import {Loading} from '../shared/components/loading';
import {isEqual} from '../shared/components/object-parser';
import {useCollectEvent} from '../shared/use-collect-event';
import {ZeroState} from '../shared/components/zero-state';
import {Context} from '../shared/context';
Expand Down Expand Up @@ -37,7 +38,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route

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

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

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

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
Member

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
Member

@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.

const resetTemplate = (template: ClusterWorkflowTemplate) => {
setTemplate(template);
setInitialTemplate(template);
};
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
const edited = !isEqual(template, initialTemplate);
useEffect(() => {
history.push(historyUrl('cluster-workflow-templates/{name}', {name, sidePanel, tab}));
}, [name, sidePanel, tab]);
Expand All @@ -56,8 +61,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 +110,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
22 changes: 12 additions & 10 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 @@ -10,6 +10,7 @@ import {CronWorkflow, Link, Workflow} from '../../models';
import {uiUrl} from '../shared/base';
import {ErrorNotice} from '../shared/components/error-notice';
import {openLinkWithKey} from '../shared/components/links';
import {isEqual} from '../shared/components/object-parser';
import {Loading} from '../shared/components/loading';
import {useCollectEvent} from '../shared/use-collect-event';
import {ZeroState} from '../shared/components/zero-state';
Expand All @@ -36,7 +37,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr
const [columns, setColumns] = useState<models.Column[]>([]);

const [cronWorkflow, setCronWorkflow] = useState<CronWorkflow>();
const [edited, setEdited] = useState(false);
const [initialCronWorkflow, setInitialCronWorkflow] = useState<CronWorkflow>();
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
const [error, setError] = useState<Error>();

useEffect(
Expand All @@ -63,13 +64,17 @@ 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]);
const resetCronWorkflow = (cronWorkflow: CronWorkflow) => {
setCronWorkflow(cronWorkflow);
setInitialCronWorkflow(cronWorkflow);
};

const edited = !isEqual(cronWorkflow, initialCronWorkflow);

useEffect(() => {
(async () => {
Expand All @@ -91,8 +96,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 +107,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 +145,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
16 changes: 10 additions & 6 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 @@ -11,6 +11,7 @@ import {ID} from '../event-flow/id';
import {uiUrl} from '../shared/base';
import {ErrorNotice} from '../shared/components/error-notice';
import {Loading} from '../shared/components/loading';
import {isEqual} from '../shared/components/object-parser';
import {useCollectEvent} from '../shared/use-collect-event';
import {Context} from '../shared/context';
import {historyUrl} from '../shared/history';
Expand Down Expand Up @@ -52,9 +53,9 @@ 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 [initialEventSource, setInitialEventSource] = useState<EventSource>();

const selected = (() => {
if (!selectedNode) {
Expand All @@ -69,16 +70,20 @@ 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]);
const resetEventSource = (eventSource: EventSource) => {
setEventSource(eventSource);
setInitialEventSource(eventSource);
};

const edited = !isEqual(eventSource, initialEventSource);

useCollectEvent('openedEventSourceDetails');

Expand All @@ -100,14 +105,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
14 changes: 9 additions & 5 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 @@ -8,6 +8,7 @@ import {Sensor} from '../../models';
import {ID} from '../event-flow/id';
import {uiUrl} from '../shared/base';
import {ErrorNotice} from '../shared/components/error-notice';
import {isEqual} from '../shared/components/object-parser';
import {Node} from '../shared/components/graph/types';
import {Loading} from '../shared/components/loading';
import {useCollectEvent} from '../shared/use-collect-event';
Expand All @@ -30,7 +31,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
const [tab, setTab] = useState<string>(queryParams.get('tab'));

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

Expand Down Expand Up @@ -59,12 +60,16 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
services.sensor
.get(name, namespace)
.then(setSensor)
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
.then(() => setEdited(false))
.then(() => setError(null))
.catch(setError);
}, [namespace, name]);

useEffect(() => setEdited(true), [sensor]);
const resetSensor = (sensor: Sensor) => {
setSensor(sensor);
setInitialSensor(sensor);
};

const edited = !isEqual(sensor, initialSensor);

useCollectEvent('openedSensorDetails');

Expand Down Expand Up @@ -94,9 +99,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
17 changes: 17 additions & 0 deletions ui/src/app/shared/components/object-parser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {isEqual} from './object-parser';

describe('isEqual', () => {
test('two objects', () => {
expect(isEqual({}, {})).toBe(true);
expect(isEqual({a: 1, b: 2}, {a: 1, b: 2})).toBe(true);
expect(isEqual({a: 1, b: 2}, {a: 1, b: 3})).toBe(false);
expect(isEqual({a: 1, b: 2}, {a: 1, c: 2})).toBe(false);
});

test('two strings', () => {
expect(isEqual('foo', 'foo')).toBe(true);
expect(isEqual('foo', 'bar')).toBe(false);
expect(isEqual('', 'bar')).toBe(false);
expect(isEqual('', '')).toBe(true);
});
});
4 changes: 4 additions & 0 deletions ui/src/app/shared/components/object-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@ export function parse<T>(value: string): T {
export function stringify<T>(value: T, type: string) {
return type === 'yaml' ? jsyaml.dump(value, {noRefs: true}) : JSON.stringify(value, null, ' ');
}

export function isEqual<T>(value1: T, value2: T) {
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
return JSON.stringify(value1) === JSON.stringify(value2);
}
16 changes: 10 additions & 6 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 {isEqual} from '../shared/components/object-parser';
import {useCollectEvent} from '../shared/use-collect-event';
import {ZeroState} from '../shared/components/zero-state';
import {Context} from '../shared/context';
Expand All @@ -36,9 +37,14 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone

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

useEffect(() => setEdited(true), [template]);
const resetTemplate = (template: WorkflowTemplate) => {
setTemplate(template);
setInitialTemplate(template);
};

const edited = !isEqual(template, initialTemplate);

useEffect(
useQueryParams(history, p => {
Expand All @@ -64,8 +70,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 +111,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