Fix flaky "New Card Pinned" behavior. (tensorflow#6708)

We noticed that the behavior for showing the "New Card Pinned" notice is
flaky. It sometimes does not trigger for the first pinned card after
page load.

We just simplify the logic by tracking the "lastPinnedCardTime" in ngrx
state and deriving the "New Card Pinned" notice from that state.

tensorboard/webapp/metrics/store/metrics_reducers.ts

@@ -444,6 +444,7 @@ const {initialState, reducers: namespaceContextedReducer} =
     {
       isSettingsPaneOpen: true,
       isSlideoutMenuOpen: false,
+      lastPinnedCardTime: 0,
       tableEditorSelectedTab: DataTableMode.SINGLE,
       timeSeriesData: {
         scalars: {},
@@ -1151,6 +1152,7 @@ const reducer = createReducer(
     let nextCardMetadataMap = {...state.cardMetadataMap};
     let nextCardStepIndexMap = {...state.cardStepIndex};
     let nextCardStateMap = {...state.cardStateMap};
+    let nextLastPinnedCardTime = state.lastPinnedCardTime;
 
     if (isPinnedCopy) {
       const originalCardId = state.pinnedCardToOriginal.get(cardId);
@@ -1177,6 +1179,7 @@ const reducer = createReducer(
         nextCardMetadataMap = resolvedResult.cardMetadataMap;
         nextCardStepIndexMap = resolvedResult.cardStepIndex;
         nextCardStateMap = resolvedResult.cardStateMap;
+        nextLastPinnedCardTime = Date.now();
       } else {
         const pinnedCardId = state.cardToPinnedCopy.get(cardId)!;
         nextCardToPinnedCopy.delete(cardId);
@@ -1195,6 +1198,7 @@ const reducer = createReducer(
       cardToPinnedCopy: nextCardToPinnedCopy,
       cardToPinnedCopyCache: nextCardToPinnedCopyCache,
       pinnedCardToOriginal: nextPinnedCardToOriginal,
+      lastPinnedCardTime: nextLastPinnedCardTime,
     };
   }),
   on(actions.linkedTimeToggled, (state) => {

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -2858,6 +2858,8 @@ describe('metrics reducers', () => {
     });
 
     it('creates a pinned copy with the same metadata, step index', () => {
+      jasmine.clock().mockDate(new Date(10000));
+
       const cardMetadata = {
         plugin: PluginType.SCALARS,
         tag: 'tagA',
@@ -2918,6 +2920,7 @@ describe('metrics reducers', () => {
         cardToPinnedCopy: new Map([['card1', expectedPinnedCopyId]]),
         cardToPinnedCopyCache: new Map([['card1', expectedPinnedCopyId]]),
         pinnedCardToOriginal: new Map([[expectedPinnedCopyId, 'card1']]),
+        lastPinnedCardTime: 10000,
         timeSeriesData,
       });
       expect(nextState).toEqual(expectedState);

tensorboard/webapp/metrics/store/metrics_selectors.ts

@@ -298,6 +298,13 @@ export const getCanCreateNewPins = createSelector(
   }
 );
 
+export const getLastPinnedCardTime = createSelector(
+  selectMetricsState,
+  (state: MetricsState): number => {
+    return state.lastPinnedCardTime;
+  }
+);
+
 const selectSettings = createSelector(
   selectMetricsState,
   (state): MetricsSettings => {

tensorboard/webapp/metrics/store/metrics_types.ts

@@ -252,6 +252,7 @@ export interface MetricsNonNamespacedState {
   timeSeriesData: TimeSeriesData;
   isSettingsPaneOpen: boolean;
   isSlideoutMenuOpen: boolean;
+  lastPinnedCardTime: number;
   tableEditorSelectedTab: DataTableMode;
   // Default settings. For the legacy reasons, we cannot change the name of the
   // prop. It either is set by application or a user via settings storage.

tensorboard/webapp/metrics/testing.ts

@@ -95,6 +95,7 @@ function buildBlankState(): MetricsState {
     cardToPinnedCopyCache: new Map(),
     pinnedCardToOriginal: new Map(),
     unresolvedImportedPinnedCards: [],
+    lastPinnedCardTime: 0,
     cardMetadataMap: {},
     cardStateMap: {},
     cardStepIndex: {},

tensorboard/webapp/metrics/views/main_view/main_view_test.ts

@@ -1572,108 +1572,35 @@ describe('metrics main view', () => {
         INDICATOR: By.css('.new-card-pinned'),
       };
 
-      function updatePinnedCards(
-        fixture: ComponentFixture<MainViewContainer>,
-        pinnedCardMetadata: CardIdWithMetadata[]
-      ) {
-        store.overrideSelector(
-          selectors.getPinnedCardsWithMetadata,
-          pinnedCardMetadata
-        );
+      it('does not show any indicator when no cards ever pinned', () => {
+        store.overrideSelector(selectors.getLastPinnedCardTime, 0);
         store.refreshState();
-        fixture.detectChanges();
-      }
 
-      it('does not show any indicator initially', () => {
         const fixture = TestBed.createComponent(MainViewContainer);
         fixture.detectChanges();
 
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
         const indicator = fixture.debugElement.query(byCss.INDICATOR);
         expect(indicator).toBeNull();
       });
 
-      it('shows an indication when pin a new card', () => {
-        const fixture = TestBed.createComponent(MainViewContainer);
-        fixture.detectChanges();
-
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-
-        const indicator = fixture.debugElement.query(byCss.INDICATOR);
-        expect(indicator).toBeTruthy();
-      });
-
-      it('shows the indicator when the same card gets pinned toggled', fakeAsync(() => {
-        const fixture = TestBed.createComponent(MainViewContainer);
-        fixture.detectChanges();
-
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        const card2IndicatorBefore = fixture.debugElement.query(
-          byCss.INDICATOR
-        );
-        // Unpin card2.
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        // Wait for 100ms before repinning to avoid flakiness.
-        tick(100);
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        const card2IndicatorAfter = fixture.debugElement.query(byCss.INDICATOR);
-
-        // It should be a different new-card-pinned indicator instance.
-        expect(card2IndicatorBefore.nativeElement).not.toBe(
-          card2IndicatorAfter.nativeElement
-        );
-        expect(card2IndicatorBefore.attributes['data-id']).not.toBe(
-          card2IndicatorAfter.attributes['data-id']
-        );
-      }));
+      it('does not show any indicator if card pinned before load', () => {
+        store.overrideSelector(selectors.getLastPinnedCardTime, 100);
+        store.refreshState();
 
-      it('does not show indicator when you remove a pin', () => {
         const fixture = TestBed.createComponent(MainViewContainer);
         fixture.detectChanges();
 
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        updatePinnedCards(fixture, [
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-
         const indicator = fixture.debugElement.query(byCss.INDICATOR);
         expect(indicator).toBeNull();
       });
 
-      it('shows an indicator a change contains both removal and addition', () => {
+      it('shows an indication when pin a new card', () => {
         const fixture = TestBed.createComponent(MainViewContainer);
         fixture.detectChanges();
 
-        updatePinnedCards(fixture, [
-          {cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
-        updatePinnedCards(fixture, [
-          {cardId: 'card2', ...createCardMetadata(PluginType.SCALARS)},
-          {cardId: 'card3', ...createCardMetadata(PluginType.SCALARS)},
-        ]);
+        store.overrideSelector(selectors.getLastPinnedCardTime, 100);
+        store.refreshState();
+        fixture.detectChanges();
 
         const indicator = fixture.debugElement.query(byCss.INDICATOR);
         expect(indicator).toBeTruthy();

tensorboard/webapp/metrics/views/main_view/pinned_view_component.ts

@@ -28,12 +28,14 @@ import {CardIdWithMetadata} from '../metrics_view_types';
         <span *ngIf="cardIdsWithMetadata.length > 1" class="group-card-count"
           >{{ cardIdsWithMetadata.length }} cards</span
         >
-        <span
-          *ngFor="let id of newCardPinnedIds"
-          [attr.data-id]="id"
-          class="new-card-pinned"
-          >New card pinned</span
-        >
+        <span *ngIf="lastPinnedCardTime">
+          <span
+            *ngFor="let id of [lastPinnedCardTime]"
+            [attr.data-id]="id"
+            class="new-card-pinned"
+            >New card pinned</span
+          >
+        </span>
       </span>
     </div>
     <metrics-card-grid
@@ -51,5 +53,5 @@ import {CardIdWithMetadata} from '../metrics_view_types';
 export class PinnedViewComponent {
   @Input() cardObserver!: CardObserver;
   @Input() cardIdsWithMetadata!: CardIdWithMetadata[];
-  @Input() newCardPinnedIds!: [number];
+  @Input() lastPinnedCardTime!: number;
 }

tensorboard/webapp/metrics/views/main_view/pinned_view_container.ts

@@ -15,10 +15,10 @@ limitations under the License.
 import {ChangeDetectionStrategy, Component, Input} from '@angular/core';
 import {Store} from '@ngrx/store';
 import {Observable} from 'rxjs';
-import {filter, map, pairwise, skip, startWith} from 'rxjs/operators';
+import {skip, startWith} from 'rxjs/operators';
 import {State} from '../../../app_state';
 import {DeepReadonly} from '../../../util/types';
-import {getPinnedCardsWithMetadata} from '../../store';
+import {getLastPinnedCardTime, getPinnedCardsWithMetadata} from '../../store';
 import {CardObserver} from '../card_renderer/card_lazy_loader';
 import {CardIdWithMetadata} from '../metrics_view_types';
 
@@ -27,7 +27,7 @@ import {CardIdWithMetadata} from '../metrics_view_types';
   template: `
     <metrics-pinned-view-component
       [cardIdsWithMetadata]="cardIdsWithMetadata$ | async"
-      [newCardPinnedIds]="newCardPinnedIds$ | async"
+      [lastPinnedCardTime]="lastPinnedCardTime$ | async"
       [cardObserver]="cardObserver"
     ></metrics-pinned-view-component>
   `,
@@ -42,33 +42,9 @@ export class PinnedViewContainer {
     DeepReadonly<CardIdWithMetadata[]>
   > = this.store.select(getPinnedCardsWithMetadata).pipe(startWith([]));
 
-  // An opaque id that changes in value when new cards are pinned.
-  readonly newCardPinnedIds$: Observable<[number]> = this.store
-    .select(getPinnedCardsWithMetadata)
-    .pipe(
-      // Ignore the first pinned card values which is empty, `[]`, in the store.
-      skip(1),
-      map((cards) => cards.map((card) => card.cardId)),
-      pairwise(),
-      map(([before, after]) => {
-        const beforeSet = new Set(before);
-        const afterSet = new Set(after);
-        for (const cardId of afterSet) {
-          if (!beforeSet.has(cardId)) return Date.now();
-        }
-        return null;
-      }),
-      // Pairwise accumulates value until there is a value for both `before` and `after`.
-      // Two `pairwise` can cause values to be ignored too one too many times and
-      // `startWith` helps with setting the first value.
-      startWith(null),
-      pairwise(),
-      map(([before, after]) => {
-        if (before === null && after === null) return null;
-        if (after === null) return [before];
-        return [after];
-      }),
-      filter((value) => value !== null),
-      map((val) => [val![0]] as [number])
-    );
+  readonly lastPinnedCardTime$ = this.store.select(getLastPinnedCardTime).pipe(
+    // Ignore the first value on component load, only reacting to new
+    // pins after page load.
+    skip(1)
+  );
 }