From 279d1b570bb5c70febafb9ab33d56d53a5cb2486 Mon Sep 17 00:00:00 2001
From: Andrew Azores <aazores@redhat.com>
Date: Mon, 6 Feb 2023 19:06:45 -0500
Subject: [PATCH] cleanup, fix bug with no-recording target selection

---
 src/app/Dashboard/Charts/ChartCard.tsx       |  84 +++++----------
 src/app/Dashboard/Charts/ChartContext.tsx    |   8 +-
 src/app/Dashboard/Charts/ChartController.tsx | 104 ++++++++++---------
 3 files changed, 85 insertions(+), 111 deletions(-)

diff --git a/src/app/Dashboard/Charts/ChartCard.tsx b/src/app/Dashboard/Charts/ChartCard.tsx
index 004a3b3899..5cf56bd6af 100644
--- a/src/app/Dashboard/Charts/ChartCard.tsx
+++ b/src/app/Dashboard/Charts/ChartCard.tsx
@@ -52,14 +52,14 @@ import {
   EmptyStateVariant,
   Title,
 } from '@patternfly/react-core';
-import { ExternalLinkAltIcon, PlusCircleIcon, RedoIcon } from '@patternfly/react-icons';
+import { ExternalLinkAltIcon, PlusCircleIcon } from '@patternfly/react-icons';
 import * as React from 'react';
 import { useHistory } from 'react-router-dom';
-import { first, interval, timer } from 'rxjs';
+import { interval } from 'rxjs';
 import { DashboardCardDescriptor, DashboardCardProps, DashboardCardSizes } from '../Dashboard';
 import { DashboardCard } from '../DashboardCard';
 import { ChartContext } from './ChartContext';
-import { MIN_REFRESH, RECORDING_NAME } from './ChartController';
+import { RECORDING_NAME } from './ChartController';
 
 export interface ChartCardProps extends DashboardCardProps {
   theme: string;
@@ -114,10 +114,10 @@ export const ChartCard: React.FC<ChartCardProps> = (props) => {
   const controllerContext = React.useContext(ChartContext);
   const history = useHistory();
   const addSubscription = useSubscriptions();
+  const [idx, setIdx] = React.useState(0);
   const [hasRecording, setHasRecording] = React.useState(false);
   const [chartSrc, setChartSrc] = React.useState('');
   const [dashboardUrl, setDashboardUrl] = React.useState('');
-  const [refreshDisabled, setRefreshDisabled] = React.useState(true);
 
   React.useEffect(() => {
     addSubscription(controllerContext.controller.hasActiveRecording().subscribe(setHasRecording));
@@ -131,31 +131,17 @@ export const ChartCard: React.FC<ChartCardProps> = (props) => {
     if (!dashboardUrl) {
       return;
     }
-    setRefreshDisabled(true);
     const u = new URL('/d-solo/main', dashboardUrl);
     u.searchParams.append('theme', props.theme);
     u.searchParams.append('panelId', String(kindToId(props.chartKind)));
     u.searchParams.append('to', 'now');
     u.searchParams.append('from', `now-${props.duration}s`);
-    // TODO make this configurable
-    u.searchParams.append('refresh', '5s');
+    u.searchParams.append('refresh', `${props.period}s`);
     setChartSrc(u.toString());
-  }, [dashboardUrl, setRefreshDisabled, props.theme, props.chartKind, props.duration, setChartSrc]);
-
-  const handleOnLoad = React.useCallback(() => {
-    addSubscription(
-      timer(MIN_REFRESH)
-        .pipe(first())
-        .subscribe((_) => setRefreshDisabled(false))
-    );
-  }, [addSubscription, setRefreshDisabled]);
+  }, [dashboardUrl, props.theme, props.chartKind, props.duration, props.period, setChartSrc]);
 
   React.useEffect(() => {
-    addSubscription(
-      controllerContext.controller.refresh().subscribe((_) => {
-        /* do nothing */
-      })
-    );
+    addSubscription(controllerContext.controller.attach().subscribe(setIdx));
   }, [addSubscription, controllerContext]);
 
   const refresh = React.useCallback(() => {
@@ -183,6 +169,7 @@ export const ChartCard: React.FC<ChartCardProps> = (props) => {
     return (
       <>
         <Button
+          key={0}
           aria-label={`Pop out ${props.chartKind} chart`}
           onClick={popout}
           variant="plain"
@@ -193,27 +180,13 @@ export const ChartCard: React.FC<ChartCardProps> = (props) => {
     );
   }, [props.chartKind, popout, chartSrc, dashboardUrl]);
 
-  const refreshButton = React.useMemo(() => {
-    return (
-      <>
-        <Button
-          aria-label={`Refresh ${props.chartKind} chart`}
-          onClick={refresh}
-          variant="plain"
-          icon={<RedoIcon />}
-          isDisabled={refreshDisabled}
-        />
-      </>
-    );
-  }, [props.chartKind, refresh, refreshDisabled]);
-
   const actions = React.useMemo(() => {
     const a = props.actions || [];
     if (!hasRecording) {
       return a;
     }
-    return [popoutButton, refreshButton, ...a];
-  }, [props.actions, hasRecording, refreshButton, popoutButton]);
+    return [popoutButton, ...a];
+  }, [props.actions, hasRecording, popoutButton]);
 
   const header = React.useMemo(() => {
     if (hasRecording) {
@@ -255,35 +228,34 @@ export const ChartCard: React.FC<ChartCardProps> = (props) => {
   return (
     <>
       <DashboardCard
+        key={idx}
+        id={props.chartKind + '-chart-card'}
         dashboardId={props.dashboardId}
         cardSizes={ChartCardSizes}
-        id={props.chartKind + '-chart-card'}
         isCompact
         style={cardStyle}
         cardHeader={header}
       >
         <CardBody>
           {hasRecording ? (
-            <iframe style={{ height: '100%', width: '100%' }} src={chartSrc} onLoad={handleOnLoad} />
+            <iframe style={{ height: '100%', width: '100%' }} src={chartSrc} />
           ) : (
-            <>
-              <Bullseye>
-                <EmptyState variant={EmptyStateVariant.large}>
-                  <EmptyStateIcon icon={PlusCircleIcon} />
-                  <Title headingLevel="h2" size="md">
-                    Start a source recording
-                  </Title>
-                  <EmptyStateBody>
-                    Metrics cards display data taken from running flight recordings with the label{' '}
-                    <code>origin={RECORDING_NAME}</code>. No such recordings are currently available.
-                  </EmptyStateBody>
+            <Bullseye>
+              <EmptyState variant={EmptyStateVariant.large}>
+                <EmptyStateIcon icon={PlusCircleIcon} />
+                <Title headingLevel="h2" size="md">
+                  Start a source recording
+                </Title>
+                <EmptyStateBody>
+                  Metrics cards display data taken from running flight recordings with the label{' '}
+                  <code>origin={RECORDING_NAME}</code>. No such recordings are currently available.
+                </EmptyStateBody>
 
-                  <Button variant="primary" onClick={handleCreateRecording}>
-                    Create
-                  </Button>
-                </EmptyState>
-              </Bullseye>
-            </>
+                <Button variant="primary" onClick={handleCreateRecording}>
+                  Create
+                </Button>
+              </EmptyState>
+            </Bullseye>
           )}
         </CardBody>
       </DashboardCard>
diff --git a/src/app/Dashboard/Charts/ChartContext.tsx b/src/app/Dashboard/Charts/ChartContext.tsx
index ab23b6afda..68bf229fa2 100644
--- a/src/app/Dashboard/Charts/ChartContext.tsx
+++ b/src/app/Dashboard/Charts/ChartContext.tsx
@@ -44,10 +44,8 @@ export interface Controllers {
   controller: ChartController;
 }
 
-const defaultControllers: Controllers = {
+const ChartContext: React.Context<Controllers> = React.createContext({
   controller: new ChartController(defaultServices.api, defaultServices.target),
-};
+});
 
-const ChartContext: React.Context<Controllers> = React.createContext(defaultControllers);
-
-export { defaultControllers, ChartContext };
+export { ChartContext };
diff --git a/src/app/Dashboard/Charts/ChartController.tsx b/src/app/Dashboard/Charts/ChartController.tsx
index 081f950e5c..4f0d1c7ab8 100644
--- a/src/app/Dashboard/Charts/ChartController.tsx
+++ b/src/app/Dashboard/Charts/ChartController.tsx
@@ -37,10 +37,10 @@
  */
 
 import { ApiService } from '@app/Shared/Services/Api.service';
-import { NO_TARGET, TargetService } from '@app/Shared/Services/Target.service';
+import { NO_TARGET, Target, TargetService } from '@app/Shared/Services/Target.service';
 import {
   BehaviorSubject,
-  combineLatest,
+  catchError,
   concatMap,
   filter,
   finalize,
@@ -49,7 +49,6 @@ import {
   of,
   pairwise,
   ReplaySubject,
-  Subject,
   Subscription,
   throttleTime,
 } from 'rxjs';
@@ -61,8 +60,7 @@ export const MIN_REFRESH = 10_000;
 
 export class ChartController {
   private readonly _refCount$ = new BehaviorSubject<number>(0);
-  private readonly _updateRequests$ = new Subject<void>();
-  private readonly _updates$ = new ReplaySubject<number>();
+  private readonly _updates$ = new ReplaySubject<void>();
   private readonly _hasRecording$ = new ReplaySubject<boolean>();
   private readonly _subscriptions: Subscription[] = [];
 
@@ -77,43 +75,48 @@ export class ChartController {
         const curr = v[1];
         if (prev && !curr) {
           // last subscriber left
-          this._subscriptions.forEach((s) => s.unsubscribe());
+          this._tearDown();
         }
         if (!prev && curr) {
           // first subscriber joined
           this._init();
         }
       });
+
+    this._target
+      .target()
+      .pipe(concatMap((t) => this._hasRecording(t)))
+      .subscribe((v) => {
+        this._hasRecording$.next(v);
+        if (!v) {
+          this._tearDown();
+        }
+      });
   }
 
-  refresh(): Observable<number> {
-    this._refCount$.next(this._refCount$.value + 1);
-    return this._updates$.asObservable().pipe(finalize(() => this._refCount$.next(this._refCount$.value - 1)));
+  hasActiveRecording(): Observable<boolean> {
+    return this._hasRecording$.asObservable();
   }
 
-  requestRefresh(): void {
-    this._updateRequests$.next();
+  attach(): Observable<number> {
+    this._refCount$.next(this._refCount$.value + 1);
+    return this._refCount$.asObservable().pipe(finalize(() => this._refCount$.next(this._refCount$.value - 1)));
   }
 
-  hasActiveRecording(): Observable<boolean> {
-    return this._hasRecording$.asObservable();
+  requestRefresh(): void {
+    this._updates$.next();
   }
 
-  private _init(): void {
-    this._subscriptions.push(
-      this._target
-        .target()
-        .pipe(
-          concatMap((target) => {
-            if (target === NO_TARGET) {
-              return of(false);
-            }
+  private _hasRecording(target: Target): Observable<boolean> {
+    if (target === NO_TARGET) {
+      return of(false);
+    }
 
-            return (
-              this._api
-                /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
-                .graphql<any>(
-                  `
+    return (
+      this._api
+        /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
+        .graphql<any>(
+          `
           query ActiveRecordingsForAutomatedAnalysis($connectUrl: String) {
             targetNodes(filter: { name: $connectUrl }) {
               recordings {
@@ -127,33 +130,29 @@ export class ChartController {
             }
             }
           }`,
-                  { connectUrl: target.connectUrl }
-                )
-                // TODO error handling
-                .pipe(map((resp) => resp.data.targetNodes[0].recordings.active.aggregate.count > 0))
-            );
-          })
+          { connectUrl: target.connectUrl }
+        )
+        // TODO error handling
+        .pipe(
+          map((resp) => resp.data.targetNodes[0].recordings.active.aggregate.count > 0),
+          catchError((_) => of(false))
         )
-        .subscribe((v) => this._hasRecording$.next(v))
-    );
-
-    this._subscriptions.push(
-      this._updateRequests$.pipe(throttleTime(MIN_REFRESH)).subscribe((_) => this._updates$.next(+Date.now()))
-    );
-
-    this._subscriptions.push(
-      this._target
-        .target()
-        .pipe(filter((v) => v !== NO_TARGET))
-        .subscribe((_) => this._updates$.next(+Date.now()))
     );
+  }
 
+  private _init(): void {
     this._subscriptions.push(
-      combineLatest([this.hasActiveRecording().pipe(filter((v) => v)), this._updates$]).subscribe((_) => {
-        this._api.uploadActiveRecordingToGrafana(RECORDING_NAME).subscribe((_) => {
-          /* do nothing */
-        });
-      })
+      this._updates$
+        .pipe(
+          throttleTime(MIN_REFRESH),
+          concatMap((_) => this._hasRecording$),
+          filter((v) => !!v)
+        )
+        .subscribe((_) => {
+          this._api.uploadActiveRecordingToGrafana(RECORDING_NAME).subscribe((_) => {
+            /* do nothing */
+          });
+        })
     );
 
     // TODO listen for websocket notifications about active recordings and update the hasRecording
@@ -204,4 +203,9 @@ export class ChartController {
     //   });
     // })
   }
+
+  private _tearDown() {
+    this._subscriptions.forEach((s) => s.unsubscribe());
+    this._subscriptions.splice(0, this._subscriptions.length);
+  }
 }