fix(ui): Submit button grayed out on details page. Fixes argoproj#13453

The React 18 upgrade in argoproj#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 <[email protected]>
MasonM · Sep 12, 2024 · a61d8e3 · a61d8e3
1 parent c551304
commit a61d8e3
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 33 deletions.
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
@@ -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<Error>();
     const [template, setTemplate] = useState<ClusterWorkflowTemplate>();
-    const [edited, setEdited] = useState(false);
+    const [initialTemplate, setInitialTemplate] = useState<ClusterWorkflowTemplate>();
 
     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,15 +101,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);
                             }
                         },

diff --git 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<models.Column[]>([]);
 
     const [cronWorkflow, setCronWorkflow] = useState<CronWorkflow>();
-    const [edited, setEdited] = useState(false);
+    const [initialCronWorkflow, setInitialCronWorkflow] = useState<CronWorkflow>();
     const [error, setError] = useState<Error>();
 
     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
@@ -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<Error>();
     const [eventSource, setEventSource] = useState<EventSource>();
+    const [initialEventSource, setInitialEventSource] = useState<EventSource>();
 
     const selected = (() => {
         if (!selectedNode) {
@@ -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');
 
@@ -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
@@ -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<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>();
 
@@ -59,12 +60,16 @@ export function SensorDetails({match, location, history}: RouteComponentProps<an
         services.sensor
             .get(name, namespace)
             .then(setSensor)
-            .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');
 
@@ -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)
                         },

diff --git a/ui/src/app/shared/components/object-parser.test.ts 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
@@ -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) {
+    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
@@ -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<Error>();
     const [template, setTemplate] = useState<WorkflowTemplate>();
-    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(() => {
         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)
                         },