From fe7d6be6fa616791b7531008c790341102a07139 Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Thu, 12 Sep 2024 15:48:49 -0700 Subject: [PATCH 1/5] 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 2cda172f8dab..97d1318e5406 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 @@ -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'; @@ -37,7 +38,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 => { @@ -47,7 +48,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]); @@ -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); @@ -106,7 +110,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route action: () => { services.clusterWorkflowTemplate .update(template, name) - .then(setTemplate) + .then(resetTemplate) .then(() => notifications.show({ content: 'Updated', @@ -114,7 +118,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 36a4986c39e6..96d41fff0ad0 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'; @@ -36,7 +37,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( @@ -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 () => { @@ -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 @@ -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 @@ -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); } }, 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 89381addb00c..abd1c9f2e8a1 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -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'; @@ -36,9 +37,14 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [template, setTemplate] = useState(); const [error, setError] = 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( useQueryParams(history, p => { @@ -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]); @@ -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) }, From 2747d1755c11b49749fb6780bcb2a46f2f4b81e4 Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Sat, 14 Sep 2024 11:06:50 -0700 Subject: [PATCH 2/5] fix(ui): stylistic fixes Signed-off-by: Mason Malone --- .../cluster-workflow-template-details.tsx | 12 +++++++----- .../app/cron-workflows/cron-workflow-details.tsx | 14 +++++++------- ui/src/app/event-sources/event-source-details.tsx | 14 +++++++------- ui/src/app/sensors/sensor-details.tsx | 14 +++++++------- ui/src/app/shared/components/object-parser.ts | 1 + .../workflow-template-details.tsx | 8 ++++---- 6 files changed, 33 insertions(+), 30 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 index 97d1318e5406..08afb7b2aaa1 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 @@ -40,6 +40,13 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route const [template, setTemplate] = useState(); const [initialTemplate, setInitialTemplate] = useState(); + const edited = !isEqual(template, initialTemplate); + + function resetTemplate(template: ClusterWorkflowTemplate) { + setTemplate(template); + setInitialTemplate(template); + } + useEffect( useQueryParams(history, p => { setSidePanel(p.get('sidePanel') === 'true'); @@ -48,11 +55,6 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route [history] ); - 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]); diff --git a/ui/src/app/cron-workflows/cron-workflow-details.tsx b/ui/src/app/cron-workflows/cron-workflow-details.tsx index 96d41fff0ad0..4020e090152c 100644 --- a/ui/src/app/cron-workflows/cron-workflow-details.tsx +++ b/ui/src/app/cron-workflows/cron-workflow-details.tsx @@ -40,6 +40,13 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr const [initialCronWorkflow, setInitialCronWorkflow] = useState(); const [error, setError] = useState(); + const edited = !isEqual(cronWorkflow, initialCronWorkflow); + + function resetCronWorkflow(cronWorkflow: CronWorkflow) { + setCronWorkflow(cronWorkflow); + setInitialCronWorkflow(cronWorkflow); + } + useEffect( useQueryParams(history, p => { setSidePanel(p.get('sidePanel')); @@ -69,13 +76,6 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr .catch(setError); }, [namespace, name]); - const resetCronWorkflow = (cronWorkflow: CronWorkflow) => { - setCronWorkflow(cronWorkflow); - setInitialCronWorkflow(cronWorkflow); - }; - - const edited = !isEqual(cronWorkflow, initialCronWorkflow); - useEffect(() => { (async () => { const workflowList = await services.workflows.list(namespace, null, [`${models.labels.cronWorkflow}=${name}`], {limit: 50}); diff --git a/ui/src/app/event-sources/event-source-details.tsx b/ui/src/app/event-sources/event-source-details.tsx index b11daa87d48d..194287ddf699 100644 --- a/ui/src/app/event-sources/event-source-details.tsx +++ b/ui/src/app/event-sources/event-source-details.tsx @@ -57,6 +57,13 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro const [eventSource, setEventSource] = useState(); const [initialEventSource, setInitialEventSource] = useState(); + const edited = !isEqual(eventSource, initialEventSource); + + function resetEventSource(eventSource: EventSource) { + setEventSource(eventSource); + setInitialEventSource(eventSource); + } + const selected = (() => { if (!selectedNode) { return; @@ -78,13 +85,6 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro })(); }, [name, namespace]); - const resetEventSource = (eventSource: EventSource) => { - setEventSource(eventSource); - setInitialEventSource(eventSource); - }; - - const edited = !isEqual(eventSource, initialEventSource); - useCollectEvent('openedEventSourceDetails'); return ( diff --git a/ui/src/app/sensors/sensor-details.tsx b/ui/src/app/sensors/sensor-details.tsx index 9fc7495de7d9..2dcd29d89883 100644 --- a/ui/src/app/sensors/sensor-details.tsx +++ b/ui/src/app/sensors/sensor-details.tsx @@ -35,6 +35,13 @@ export function SensorDetails({match, location, history}: RouteComponentProps(queryParams.get('selectedLogNode')); const [error, setError] = useState(); + const edited = !isEqual(sensor, initialSensor); + + function resetSensor(sensor: Sensor) { + setSensor(sensor); + setInitialSensor(sensor); + } + useEffect( useQueryParams(history, p => { setTab(p.get('tab')); @@ -64,13 +71,6 @@ export function SensorDetails({match, location, history}: RouteComponentProps { - setSensor(sensor); - setInitialSensor(sensor); - }; - - const edited = !isEqual(sensor, initialSensor); - useCollectEvent('openedSensorDetails'); const selected = (() => { diff --git a/ui/src/app/shared/components/object-parser.ts b/ui/src/app/shared/components/object-parser.ts index 9c64f157517b..1c2b09456986 100644 --- a/ui/src/app/shared/components/object-parser.ts +++ b/ui/src/app/shared/components/object-parser.ts @@ -11,6 +11,7 @@ export function stringify(value: T, type: string) { return type === 'yaml' ? jsyaml.dump(value, {noRefs: true}) : JSON.stringify(value, null, ' '); } +/** Use JSON.stringify() to compare given values */ 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 abd1c9f2e8a1..2cb3fbc566b5 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -39,12 +39,12 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [error, setError] = useState(); const [initialTemplate, setInitialTemplate] = useState(); - const resetTemplate = (template: WorkflowTemplate) => { + const edited = !isEqual(template, initialTemplate); + + function resetTemplate(template: WorkflowTemplate) { setTemplate(template); setInitialTemplate(template); - }; - - const edited = !isEqual(template, initialTemplate); + } useEffect( useQueryParams(history, p => { From 1ef4958e4469e8ebd41719a81a45201385e7e476 Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Sat, 14 Sep 2024 11:14:33 -0700 Subject: [PATCH 3/5] fix(ui): cache using useMemo() and fix sensor-details.tsx Signed-off-by: Mason Malone --- .../cluster-workflow-template-details.tsx | 4 ++-- ui/src/app/cron-workflows/cron-workflow-details.tsx | 4 ++-- ui/src/app/event-sources/event-source-details.tsx | 4 ++-- ui/src/app/sensors/sensor-details.tsx | 6 +++--- ui/src/app/workflow-templates/workflow-template-details.tsx | 4 ++-- 5 files changed, 11 insertions(+), 11 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 index 08afb7b2aaa1..f673878f7331 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 @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; +import {useContext, useEffect, useMemo, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -40,7 +40,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route const [template, setTemplate] = useState(); const [initialTemplate, setInitialTemplate] = useState(); - const edited = !isEqual(template, initialTemplate); + const edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]); function resetTemplate(template: ClusterWorkflowTemplate) { setTemplate(template); diff --git a/ui/src/app/cron-workflows/cron-workflow-details.tsx b/ui/src/app/cron-workflows/cron-workflow-details.tsx index 4020e090152c..8ba7e98ea441 100644 --- a/ui/src/app/cron-workflows/cron-workflow-details.tsx +++ b/ui/src/app/cron-workflows/cron-workflow-details.tsx @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; +import {useContext, useEffect, useMemo, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -40,7 +40,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr const [initialCronWorkflow, setInitialCronWorkflow] = useState(); const [error, setError] = useState(); - const edited = !isEqual(cronWorkflow, initialCronWorkflow); + const edited = useMemo(() => !isEqual(cronWorkflow, initialCronWorkflow), [cronWorkflow, initialCronWorkflow]); function resetCronWorkflow(cronWorkflow: CronWorkflow) { setCronWorkflow(cronWorkflow); diff --git a/ui/src/app/event-sources/event-source-details.tsx b/ui/src/app/event-sources/event-source-details.tsx index 194287ddf699..a3af26951db9 100644 --- a/ui/src/app/event-sources/event-source-details.tsx +++ b/ui/src/app/event-sources/event-source-details.tsx @@ -3,7 +3,7 @@ import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import {Tabs} from 'argo-ui/src/components/tabs/tabs'; import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; +import {useContext, useEffect, useMemo, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import {EventSource} from '../../models'; @@ -57,7 +57,7 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro const [eventSource, setEventSource] = useState(); const [initialEventSource, setInitialEventSource] = useState(); - const edited = !isEqual(eventSource, initialEventSource); + const edited = useMemo(() => !isEqual(eventSource, initialEventSource), [eventSource, initialEventSource]); function resetEventSource(eventSource: EventSource) { setEventSource(eventSource); diff --git a/ui/src/app/sensors/sensor-details.tsx b/ui/src/app/sensors/sensor-details.tsx index 2dcd29d89883..26a91149a9c8 100644 --- a/ui/src/app/sensors/sensor-details.tsx +++ b/ui/src/app/sensors/sensor-details.tsx @@ -1,7 +1,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notifications'; import {Page} from 'argo-ui/src/components/page/page'; import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; +import {useContext, useEffect, useMemo, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import {Sensor} from '../../models'; @@ -35,7 +35,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps(queryParams.get('selectedLogNode')); const [error, setError] = useState(); - const edited = !isEqual(sensor, initialSensor); + const edited = useMemo(() => !isEqual(sensor, initialSensor), [sensor, initialSensor]); function resetSensor(sensor: Sensor) { setSensor(sensor); @@ -66,7 +66,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps { services.sensor .get(name, namespace) - .then(setSensor) + .then(resetSensor) .then(() => setError(null)) .catch(setError); }, [namespace, name]); diff --git a/ui/src/app/workflow-templates/workflow-template-details.tsx b/ui/src/app/workflow-templates/workflow-template-details.tsx index 2cb3fbc566b5..029aa37aedcd 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; +import {useContext, useEffect, useMemo, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -39,7 +39,7 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [error, setError] = useState(); const [initialTemplate, setInitialTemplate] = useState(); - const edited = !isEqual(template, initialTemplate); + const edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]); function resetTemplate(template: WorkflowTemplate) { setTemplate(template); From 24c0206b9071503467034cb67716ef0b27158b0f Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Sat, 28 Sep 2024 10:21:47 -0700 Subject: [PATCH 4/5] refactor(ui): Extract to hook and delete deep Object comparison Signed-off-by: Mason Malone --- .../cluster-workflow-template-details.tsx | 14 +++--------- .../cron-workflows/cron-workflow-details.tsx | 14 +++--------- .../event-sources/event-source-details.tsx | 14 +++--------- ui/src/app/sensors/sensor-details.tsx | 14 +++--------- .../shared/components/object-parser.test.ts | 17 -------------- ui/src/app/shared/components/object-parser.ts | 5 ----- ui/src/app/shared/use-editable-resource.ts | 22 +++++++++++++++++++ .../workflow-template-details.tsx | 14 +++--------- 8 files changed, 37 insertions(+), 77 deletions(-) delete mode 100644 ui/src/app/shared/components/object-parser.test.ts create mode 100644 ui/src/app/shared/use-editable-resource.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 15046f502f9c..b2dd6dbb92e1 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 @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useMemo, useState} from 'react'; +import {useContext, useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -10,12 +10,12 @@ 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'; 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'; @@ -37,15 +37,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route const [columns, setColumns] = useState([]); const [error, setError] = useState(); - const [template, setTemplate] = useState(); - const [initialTemplate, setInitialTemplate] = useState(); - - const edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]); - - function resetTemplate(template: ClusterWorkflowTemplate) { - setTemplate(template); - setInitialTemplate(template); - } + const [template, edited, setTemplate, resetTemplate] = useEditableResource(); useEffect( useQueryParams(history, p => { diff --git a/ui/src/app/cron-workflows/cron-workflow-details.tsx b/ui/src/app/cron-workflows/cron-workflow-details.tsx index 8ba7e98ea441..be884d0c0d3c 100644 --- a/ui/src/app/cron-workflows/cron-workflow-details.tsx +++ b/ui/src/app/cron-workflows/cron-workflow-details.tsx @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useMemo, useState} from 'react'; +import {useContext, useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -10,7 +10,6 @@ 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'; @@ -18,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'; @@ -36,17 +36,9 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr const [workflows, setWorkflows] = useState([]); const [columns, setColumns] = useState([]); - const [cronWorkflow, setCronWorkflow] = useState(); - const [initialCronWorkflow, setInitialCronWorkflow] = useState(); + const [cronWorkflow, edited, setCronWorkflow, resetCronWorkflow] = useEditableResource(); const [error, setError] = useState(); - const edited = useMemo(() => !isEqual(cronWorkflow, initialCronWorkflow), [cronWorkflow, initialCronWorkflow]); - - function resetCronWorkflow(cronWorkflow: CronWorkflow) { - setCronWorkflow(cronWorkflow); - setInitialCronWorkflow(cronWorkflow); - } - useEffect( useQueryParams(history, p => { setSidePanel(p.get('sidePanel')); diff --git a/ui/src/app/event-sources/event-source-details.tsx b/ui/src/app/event-sources/event-source-details.tsx index a3af26951db9..9f4d4db56084 100644 --- a/ui/src/app/event-sources/event-source-details.tsx +++ b/ui/src/app/event-sources/event-source-details.tsx @@ -3,7 +3,7 @@ import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import {Tabs} from 'argo-ui/src/components/tabs/tabs'; import * as React from 'react'; -import {useContext, useEffect, useMemo, useState} from 'react'; +import {useContext, useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import {EventSource} from '../../models'; @@ -11,12 +11,12 @@ 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'; 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'; @@ -54,15 +54,7 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro ); const [error, setError] = useState(); - const [eventSource, setEventSource] = useState(); - const [initialEventSource, setInitialEventSource] = useState(); - - const edited = useMemo(() => !isEqual(eventSource, initialEventSource), [eventSource, initialEventSource]); - - function resetEventSource(eventSource: EventSource) { - setEventSource(eventSource); - setInitialEventSource(eventSource); - } + const [eventSource, edited, setEventSource, resetEventSource] = useEditableResource(); const selected = (() => { if (!selectedNode) { diff --git a/ui/src/app/sensors/sensor-details.tsx b/ui/src/app/sensors/sensor-details.tsx index 26a91149a9c8..f9a272df1980 100644 --- a/ui/src/app/sensors/sensor-details.tsx +++ b/ui/src/app/sensors/sensor-details.tsx @@ -1,14 +1,13 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notifications'; import {Page} from 'argo-ui/src/components/page/page'; import * as React from 'react'; -import {useContext, useEffect, useMemo, useState} from 'react'; +import {useContext, useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; 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'; @@ -16,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'; @@ -30,18 +30,10 @@ export function SensorDetails({match, location, history}: RouteComponentProps(queryParams.get('tab')); - const [sensor, setSensor] = useState(); - const [initialSensor, setInitialSensor] = useState(); + const [sensor, edited, setSensor, resetSensor] = useEditableResource(); const [selectedLogNode, setSelectedLogNode] = useState(queryParams.get('selectedLogNode')); const [error, setError] = useState(); - const edited = useMemo(() => !isEqual(sensor, initialSensor), [sensor, initialSensor]); - - function resetSensor(sensor: Sensor) { - setSensor(sensor); - setInitialSensor(sensor); - } - useEffect( useQueryParams(history, p => { setTab(p.get('tab')); diff --git a/ui/src/app/shared/components/object-parser.test.ts b/ui/src/app/shared/components/object-parser.test.ts deleted file mode 100644 index 2f3f235e69a7..000000000000 --- a/ui/src/app/shared/components/object-parser.test.ts +++ /dev/null @@ -1,17 +0,0 @@ -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 1c2b09456986..96f2e15d3e91 100644 --- a/ui/src/app/shared/components/object-parser.ts +++ b/ui/src/app/shared/components/object-parser.ts @@ -10,8 +10,3 @@ 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, ' '); } - -/** Use JSON.stringify() to compare given values */ -export function isEqual(value1: T, value2: T) { - return JSON.stringify(value1) === JSON.stringify(value2); -} diff --git a/ui/src/app/shared/use-editable-resource.ts b/ui/src/app/shared/use-editable-resource.ts new file mode 100644 index 000000000000..9e9252150f72 --- /dev/null +++ b/ui/src/app/shared/use-editable-resource.ts @@ -0,0 +1,22 @@ +import {useState} from 'react'; + +/** + * useEditableResource is a hook to manage the state of a resource that be edited and updated. + */ +export function useEditableResource(initial?: T): [T, boolean, React.Dispatch, (value: T) => void] { + const [value, setValue] = useState(initial); + const [initialValue, setInitialValue] = useState(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. + const edited = value !== initialValue; + + function resetValue(value: T) { + setValue(value); + setInitialValue(value); + } + + return [value, edited, setValue, resetValue]; +} diff --git a/ui/src/app/workflow-templates/workflow-template-details.tsx b/ui/src/app/workflow-templates/workflow-template-details.tsx index 029aa37aedcd..74f2b92a5e6d 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -2,7 +2,7 @@ import {NotificationType} from 'argo-ui/src/components/notifications/notificatio import {Page} from 'argo-ui/src/components/page/page'; import {SlidingPanel} from 'argo-ui/src/components/sliding-panel/sliding-panel'; import * as React from 'react'; -import {useContext, useEffect, useMemo, useState} from 'react'; +import {useContext, useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import * as models from '../../models'; @@ -10,7 +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 {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'; @@ -35,16 +35,8 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [workflows, setWorkflows] = useState([]); const [columns, setColumns] = useState([]); - const [template, setTemplate] = useState(); + const [template, edited, setTemplate, resetTemplate] = useEditableResource(); const [error, setError] = useState(); - const [initialTemplate, setInitialTemplate] = useState(); - - const edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]); - - function resetTemplate(template: WorkflowTemplate) { - setTemplate(template); - setInitialTemplate(template); - } useEffect( useQueryParams(history, p => { From cf9a3f383d51dc74fc6e8edd0f86b9f413e14d52 Mon Sep 17 00:00:00 2001 From: Mason Malone Date: Mon, 30 Sep 2024 09:24:03 -0700 Subject: [PATCH 5/5] refactor: rename to useEditableObject and update comments Signed-off-by: Mason Malone --- .../cluster-workflow-template-details.tsx | 4 ++-- .../cron-workflows/cron-workflow-details.tsx | 4 ++-- .../event-sources/event-source-details.tsx | 4 ++-- ui/src/app/sensors/sensor-details.tsx | 4 ++-- ui/src/app/shared/use-editable-object.ts | 21 ++++++++++++++++++ ui/src/app/shared/use-editable-resource.ts | 22 ------------------- .../workflow-template-details.tsx | 4 ++-- 7 files changed, 31 insertions(+), 32 deletions(-) create mode 100644 ui/src/app/shared/use-editable-object.ts delete mode 100644 ui/src/app/shared/use-editable-resource.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 b2dd6dbb92e1..3523474acfc6 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 @@ -15,7 +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 {useEditableObject} from '../shared/use-editable-object'; import {useQueryParams} from '../shared/use-query-params'; import * as nsUtils from '../shared/namespaces'; import {WorkflowDetailsList} from '../workflows/components/workflow-details-list/workflow-details-list'; @@ -37,7 +37,7 @@ export function ClusterWorkflowTemplateDetails({history, location, match}: Route const [columns, setColumns] = useState([]); const [error, setError] = useState(); - const [template, edited, setTemplate, resetTemplate] = useEditableResource(); + const [template, edited, setTemplate, resetTemplate] = useEditableObject(); useEffect( useQueryParams(history, p => { diff --git a/ui/src/app/cron-workflows/cron-workflow-details.tsx b/ui/src/app/cron-workflows/cron-workflow-details.tsx index be884d0c0d3c..3552c902063f 100644 --- a/ui/src/app/cron-workflows/cron-workflow-details.tsx +++ b/ui/src/app/cron-workflows/cron-workflow-details.tsx @@ -17,7 +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 {useEditableObject} from '../shared/use-editable-object'; import {WidgetGallery} from '../widgets/widget-gallery'; import {WorkflowDetailsList} from '../workflows/components/workflow-details-list/workflow-details-list'; import {CronWorkflowEditor} from './cron-workflow-editor'; @@ -36,7 +36,7 @@ export function CronWorkflowDetails({match, location, history}: RouteComponentPr const [workflows, setWorkflows] = useState([]); const [columns, setColumns] = useState([]); - const [cronWorkflow, edited, setCronWorkflow, resetCronWorkflow] = useEditableResource(); + const [cronWorkflow, edited, setCronWorkflow, resetCronWorkflow] = useEditableObject(); const [error, setError] = useState(); useEffect( diff --git a/ui/src/app/event-sources/event-source-details.tsx b/ui/src/app/event-sources/event-source-details.tsx index 9f4d4db56084..42937fccb90d 100644 --- a/ui/src/app/event-sources/event-source-details.tsx +++ b/ui/src/app/event-sources/event-source-details.tsx @@ -16,7 +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 {useEditableObject} from '../shared/use-editable-object'; import {EventsPanel} from '../workflows/components/events-panel'; import {EventSourceEditor} from './event-source-editor'; import {EventSourceLogsViewer} from './event-source-log-viewer'; @@ -54,7 +54,7 @@ export function EventSourceDetails({history, location, match}: RouteComponentPro ); const [error, setError] = useState(); - const [eventSource, edited, setEventSource, resetEventSource] = useEditableResource(); + const [eventSource, edited, setEventSource, resetEventSource] = useEditableObject(); const selected = (() => { if (!selectedNode) { diff --git a/ui/src/app/sensors/sensor-details.tsx b/ui/src/app/sensors/sensor-details.tsx index f9a272df1980..c4ddf7b3f00c 100644 --- a/ui/src/app/sensors/sensor-details.tsx +++ b/ui/src/app/sensors/sensor-details.tsx @@ -15,7 +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 {useEditableObject} from '../shared/use-editable-object'; import {SensorEditor} from './sensor-editor'; import {SensorSidePanel} from './sensor-side-panel'; @@ -30,7 +30,7 @@ export function SensorDetails({match, location, history}: RouteComponentProps(queryParams.get('tab')); - const [sensor, edited, setSensor, resetSensor] = useEditableResource(); + const [sensor, edited, setSensor, resetSensor] = useEditableObject(); const [selectedLogNode, setSelectedLogNode] = useState(queryParams.get('selectedLogNode')); const [error, setError] = useState(); diff --git a/ui/src/app/shared/use-editable-object.ts b/ui/src/app/shared/use-editable-object.ts new file mode 100644 index 000000000000..bb18287af8a8 --- /dev/null +++ b/ui/src/app/shared/use-editable-object.ts @@ -0,0 +1,21 @@ +import {useState} from 'react'; + +/** + * useEditableObject is a React hook to manage the state of object that can be edited and updated. + * Uses ref comparisons to determine whether the resource has been edited. + */ +export function useEditableObject(initial?: T): [T, boolean, React.Dispatch, (value: T) => void] { + const [value, setValue] = useState(initial); + const [initialValue, setInitialValue] = useState(initial); + + // Note: This is a pure reference comparison instead of a deep comparison for performance + // reasons, since changes are latency-sensitive. + const edited = value !== initialValue; + + function resetValue(value: T) { + setValue(value); + setInitialValue(value); + } + + return [value, edited, setValue, resetValue]; +} diff --git a/ui/src/app/shared/use-editable-resource.ts b/ui/src/app/shared/use-editable-resource.ts deleted file mode 100644 index 9e9252150f72..000000000000 --- a/ui/src/app/shared/use-editable-resource.ts +++ /dev/null @@ -1,22 +0,0 @@ -import {useState} from 'react'; - -/** - * useEditableResource is a hook to manage the state of a resource that be edited and updated. - */ -export function useEditableResource(initial?: T): [T, boolean, React.Dispatch, (value: T) => void] { - const [value, setValue] = useState(initial); - const [initialValue, setInitialValue] = useState(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. - const edited = value !== initialValue; - - function resetValue(value: T) { - setValue(value); - setInitialValue(value); - } - - return [value, edited, setValue, resetValue]; -} diff --git a/ui/src/app/workflow-templates/workflow-template-details.tsx b/ui/src/app/workflow-templates/workflow-template-details.tsx index 74f2b92a5e6d..43114cf2d031 100644 --- a/ui/src/app/workflow-templates/workflow-template-details.tsx +++ b/ui/src/app/workflow-templates/workflow-template-details.tsx @@ -10,7 +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 {useEditableObject} from '../shared/use-editable-object'; import {useCollectEvent} from '../shared/use-collect-event'; import {ZeroState} from '../shared/components/zero-state'; import {Context} from '../shared/context'; @@ -35,7 +35,7 @@ export function WorkflowTemplateDetails({history, location, match}: RouteCompone const [workflows, setWorkflows] = useState([]); const [columns, setColumns] = useState([]); - const [template, edited, setTemplate, resetTemplate] = useEditableResource(); + const [template, edited, setTemplate, resetTemplate] = useEditableObject(); const [error, setError] = useState(); useEffect(