Merge pull request #1289 from GnsP/error-classification-ui

fix logviewer styles

Author: GnsP

app/cdap/components/LogViewer/LogRow.tsx

@@ -20,6 +20,7 @@ import { ILogResponse, LogLevel } from 'components/LogViewer/types';
 import moment from 'moment';
 import { logsTableGridStyle } from 'components/LogViewer';
 import classnames from 'classnames';
+import LoadingSVG from 'components/shared/LoadingSVG';
 
 const styles = (theme): StyleRules => {
   const tableStyle = logsTableGridStyle(theme);
@@ -43,11 +44,20 @@ const styles = (theme): StyleRules => {
 
 interface ILogRowProps extends WithStyles<typeof styles> {
   logObj: ILogResponse;
+  loading?: boolean;
 }
 
 const TIMESTAMP_FORMAT = 'L H:mm:ss';
 
-const LogRowView: React.FC<ILogRowProps> = ({ classes, logObj }) => {
+const LogRowView: React.FC<ILogRowProps> = ({ classes, logObj, loading = false }) => {
+  if (loading) {
+    return (
+      <div className={classes.root} data-cy="log-viewer-row" data-testid="log-viewer-row">
+        <LoadingSVG />
+      </div>
+    );
+  }
+
   const timeDate = new Date(logObj.log.timestamp);
   const displayTime = moment(timeDate).format(TIMESTAMP_FORMAT);
 

app/cdap/components/LogViewer/index.tsx

@@ -93,6 +93,8 @@ interface ILogViewerState {
   isPolling: boolean;
   error?: string;
   initLoading: boolean;
+  isFetchingPrev: boolean;
+  isFetchingNext: boolean;
 }
 
 const MAX_LOG_ROWS = 100;
@@ -120,6 +122,8 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
     isPolling: true,
     error: null,
     initLoading: true,
+    isFetchingPrev: false,
+    isFetchingNext: false,
   };
 
   public componentDidMount() {
@@ -270,7 +274,7 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
     if (this.state.isFetching || this.state.isPolling) {
       return;
     }
-    this.setState({ isFetching: true });
+    this.setState({ isFetching: true, isFetchingPrev: true });
 
     const logsContainer = this.logsContainer.current;
     const currentScrollHeight = logsContainer.scrollHeight;
@@ -284,6 +288,7 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
       this.setState(
         {
           isFetching: false,
+          isFetchingPrev: false,
         },
         () => {
           // maintaining scroll position
@@ -309,7 +314,7 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
     if (this.state.isFetching || this.state.isPolling) {
       return;
     }
-    this.setState({ isFetching: true });
+    this.setState({ isFetching: true, isFetchingNext: true });
 
     this.props.dataFetcher.getNext().subscribe((res) => {
       if (res.length > 0) {
@@ -320,6 +325,7 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
       this.setState(
         {
           isFetching: false,
+          isFetchingNext: false,
         },
         this.trimTopExcessLogs
       );
@@ -431,9 +437,11 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
 
     return (
       <React.Fragment>
+        {this.state.isFetchingPrev && <LogRow loading logObj={null} />}
         {this.state.logs.map((logObj, i) => {
           return <LogRow key={`${logObj.offset}-${i}`} logObj={logObj} />;
         })}
+        {this.state.isFetchingNext && <LogRow loading logObj={null} />}
       </React.Fragment>
     );
   }
@@ -448,7 +456,7 @@ class LogViewerView extends React.PureComponent<ILogViewerProps, ILogViewerState
           getLatestLogs={this.getLatestLogs}
           setSystemLogs={this.setIncludeSystemLogs}
           onClose={this.props.onClose}
-          loading={this.state.isFetching}
+          loading={this.state.initLoading}
           showStats={this.props.showStats}
         />
         <div className={classes.logsTableHeader}>

app/cdap/components/PipelineCanvasActions/ActionButtons/PipelineCanvasActionBtns.tsx

@@ -14,16 +14,20 @@
  * the License.
  */
 
+import React from 'react';
+import { Provider, useSelector } from 'react-redux';
+
 import ZoomInIcon from '@material-ui/icons/ZoomIn';
 import ZoomOutIcon from '@material-ui/icons/ZoomOut';
 import UndoIcon from '@material-ui/icons/Undo';
 import RedoIcon from '@material-ui/icons/Redo';
 import AspectRatioIcon from '@material-ui/icons/AspectRatio';
 import OpenWithIcon from '@material-ui/icons/OpenWith';
 import DragIndicatorIcon from '@material-ui/icons/DragIndicator';
-import React from 'react';
 import PipelineCommentsActionBtn from '../PipelineCommentsActionBtn';
 import { ActionButton, ActionButtonGroup, CanvasButtonTooltip } from './styles';
+import PipelineDetailStore from 'components/PipelineDetails/store';
+import { isErrorClassificationBannerDisplayed } from 'components/PipelineDetails/PipelineDetailsTopPanel/PipelineRunErrorDetails';
 
 interface IPipelineCanvasActionBtnsProps {
   setPipelineComments: () => void;
@@ -41,7 +45,7 @@ interface IPipelineCanvasActionBtnsProps {
   zoomIn: () => void;
 }
 
-export const PipelineCanvasActionBtns = ({
+export const PipelineCanvasActionBtnsView = ({
   setPipelineComments,
   pipelineComments,
   isDisabled,
@@ -56,8 +60,10 @@ export const PipelineCanvasActionBtns = ({
   zoomOut,
   zoomIn,
 }: IPipelineCanvasActionBtnsProps) => {
+  const isErrorBannerDisplayed = useSelector(isErrorClassificationBannerDisplayed);
+
   return (
-    <ActionButtonGroup>
+    <ActionButtonGroup isErrorBannerDisplayed={isErrorBannerDisplayed}>
       {!disableNodeClick && (
         <>
           <CanvasButtonTooltip title="Zoom In">
@@ -143,3 +149,9 @@ export const PipelineCanvasActionBtns = ({
     </ActionButtonGroup>
   );
 };
+
+export const PipelineCanvasActionBtns = (props: IPipelineCanvasActionBtnsProps) => (
+  <Provider store={PipelineDetailStore}>
+    <PipelineCanvasActionBtnsView {...props} />
+  </Provider>
+);

app/cdap/components/PipelineCanvasActions/ActionButtons/styles.tsx

@@ -33,7 +33,7 @@ export const ActionButtonGroup = styled.div`
   position: fixed;
   width: 37px;
   right: 15px;
-  top: 170px;
+  top: ${(props) => (props.isErrorBannerDisplayed ? '202px' : '170px')};
   vertical-align: middle;
   z-index: 998;
   > * {

app/cdap/components/PipelineDetails/PipelineDetailsTopPanel/PipelineRunErrorDetails/ErrorDetailsBanner.tsx

@@ -18,6 +18,7 @@ import React, { useEffect, useState } from 'react';
 import T from 'i18n-react';
 import { useSelector, Provider, useDispatch } from 'react-redux';
 import styled from 'styled-components';
+import _uniq from 'lodash/uniq';
 
 import LaunchIcon from '@material-ui/icons/Launch';
 import GetAppIcon from '@material-ui/icons/GetApp';
@@ -232,7 +233,7 @@ function ErrorDetailsBannerView({
           <ShortErrorMessage>
             <PipelineErrorCountMessage
               classifiedErrorCount={errorDetails?.length}
-              stages={errorDetails?.map((err) => err.stageName)}
+              stages={_uniq(errorDetails?.map((err) => err.stageName)?.filter(Boolean) || [])}
             />
             {canExapndErrorDetails ? (
               <Button
@@ -335,7 +336,7 @@ function ErrorDetailsBannerView({
           />
         ) : (
           <Provider store={PipelineDetailStore}>
-            <PipelineLogViewer toggleLogViewer={toggleLogs} withErrorBanner={true} />
+            <PipelineLogViewer toggleLogViewer={toggleLogs} />
           </Provider>
         ))}
     </ThemeWrapper>

app/cdap/components/PipelineDetails/PipelineDetailsTopPanel/PipelineRunErrorDetails/index.tsx

@@ -17,7 +17,7 @@
 import React, { useEffect } from 'react';
 import { useSelector, useDispatch } from 'react-redux';
 
-import { ACTIONS } from '../../store';
+import PipelineDetailStore, { ACTIONS } from '../../store';
 import { MyPipelineApi } from 'api/pipeline';
 import { getCurrentNamespace } from 'services/NamespaceStore';
 import ProgramDataFetcher from 'components/LogViewer/DataFetcher/ProgramDataFetcher';
@@ -27,6 +27,13 @@ import { IErrorEntry } from './types';
 import { RETRY_DELAY_MS } from './constants';
 import ErrorDetailsBanner from './ErrorDetailsBanner';
 
+export function isErrorClassificationBannerDisplayed(pipelineDetailsState) {
+  const { currentRun, runErrorDetailsLoading } = pipelineDetailsState;
+  const runid = currentRun?.runid;
+  const loading = runErrorDetailsLoading[runid] || false;
+  return currentRun?.status === 'FAILED' && !loading;
+}
+
 export default function PipelineRunErrorDetails() {
   const appId = useSelector((state) => state.name);
   const artifactName = useSelector((state) => state.artifact.name);

app/cdap/components/PipelineDetails/RunLevelInfo/PipelineLogViewer/index.tsx

@@ -26,42 +26,38 @@ import { GLOBALS } from 'services/global-constants';
 import ProgramDataFetcher from 'components/LogViewer/DataFetcher/ProgramDataFetcher';
 import LogViewer from 'components/LogViewer';
 import { PIPELINE_LOGS_FILTER } from 'services/global-constants';
+import { isErrorClassificationBannerDisplayed } from 'components/PipelineDetails/PipelineDetailsTopPanel/PipelineRunErrorDetails';
 
-const PIPELINE_TOP_PANEL_OFFSET = '160px';
+const LOGVIEWER_TOP_OFFSET = '110px';
 const FOOTER_HEIGHT = '54px';
-
-const ERROR_BANNER_OFFSET = '20px'; // to align the top of the logs viewer when the error banner is present
-const LOGS_PORTAL_OFFSET = '50px'; // to remove the unnecessary page scroll
+const HEADER_HEIGHT = '48px';
+const ERROR_BANNER_OFFSET = '32px'; // to align the top of the logs viewer when the error banner is present
 
 const styles = (theme): StyleRules => {
   const portalContainerBase = {
-    position: 'absolute',
-    top: 0,
+    position: 'fixed',
+    top: HEADER_HEIGHT,
     left: 0,
-    height: '100vh',
+    height: `calc(100vh - ${HEADER_HEIGHT} - ${FOOTER_HEIGHT})`,
     width: '100vw',
     zIndex: 1301,
   };
 
   const logsContainerBase = {
     position: 'absolute',
-    top: PIPELINE_TOP_PANEL_OFFSET,
-    height: `calc(100% - ${PIPELINE_TOP_PANEL_OFFSET} - ${FOOTER_HEIGHT})`,
+    top: LOGVIEWER_TOP_OFFSET,
+    height: `calc(100% - ${LOGVIEWER_TOP_OFFSET})`,
     width: '100%',
     backgroundColor: theme.palette.white[50],
   };
 
   return {
     portalContainer: portalContainerBase as CreateCSSProperties<{}>,
     logsContainer: logsContainerBase as CreateCSSProperties<{}>,
-    portalContainerWithErrorBanner: {
-      ...portalContainerBase,
-      height: `calc(100vh - ${LOGS_PORTAL_OFFSET})`,
-    } as CreateCSSProperties<{}>,
     logsContainerWithErrorBanner: {
       ...logsContainerBase,
-      height: `calc(100% - ${PIPELINE_TOP_PANEL_OFFSET} - ${FOOTER_HEIGHT} - ${ERROR_BANNER_OFFSET})`,
-      top: `calc(${PIPELINE_TOP_PANEL_OFFSET} - ${ERROR_BANNER_OFFSET})`,
+      height: `calc(100% - ${LOGVIEWER_TOP_OFFSET} - ${ERROR_BANNER_OFFSET})`,
+      top: `calc(${LOGVIEWER_TOP_OFFSET} + ${ERROR_BANNER_OFFSET})`,
     } as CreateCSSProperties<{}>,
   };
 };
@@ -107,11 +103,7 @@ const LogViewerContainer: React.FC<ILogViewerProps> = ({
   }
 
   return (
-    <div
-      className={withErrorBanner ? classes.portalContainerWithErrorBanner : classes.portalContainer}
-      ref={backgroundElem}
-      onClick={handleBackgroundClick}
-    >
+    <div className={classes.portalContainer} ref={backgroundElem} onClick={handleBackgroundClick}>
       <div
         className={withErrorBanner ? classes.logsContainerWithErrorBanner : classes.logsContainer}
       >
@@ -128,6 +120,7 @@ const mapStateToProps = (state) => {
     currentRun: state.currentRun,
     appId: state.name,
     artifactName: state.artifact.name,
+    withErrorBanner: isErrorClassificationBannerDisplayed(state),
   };
 };