feat(ui): Make UI errors recoverable. Fixes #3666 #3674

alexec · 2020-08-05T00:09:31Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
~~I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.~~ N/A
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

I recommend the reviewer and I sit down and go over the changes.

Goals

The goal of this PR is to make the UI usable under error scenarios. Typically if an error occurs, we throw it and a global <ErrorBoundary/> render a generic "ops!" page. The user must then reload. This looses the user's context as well as any data entered into forms - very annoying.

Errors are now more localised in the interface. E.g. an error listing workflows just shows the error and keeps the page usable. The user can then change their search parameters. If the error was a 403, this would most useful.

Changes

Refactored handling of managed namespace in UI. Rather than have each component enforce this, it is done in <App/>. This simplifies many component.
Add automatic redirection to the correct URLs for managed namespaces.
Add malformed-*.yaml manifests to aid testing with malformed manifests.
Removed throw this.state.error and replaced with <ErrorNotice/> so that the UI remains usable on error.
<ResourceEditor/> now displays submission error in a way so that you can edit and retry.
Simplified history by adding saveHistory functions and this.url =.

Testing Notes

I'm not expecting the reviewers to do this - this is for my notes.

Does the UI stay usable?

Try listing and deep-linking malformed resources. UI should still be usable.
List each type of resource.
Creating each type of resource (both valid and malformed). Check namespace is correct.
Deleting each type of resource. Check namespace is correct.
Submitting each type of submittable resources. Check the correct namespace is used.

Can we deal with really bad errors?

Run yarn start and turn off Argo Server so that you get gateway errors, try all pages.

Does login still work?

Run in make AUTH_MODE=client start and verify that you are redirect to login page.

Does namespace preference still work?

Run in make NAMESPACED=false start clear local storage and verify no redirection occurs (you should just list the at cluster scope).
Change namespace in each page and verify that your local preference (stored in local storage) is updated.

Known Issues

The namespace preference does not affect URLs unless you reload the page. Fix to this is complex vs benefit IMHO.

alexec · 2020-08-05T00:14:53Z

ui/src/app/shared/components/resource-editor/resource-editor.tsx

@@ -15,7 +15,7 @@ interface Props<T> {
    value: T;
    readonly?: boolean;
    editing?: boolean;
-    onSubmit?: (value: T) => void;
+    onSubmit?: (value: T) => Promise<any>;


small change, big impact

alexec · 2020-08-05T00:15:12Z

ui/src/app/shared/components/namespace-filter.tsx

 import {InputFilter} from './input-filter';

-interface Props {


this component has always been a big buggy - so now it is dumb

alexec · 2020-08-05T00:15:37Z

ui/src/app/shared/services/requests.ts

        return Observable.create((observer: Observer<any>) => {
            const eventSource = new EventSource(url);
-            let opened = false;
-            eventSource.onopen = () => {


this is canonical way to do this, I hope is more robust, certainly surfaces errors correctly

alexec · 2020-08-05T17:39:18Z

manifests/quick-start/sso/dex/dex-cm.yaml

@@ -15,7 +15,7 @@ data:
    staticClients:
      - id: argo-server
        redirectURIs:
-          - https://localhost:2746/oauth2/callback
+          - http://localhost:2746/oauth2/callback


alexec · 2020-08-05T17:39:34Z

Procfile

@@ -1,2 +1,2 @@
-controller: ALWAYS_OFFLOAD_NODE_STATUS=${ALWAYS_OFFLOAD_NODE_STATUS} OFFLOAD_NODE_STATUS_TTL=30s WORKFLOW_GC_PERIOD=30s UPPERIO_DB_DEBUG=1 ARCHIVED_WORKFLOW_GC_PERIOD=30s ./dist/workflow-controller --executor-image argoproj/argoexec:${VERSION} --namespaced --loglevel ${LOG_LEVEL}
-argo-server: UPPERIO_DB_DEBUG=1 ./dist/argo --loglevel ${LOG_LEVEL} server --namespaced --auth-mode ${AUTH_MODE} --secure=$SECURE
+controller: ALWAYS_OFFLOAD_NODE_STATUS=${ALWAYS_OFFLOAD_NODE_STATUS} OFFLOAD_NODE_STATUS_TTL=30s WORKFLOW_GC_PERIOD=30s UPPERIO_DB_DEBUG=1 ARCHIVED_WORKFLOW_GC_PERIOD=30s ./dist/workflow-controller --executor-image argoproj/argoexec:${VERSION} --namespaced=${NAMESPACED} --namespace ${NAMESPACE} --loglevel ${LOG_LEVEL}


run in non-namespaced mode in dev

alexec · 2020-08-05T17:41:18Z

ui/src/app/app.tsx

@@ -110,7 +98,17 @@ export class App extends React.Component<{}, {version?: Version; popupProps: Pop

    public componentDidMount() {
        this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps}));
-        services.info.getVersion().then(version => this.setState({version}));
+        services.info


🔑 This is one of the key changes in this PR. This allows us to know what managed namespace we're running in almost immediately. We use this to create redirects that mean that child components no longer need to know about namespaces.

Great change

alexec · 2020-08-05T17:41:51Z

ui/src/app/app.tsx

+            .then(version => this.setState({version}))
+            .then(() => services.info.getInfo())
+            .then(info => this.setState({namespace: info.managedNamespace || Utils.getCurrentNamespace() || ''}))
+            .catch(error => {


I make an exception here - report this error in a popup. We still need to be able to get to the login page on 401.

alexec · 2020-08-05T17:43:13Z

ui/src/app/app.tsx

-                        <Redirect from={timelineUrl} to={uiUrl('workflows')} />
+                    <Layout navItems={navItems} version={() => <>{this.state.version ? this.state.version.version : 'unknown'}</>}>
+                        <Notifications notifications={this.notificationsManager.notifications} />
+
                        <ErrorBoundary>


move this inside the layout, so we can keep button even in case of a "ops!" error:

alexec · 2020-08-05T17:43:39Z

ui/src/app/app.tsx

+                                <Route exact={true} strict={true} path={timelineUrl}>
+                                    <Redirect to={workflowsUrl} />
+                                </Route>
+                                {this.state.namespace && (


these are the redirects, it seems you should use exact and strict if you can

alexec · 2020-08-05T17:44:02Z

ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx

@@ -20,9 +20,6 @@ import {ArchivedWorkflowFilters} from '../archived-workflow-filters/archived-wor

 interface State {
    pagination: Pagination;
-    loading: boolean;


we don't need these anymore, infer from other fields

alexec · 2020-08-05T17:44:33Z

ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx

@@ -62,42 +56,32 @@ export class ArchivedWorkflowList extends BasePage<RouteComponentProps<any>, Sta
    }

    public render() {
-        if (this.state.loading) {


move error shims lower in the dom, so more the page remains responsive, you'll see this several times in this PR

alexec · 2020-08-05T17:44:58Z

ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx

+    }
+
+    private saveHistory() {
+        this.url = uiUrl('archived-workflows/' + this.state.namespace + '?' + this.filterParams.toString());


you'll see saveHistory several times as a new pattern for - saving history

alexec · 2020-08-05T17:45:40Z

ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx

-                    loading: false
-                });
-                Utils.setCurrentNamespace(newNamespace);
+                    this.saveHistory


part of the saveHistory pattern is to invoke it after only on successful state update, again - you'll see this as a repeating pattern

alexec · 2020-08-05T17:46:04Z

...templates/components/cluster-workflow-template-details/cluster-workflow-template-details.tsx

        services.clusterWorkflowTemplate
            .get(this.name)
            .then(template => this.setState({template}))
+            .then(() => services.info.getInfo())
+            .then(info => this.setState({namespace: info.managedNamespace || Utils.getCurrentNamespace() || 'default'}))


cluster templates an the one type where we cannot infer the namespace from the URL

alexec · 2020-08-05T17:46:35Z

ui/src/app/cluster-workflow-templates/components/cluster-workflow-template-summary-panel.tsx

-                                .then(clusterWorkflowTemplate => props.onChange(clusterWorkflowTemplate))
-                                .catch(err => props.onError(err));
-                        }}
+                        onSubmit={(value: WorkflowTemplate) =>


onSubmit now requires a promise which we display in the UI

alexec · 2020-08-05T17:47:30Z

ui/src/app/shared/components/error-notice.tsx

-        {props.error.message}
+export const ErrorNotice = (props: {style?: CSSProperties; error: Error & {response?: {body: {message?: string}}}}) => (
+    <Notice style={props.style}>
+        <PhaseIcon value='Error' /> {props.error.message || 'Unknown error. Open your browser error console for more information.'}


there are rare cases where we don't get info in the error - ask the user to look in the console

alexec · 2020-08-05T17:58:33Z

...rc/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx

@@ -135,6 +133,9 @@ export class ArchivedWorkflowDetails extends BasePage<RouteComponentProps<any>,
    }

    private renderArchivedWorkflowDetails() {
+        if (this.state.error) {


alexec · 2020-08-05T17:59:18Z

...rc/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx

@@ -212,12 +213,11 @@ export class ArchivedWorkflowDetails extends BasePage<RouteComponentProps<any>,
                                },
                                spec: this.state.workflow.spec
                            }}
-                            onSubmit={(value: Workflow) => {
+                            onSubmit={(value: Workflow) =>


alexec · 2020-08-05T18:00:57Z

ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx

    }

    private renderWorkflows() {
+        if (this.state.error) {


alexec · 2020-08-05T18:04:53Z

...templates/components/cluster-workflow-template-details/cluster-workflow-template-details.tsx

@@ -102,10 +99,13 @@ export class ClusterWorkflowTemplateDetails extends BasePage<RouteComponentProps
    }

    private renderClusterWorkflowTemplate() {
+        if (this.state.error) {


alexec · 2020-08-05T18:08:04Z

ui/src/app/cron-workflows/components/cron-workflow-details/cron-workflow-details.tsx

@@ -90,12 +88,13 @@ export class CronWorkflowDetails extends BasePage<RouteComponentProps<any>, Stat
    }

    private renderCronWorkflow() {
+        if (this.state.error) {


alexec · 2020-08-05T18:08:57Z

ui/src/app/cron-workflows/components/cron-workflow-list/cron-workflow-list.tsx

    }

    private renderCronWorkflows() {
+        if (this.state.error) {


alexec · 2020-08-05T18:09:44Z

ui/src/app/cron-workflows/components/cron-workflow-summary-panel.tsx

@@ -72,12 +70,11 @@ export const CronWorkflowSummaryPanel = (props: Props) => {
                            // magic - we get the latest from the server and then apply the changes from the rendered version to this
                            const original = props.cronWorkflow;
                            const patch = jsonMergePatch.generate(original, value) || {};
-                            services.cronWorkflows
+                            return services.cronWorkflows


alexec · 2020-08-05T18:10:41Z

ui/src/app/userinfo/components/user-info.tsx

        return (
            <Page title='User Info' toolbar={{breadcrumbs: [{title: 'User Info'}]}}>
                <div className='argo-container'>
+                    {this.state.error && <ErrorNotice error={this.state.error} />}


alexec · 2020-08-05T18:11:18Z

...rc/app/workflow-templates/components/workflow-template-details/workflow-template-details.tsx

@@ -101,10 +99,13 @@ export class WorkflowTemplateDetails extends BasePage<RouteComponentProps<any>,
    }

    private renderWorkflowTemplate() {
+        if (this.state.error) {


alexec · 2020-08-05T18:12:02Z

ui/src/app/workflow-templates/components/workflow-template-list/workflow-template-list.tsx

    }

    private renderTemplates() {
+        if (this.state.error) {


alexec · 2020-08-05T18:13:27Z

ui/src/app/workflow-templates/components/workflow-template-summary-panel.tsx

@@ -34,12 +33,11 @@ export const WorkflowTemplateSummaryPanel = (props: Props) => {
                <div className='white-box__details'>
                    <ResourceEditor
                        value={props.template}
-                        onSubmit={(value: WorkflowTemplate) => {
+                        onSubmit={(value: WorkflowTemplate) =>


alexec · 2020-08-05T18:15:48Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

@@ -290,10 +282,12 @@ export class WorkflowsList extends BasePage<RouteComponentProps<any>, State> {
    }

    private renderWorkflows() {
+        if (this.state.error) {


simster7

The screenshots look great!

simster7 · 2020-08-06T18:40:03Z

ui/src/app/app.tsx

@@ -110,7 +98,17 @@ export class App extends React.Component<{}, {version?: Version; popupProps: Pop

    public componentDidMount() {
        this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps}));
-        services.info.getVersion().then(version => this.setState({version}));
+        services.info


Great change

alexec added 3 commits August 4, 2020 15:06

feat(ui): Improve error handling in UI. Fixes argoproj#3666

29d13be

Merge branch 'master' into fix-3666

970cc2f

fix-3666

034000d

alexec commented Aug 5, 2020

View reviewed changes

alexec added 7 commits August 4, 2020 17:16

fix-3666

a4c4c45

fix-3666

09321af

fix-3666

411fc46

fix-3666

4368c76

fix-3666

0ce7478

Merge branch 'master' into fix-3666

ab346eb

fix-3666

32c6fa8

alexec commented Aug 5, 2020

View reviewed changes

fix-3666

e5b1caa

alexec commented Aug 5, 2020

View reviewed changes

alexec changed the title ~~feat(ui): Improve error handling in UI. Fixes #3666~~ feat(ui): Make UI errors recoverable. Fixes #3666 Aug 5, 2020

fix-3666

55add81

alexec marked this pull request as ready for review August 5, 2020 18:20

jessesuen assigned simster7 Aug 5, 2020

simster7 approved these changes Aug 6, 2020

View reviewed changes

simster7 reviewed Aug 6, 2020

View reviewed changes

alexec merged commit 6e3c5be into argoproj:master Aug 6, 2020

alexec deleted the fix-3666 branch August 6, 2020 19:09

simster7 mentioned this pull request Aug 10, 2020

Improve expected error reporting in the UI #3210

Closed

alexec mentioned this pull request Aug 14, 2020

fix(ui): Correctly load and store namespace. Fixes #3773 and #3775 #3778

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Make UI errors recoverable. Fixes #3666 #3674

feat(ui): Make UI errors recoverable. Fixes #3666 #3674

alexec commented Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020

simster7 Aug 6, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

alexec Aug 5, 2020

simster7 left a comment

simster7 Aug 6, 2020

		import {InputFilter} from './input-filter';

		interface Props {

feat(ui): Make UI errors recoverable. Fixes #3666 #3674

feat(ui): Make UI errors recoverable. Fixes #3666 #3674

Conversation

alexec commented Aug 5, 2020 • edited Loading

Goals

Changes

Testing Notes

Known Issues

Choose a reason for hiding this comment

alexec Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020 •

edited

Loading

alexec Aug 5, 2020 •

edited

Loading