Fix flaky "New Card Pinned" behavior.

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)
+  );
 }