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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
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
19 changes: 12 additions & 7 deletions ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ 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';
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,14 @@ 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>();

const edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]);

function resetTemplate(template: ClusterWorkflowTemplate) {
setTemplate(template);
setInitialTemplate(template);
}

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

useEffect(() => setEdited(true), [template]);
useEffect(() => {
history.push(historyUrl('cluster-workflow-templates/{name}', {name, sidePanel, tab}));
}, [name, sidePanel, tab]);
Expand All @@ -56,8 +63,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 +112,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
26 changes: 14 additions & 12 deletions ui/src/app/cron-workflows/cron-workflow-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ 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';
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,9 +37,16 @@ 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>();

const edited = useMemo(() => !isEqual(cronWorkflow, initialCronWorkflow), [cronWorkflow, initialCronWorkflow]);

function resetCronWorkflow(cronWorkflow: CronWorkflow) {
setCronWorkflow(cronWorkflow);
setInitialCronWorkflow(cronWorkflow);
}

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

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

useEffect(() => {
(async () => {
const workflowList = await services.workflows.list(namespace, null, [`${models.labels.cronWorkflow}=${name}`], {limit: 50});
Expand All @@ -91,8 +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
20 changes: 12 additions & 8 deletions ui/src/app/event-sources/event-source-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ 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';
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,16 @@ 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 edited = useMemo(() => !isEqual(eventSource, initialEventSource), [eventSource, initialEventSource]);

function resetEventSource(eventSource: EventSource) {
setEventSource(eventSource);
setInitialEventSource(eventSource);
}

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

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

useCollectEvent('openedEventSourceDetails');

return (
Expand All @@ -100,14 +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
20 changes: 12 additions & 8 deletions ui/src/app/sensors/sensor-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
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';
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,10 +31,17 @@ 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>();

const edited = useMemo(() => !isEqual(sensor, initialSensor), [sensor, initialSensor]);

function resetSensor(sensor: Sensor) {
setSensor(sensor);
setInitialSensor(sensor);
}

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

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

useCollectEvent('openedSensorDetails');

const selected = (() => {
Expand Down Expand Up @@ -94,9 +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);
});
});
5 changes: 5 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,8 @@ 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, ' ');
}

/** Use JSON.stringify() to compare given values */
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);
}
18 changes: 11 additions & 7 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 @@ -2,14 +2,15 @@ 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';
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 edited = useMemo(() => !isEqual(template, initialTemplate), [template, initialTemplate]);

function resetTemplate(template: WorkflowTemplate) {
setTemplate(template);
setInitialTemplate(template);
}

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