From a61d8e3c6487482a01a0e02ffb8843322f84dae1 Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Thu, 12 Sep 2024 15:48:49 -0700 Subject: [PATCH] fix(ui): Submit button grayed out on details page. Fixes #13453 The React 18 upgrade in https://github.com/argoproj/argo-workflows/pull/13069 introduced a bug on the details page where the "Submit" button is disabled immediately on page load. Specifically, I believe this is happening due to batching changes that affect the following two `useEffect()` calls, which are common to all the details pages modified in this PR: ``` 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 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 are fired separately. This PR fixes that by removing the second `useEffect()` call, which eliminates the race condition. Instead, we only call `setEdited(true)` when the editor is modified. Signed-off-by: Mason Malone --- .../cluster-workflow-template-details.tsx | 15 ++++++++----- .../cron-workflows/cron-workflow-details.tsx | 22 ++++++++++--------- .../event-sources/event-source-details.tsx | 16 +++++++++----- ui/src/app/sensors/sensor-details.tsx | 14 +++++++----- .../shared/components/object-parser.test.ts | 17 ++++++++++++++ ui/src/app/shared/components/object-parser.ts | 4 ++++ .../workflow-template-details.tsx | 16 +++++++++----- 7 files changed, 71 insertions(+), 33 deletions(-) create mode 100644 ui/src/app/shared/components/object-parser.test.ts diff --git a/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx b/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx index 1675e50ace64..196ec9114d6b 100644 --- a/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx +++ b/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx @@ -9,6 +9,7 @@ import {ClusterWorkflowTemplate} 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 {Context} from '../shared/context'; import {historyUrl} from '../shared/history'; @@ -32,7 +33,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route const [error, setError] = useState(); const [template, setTemplate] = useState(); - const [edited, setEdited] = useState(false); + const [initialTemplate, setInitialTemplate] = useState(); useEffect( useQueryParams(history, p => { @@ -42,7 +43,11 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route [history] ); - useEffect(() => setEdited(true), [template]); + const resetTemplate = (template: ClusterWorkflowTemplate) => { + setTemplate(template); + setInitialTemplate(template); + }; + const edited = !isEqual(template, initialTemplate); useEffect(() => { history.push(historyUrl('cluster-workflow-templates/{name}', {name, sidePanel, tab})); }, [name, sidePanel, tab]); @@ -51,8 +56,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); @@ -97,7 +101,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route action: () => { services.clusterWorkflowTemplate .update(template, name) - .then(setTemplate) + .then(resetTemplate) .then(() => notifications.show({ content: 'Updated', @@ -105,7 +109,6 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route }) ) .then(() => setError(null)) - .then(() => setEdited(false)) .catch(setError); } }, diff --git a/ui/src/app/cron-workflows/cron-workflow-details.tsx b/ui/src/app/cron-workflows/cron-workflow-details.tsx index 07017f23cc23..50c4c4748e5b 100644 --- a/ui/src/app/cron-workflows/cron-workflow-details.tsx +++ b/ui/src/app/cron-workflows/cron-workflow-details.tsx @@ -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'; @@ -37,7 +38,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr const [columns, setColumns] = useState([]); const [cronWorkflow, setCronWorkflow] = useState(); - const [edited, setEdited] = useState(false); + const [initialCronWorkflow, setInitialCronWorkflow] = useState(); const [error, setError] = useState(); useEffect( @@ -64,13 +65,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 () => { @@ -92,8 +97,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 @@ -104,8 +108,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 @@ -143,10 +146,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); } }, diff --git a/ui/src/app/event-sources/event-source-details.tsx b/ui/src/app/event-sources/event-source-details.tsx index 0601dbd75421..b11daa87d48d 100644 --- a/ui/src/app/event-sources/event-source-details.tsx +++ b/ui/src/app/event-sources/event-source-details.tsx @@ -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'; @@ -52,9 +53,9 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro [namespace, name, tab, selectedNode] ); - const [edited, setEdited] = useState(false); const [error, setError] = useState(); const [eventSource, setEventSource] = useState(); + const [initialEventSource, setInitialEventSource] = useState(); const selected = (() => { if (!selectedNode) { @@ -69,8 +70,7 @@ 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); @@ -78,7 +78,12 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro })(); }, [name, namespace]); - useEffect(() => setEdited(true), [eventSource]); + const resetEventSource = (eventSource: EventSource) => { + setEventSource(eventSource); + setInitialEventSource(eventSource); + }; + + const edited = !isEqual(eventSource, initialEventSource); useCollectEvent('openedEventSourceDetails'); @@ -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) }, diff --git a/ui/src/app/sensors/sensor-details.tsx b/ui/src/app/sensors/sensor-details.tsx index bbd14aef8f65..9fc7495de7d9 100644 --- a/ui/src/app/sensors/sensor-details.tsx +++ b/ui/src/app/sensors/sensor-details.tsx @@ -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'; @@ -30,7 +31,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps(queryParams.get('tab')); const [sensor, setSensor] = useState(); - const [edited, setEdited] = useState(false); + const [initialSensor, setInitialSensor] = useState(); const [selectedLogNode, setSelectedLogNode] = useState(queryParams.get('selectedLogNode')); const [error, setError] = useState(); @@ -59,12 +60,16 @@ export function SensorDetails({match, location, history}: RouteComponentProps 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'); @@ -94,9 +99,8 @@ export function SensorDetails({match, location, history}: RouteComponentProps 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) }, diff --git a/ui/src/app/shared/components/object-parser.test.ts b/ui/src/app/shared/components/object-parser.test.ts new file mode 100644 index 000000000000..2f3f235e69a7 --- /dev/null +++ b/ui/src/app/shared/components/object-parser.test.ts @@ -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); + }); +}); diff --git a/ui/src/app/shared/components/object-parser.ts b/ui/src/app/shared/components/object-parser.ts index 96f2e15d3e91..9c64f157517b 100644 --- a/ui/src/app/shared/components/object-parser.ts +++ b/ui/src/app/shared/components/object-parser.ts @@ -10,3 +10,7 @@ export function parse(value: string): T { export function stringify(value: T, type: string) { return type === 'yaml' ? jsyaml.dump(value, {noRefs: true}) : JSON.stringify(value, null, ' '); } + +export function isEqual(value1: T, value2: T) { + return JSON.stringify(value1) === JSON.stringify(value2); +} diff --git a/ui/src/app/workflow-templates/workflow-template-details.tsx b/ui/src/app/workflow-templates/workflow-template-details.tsx index 129228ee13fe..531299357944 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -9,6 +9,7 @@ import {WorkflowTemplate} 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 {Context} from '../shared/context'; import {historyUrl} from '../shared/history'; @@ -52,15 +53,19 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [error, setError] = useState(); const [template, setTemplate] = useState(); - const [edited, setEdited] = useState(false); + const [initialTemplate, setInitialTemplate] = useState(); - useEffect(() => setEdited(true), [template]); + const resetTemplate = (template: WorkflowTemplate) => { + setTemplate(template); + setInitialTemplate(template); + }; + + const edited = !isEqual(template, initialTemplate); useEffect(() => { services.workflowTemplate .get(name, namespace) - .then(setTemplate) - .then(() => setEdited(false)) // set back to false + .then(resetTemplate) .then(() => setError(null)) .catch(setError); }, [name, namespace]); @@ -91,9 +96,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) },