fix(core): fix flickering render of stage labels on hydration (#7181)

spinnaker · Jul 8, 2019 · e26f797 · e26f797
1 parent 87e2e4f
commit e26f797
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 100 deletions.
diff --git a/app/scripts/modules/core/src/pipeline/config/stages/common/ExecutionBarLabel.tsx b/app/scripts/modules/core/src/pipeline/config/stages/common/ExecutionBarLabel.tsx
@@ -27,8 +27,13 @@ export class ExecutionBarLabel extends React.Component<IExecutionBarLabelProps,
   }
 
   private hydrate = (): void => {
-    const { execution, application } = this.props;
-    if (!execution) {
+    const { stage, application, execution } = this.props;
+    const { suspendedStageTypes } = stage;
+    const suspendedTypesNeedingHydration = ['restrictExecutionDuringTimeWindow', 'waitForCondition'];
+    const requiresHydration =
+      stage.labelComponent !== ExecutionBarLabel ||
+      suspendedTypesNeedingHydration.some(s => suspendedStageTypes.has(s));
+    if (!requiresHydration || !execution || execution.hydrated) {
       return;
     }
     ReactInjector.executionService.hydrate(application, execution).then(() => {
@@ -46,61 +51,50 @@ export class ExecutionBarLabel extends React.Component<IExecutionBarLabelProps,
     this.mounted = false;
   }
 
-  public render() {
-    const { stage, application, execution, executionMarker } = this.props;
-    const { suspendedStageTypes } = stage;
-    if (suspendedStageTypes.has('restrictExecutionDuringTimeWindow') && executionMarker) {
-      const executionWindowStage = stage.stages.find(s => s.type === 'restrictExecutionDuringTimeWindow');
-      const template = (
-        <div>
-          <div>
-            <b>{stage.name}</b> (waiting for execution window)
-          </div>
-          <ExecutionWindowActions application={application} execution={execution} stage={executionWindowStage} />
-        </div>
-      );
-      return <HoverablePopover template={template}>{this.props.children}</HoverablePopover>;
-    } else if (suspendedStageTypes.has('waitForCondition') && executionMarker) {
-      const waitForConditionStage = stage.stages.find(s => s.type === 'waitForCondition');
-      const template = (
+  private DefaultLabel = () => {
+    const { stage, application, execution } = this.props;
+    const LabelComponent = stage.labelComponent;
+    const tooltip = (
+      <Tooltip id={stage.id}>
+        <LabelComponent application={application} execution={execution} stage={stage} />
+      </Tooltip>
+    );
+    return (
+      <OverlayTrigger placement="top" overlay={tooltip}>
+        {this.props.children}
+      </OverlayTrigger>
+    );
+  };
+
+  private getExecutionWindowTemplate = () => {
+    const { stage, application, execution } = this.props;
+    const executionWindowStage = stage.stages.find(s => s.type === 'restrictExecutionDuringTimeWindow');
+    return (
+      <div>
         <div>
-          <p>
-            <b>{stage.name}</b> (waiting until conditions are met)
-          </p>
-          Conditions:
-          <SkipConditionWait application={application} execution={execution} stage={waitForConditionStage} />
+          <b>{stage.name}</b> (waiting for execution window)
         </div>
-      );
-      return <HoverablePopover template={template}>{this.props.children}</HoverablePopover>;
-    }
-    if (executionMarker) {
-      const LabelComponent = stage.labelComponent;
-      if (LabelComponent !== ExecutionBarLabel && !this.state.hydrated) {
-        const loadingTooltip = (
-          <Tooltip id={stage.id}>
-            <Spinner size="small" />
-          </Tooltip>
-        );
-        return (
-          <span onMouseEnter={this.hydrate}>
-            <OverlayTrigger placement="top" overlay={loadingTooltip}>
-              {this.props.children}
-            </OverlayTrigger>
-          </span>
-        );
-      }
-      const tooltip = (
-        <Tooltip id={stage.id}>
-          <LabelComponent application={application} execution={execution} stage={stage} />
-        </Tooltip>
-      );
-      return (
-        <OverlayTrigger placement="top" overlay={tooltip}>
-          {this.props.children}
-        </OverlayTrigger>
-      );
-    }
+        <ExecutionWindowActions application={application} execution={execution} stage={executionWindowStage} />
+      </div>
+    );
+  };
+
+  private getWaitForConditionTemplate = () => {
+    const { stage, application, execution } = this.props;
+    const waitForConditionStage = stage.stages.find(s => s.type === 'waitForCondition');
+    return (
+      <div>
+        <p>
+          <b>{stage.name}</b> (waiting until conditions are met)
+        </p>
+        Conditions:
+        <SkipConditionWait application={application} execution={execution} stage={waitForConditionStage} />
+      </div>
+    );
+  };
 
+  private getRenderableStageName(): string {
+    const { stage } = this.props;
     let stageName = stage.name ? stage.name : stage.type;
     const params = ReactInjector.$uiRouter.globals.params;
     if (stage.type === 'group' && stage.groupStages && stage.index === Number(params.stage)) {
@@ -112,6 +106,37 @@ export class ExecutionBarLabel extends React.Component<IExecutionBarLabelProps,
         }
       }
     }
-    return <span>{stageName}</span>;
+    return stageName;
+  }
+
+  public render() {
+    const { stage, execution, executionMarker } = this.props;
+    const { suspendedStageTypes } = stage;
+    const { getExecutionWindowTemplate, getWaitForConditionTemplate, DefaultLabel } = this;
+    if (executionMarker) {
+      const suspendedTypesNeedingHydration = ['restrictExecutionDuringTimeWindow', 'waitForCondition'];
+      const requiresHydration =
+        stage.labelComponent !== ExecutionBarLabel ||
+        suspendedTypesNeedingHydration.some(s => suspendedStageTypes.has(s));
+      let template: JSX.Element = null;
+      if (requiresHydration && !execution.hydrated) {
+        template = <Spinner size="small" />;
+      } else if (suspendedStageTypes.has('restrictExecutionDuringTimeWindow')) {
+        template = getExecutionWindowTemplate();
+      } else if (suspendedStageTypes.has('waitForCondition')) {
+        template = getWaitForConditionTemplate();
+      }
+      if (template) {
+        return (
+          <span onMouseEnter={this.hydrate}>
+            <HoverablePopover template={template}>{this.props.children}</HoverablePopover>
+          </span>
+        );
+      } else {
+        return <DefaultLabel />;
+      }
+    }
+
+    return <span>{this.getRenderableStageName()}</span>;
   }
 }
diff --git a/app/scripts/modules/core/src/pipeline/config/stages/concourse/concourseStage.ts b/app/scripts/modules/core/src/pipeline/config/stages/concourse/concourseStage.ts
@@ -12,7 +12,6 @@ Registry.pipeline.registerStage({
   },
   component: ConcourseStageConfig,
   executionDetailsSections: [ConcourseExecutionDetails],
-  useCustomTooltip: true,
   strategy: true,
   validators: [
     { type: 'requiredField', fieldName: 'master' },

diff --git a/app/scripts/modules/core/src/pipeline/config/stages/entityTags/applyEntityTagsStage.ts b/app/scripts/modules/core/src/pipeline/config/stages/entityTags/applyEntityTagsStage.ts
@@ -13,7 +13,6 @@ Registry.pipeline.registerStage({
   },
   component: ApplyEntityTagsStageConfig,
   executionDetailsSections: [ApplyEntityTagsExecutionDetails, ExecutionDetailsTasks],
-  useCustomTooltip: true,
   strategy: true,
   validators: [{ type: 'requiredField', fieldName: 'entityRef.entityType' }],
 });
diff --git a/...ripts/modules/core/src/pipeline/config/stages/evaluateVariables/evaluateVariablesStage.ts b/...ripts/modules/core/src/pipeline/config/stages/evaluateVariables/evaluateVariablesStage.ts
@@ -14,7 +14,6 @@ Registry.pipeline.registerStage({
   },
   component: EvaluateVariablesStageConfig,
   executionDetailsSections: [EvaluateVariablesExecutionDetails, ExecutionDetailsTasks],
-  useCustomTooltip: true,
   strategy: true,
   validators: [{ type: 'requiredField', fieldName: 'variables' }],
 });
diff --git a/app/scripts/modules/core/src/pipeline/executions/execution/ExecutionMarker.tsx b/app/scripts/modules/core/src/pipeline/executions/execution/ExecutionMarker.tsx
@@ -1,10 +1,7 @@
 import * as React from 'react';
 import * as ReactGA from 'react-ga';
-import { OverlayTrigger, Tooltip } from 'react-bootstrap';
 
 import { IExecution, IExecutionStageSummary } from 'core/domain';
-import { ReactInjector } from 'core/reactShims';
-import { Spinner } from 'core/widgets';
 import { OrchestratedItemRunningTime } from './OrchestratedItemRunningTime';
 import { duration } from 'core/utils/timeFormatters';
 
@@ -30,7 +27,6 @@ export interface IExecutionMarkerState {
 
 export class ExecutionMarker extends React.Component<IExecutionMarkerProps, IExecutionMarkerState> {
   private runningTime: OrchestratedItemRunningTime;
-  private mounted = false;
 
   constructor(props: IExecutionMarkerProps) {
     super(props);
@@ -43,17 +39,7 @@ export class ExecutionMarker extends React.Component<IExecutionMarkerProps, IExe
     };
   }
 
-  private hydrate = (): void => {
-    const { execution, application } = this.props;
-    ReactInjector.executionService.hydrate(application, execution).then(() => {
-      if (this.mounted && !this.state.hydrated) {
-        this.setState({ hydrated: true });
-      }
-    });
-  };
-
   public componentDidMount() {
-    this.mounted = true;
     this.runningTime = new OrchestratedItemRunningTime(this.props.stage, (time: number) =>
       this.setState({ duration: duration(time) }),
     );
@@ -64,7 +50,6 @@ export class ExecutionMarker extends React.Component<IExecutionMarkerProps, IExe
   }
 
   public componentWillUnmount() {
-    this.mounted = false;
     this.runningTime.reset();
   }
 
@@ -87,7 +72,7 @@ export class ExecutionMarker extends React.Component<IExecutionMarkerProps, IExe
       stage.isRunning ? 'glowing' : '',
     ].join(' ');
 
-    const TooltipComponent = stage.labelComponent;
+    const TooltipComponent = stage.useCustomTooltip ? stage.labelComponent : ExecutionBarLabel;
     const MarkerIcon = stage.markerIcon;
     const stageContents = (
       <div className={markerClassName} style={{ width, backgroundColor: stage.color }} onClick={this.handleStageClick}>
@@ -97,32 +82,10 @@ export class ExecutionMarker extends React.Component<IExecutionMarkerProps, IExe
         </span>
       </div>
     );
-    if (stage.useCustomTooltip) {
-      if (execution.hydrated) {
-        return (
-          <TooltipComponent application={application} execution={execution} stage={stage} executionMarker={true}>
-            {stageContents}
-          </TooltipComponent>
-        );
-      } else {
-        const loadingTooltip = (
-          <Tooltip id={stage.id}>
-            <Spinner size="small" />
-          </Tooltip>
-        );
-        return (
-          <span onMouseEnter={this.hydrate}>
-            <OverlayTrigger placement="top" overlay={loadingTooltip}>
-              {stageContents}
-            </OverlayTrigger>
-          </span>
-        );
-      }
-    }
     return (
-      <ExecutionBarLabel application={application} execution={execution} stage={stage} executionMarker={true}>
+      <TooltipComponent application={application} execution={execution} stage={stage} executionMarker={true}>
         {stageContents}
-      </ExecutionBarLabel>
+      </TooltipComponent>
     );
   }
 }
diff --git a/app/scripts/modules/core/src/pipeline/service/execution.service.ts b/app/scripts/modules/core/src/pipeline/service/execution.service.ts
@@ -480,7 +480,7 @@ export class ExecutionService {
    * If this method is called multiple times, only the first call performs the fetch;
    * subsequent calls will return the promise produced by the first call.
    *
-   * This is a mutating operation.
+   * This is a mutating operation - it fills the context, outputs, and tasks on the stages of the unhydrated execution.
    * @param application the application owning the execution; needed because the stupid
    *   transformExecution requires it.
    * @param unhydrated the execution to hydrate (which may already be hydrated)
@@ -495,7 +495,18 @@ export class ExecutionService {
     }
     const executionHydrator = this.getExecution(unhydrated.id).then(hydrated => {
       this.transformExecution(application, hydrated);
-      Object.assign(unhydrated, hydrated);
+      unhydrated.stages.forEach((s, i) => {
+        // stages *should* be in the same order, so getting the hydrated one by index should be fine.
+        // worth verifying, though, and, if not, find the stage by id (which makes this an O(n^2) operation instead of O(n))
+        const hydratedStage =
+          hydrated.stages[i].id === s.id ? hydrated.stages[i] : hydrated.stages.find(s2 => s.id === s2.id);
+        if (hydratedStage) {
+          s.context = hydratedStage.context;
+          s.outputs = hydratedStage.outputs;
+          s.tasks = hydratedStage.tasks;
+        }
+      });
+      unhydrated.hydrated = true;
       unhydrated.graphStatusHash = this.calculateGraphStatusHash(unhydrated);
       return unhydrated;
     });

diff --git a/app/scripts/modules/core/src/presentation/HoverablePopover.tsx b/app/scripts/modules/core/src/presentation/HoverablePopover.tsx
@@ -165,6 +165,7 @@ export class HoverablePopover extends React.Component<IHoverablePopoverProps, IH
           placement={placementOverride || placement}
           target={this.targetRef.current}
           container={container}
+          shouldUpdatePosition={true}
         >
           <PopoverOffset
             ref={this.rendererRefCallback}