Skip to content

Commit d1b4cd9

Browse files
psybuzzpsybuzz
authored
timeseries: improve card observer visibility logic with element ids (#5234)
The Time Series dashboard has been under the assumption that `getVisibleCardIds` accurately returns the set of CardIds that are within the visible viewport. This assumption, backed by the CardObserver using an IntersectionObserver, is false in an event sequence involving the same CardId twice: - domElementA with cardId1 is shown - domElementB with cardId1 is hidden The Ngrx store is misled to believe that cardId1 is hidden, even though domElementA may still be visible. This change corrects the CardObserver to track unique DOM Elements. The symptom is flaky and hard to consistently reproduce. Manually tested by patching in https://github.com/psybuzz/tensorboard/tree/persist-tag for tagFilter persistence in the URL, opened URLs with "?tagFilter=accuracy" that cause the bad behavior more consistently, and observed that they no longer occur. Googlers, see b/179450711.
1 parent 31db4e0 commit d1b4cd9

File tree

16 files changed

+253
-102
lines changed

16 files changed

+253
-102
lines changed

‎tensorboard/webapp/metrics/actions/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ tf_ts_library(
1212
deps = [
1313
"//tensorboard/webapp/metrics:internal_types",
1414
"//tensorboard/webapp/metrics/data_source",
15+
"//tensorboard/webapp/util:dom",
1516
"@npm//@ngrx/store",
1617
],
1718
)

‎tensorboard/webapp/metrics/actions/index.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ limitations under the License.
1414
==============================================================================*/
1515
import {createAction, props} from '@ngrx/store';
1616

17+
import {ElementId} from '../../util/dom';
1718
import {
1819
TagMetadata,
1920
TimeSeriesRequest,
@@ -113,12 +114,15 @@ export const fetchTimeSeriesLoaded = createAction(
113114
);
114115

115116
/**
116-
* An event when some cards enter or exit the viewport. The card sets must be
117-
* mutually exclusive.
117+
* An event when some cards enter or exit the viewport. An element within
118+
* `enteredCards` must not be found within `exitedCards`, and vice versa.
118119
*/
119120
export const cardVisibilityChanged = createAction(
120121
'[Metrics] Card Visibility Changed',
121-
props<{enteredCards: Set<CardId>; exitedCards: Set<CardId>}>()
122+
props<{
123+
enteredCards: Array<{elementId: ElementId; cardId: CardId}>;
124+
exitedCards: Array<{elementId: ElementId; cardId: CardId}>;
125+
}>()
122126
);
123127

124128
export const cardStepSliderChanged = createAction(

‎tensorboard/webapp/metrics/effects/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ tf_ts_library(
4646
"//tensorboard/webapp/metrics/data_source",
4747
"//tensorboard/webapp/metrics/store",
4848
"//tensorboard/webapp/types",
49+
"//tensorboard/webapp/util:dom",
4950
"//tensorboard/webapp/webapp_data_source:http_client_testing",
5051
"@npm//@ngrx/effects",
5152
"@npm//@ngrx/store",

‎tensorboard/webapp/metrics/effects/metrics_effects_test.ts‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {of, Subject} from 'rxjs';
2626
import {buildNavigatedAction} from '../../app_routing/testing';
2727
import {State} from '../../app_state';
2828
import * as selectors from '../../selectors';
29+
import {nextElementId} from '../../util/dom';
2930
import * as actions from '../actions';
3031
import {
3132
METRICS_PLUGIN_ID,
@@ -450,10 +451,7 @@ describe('metrics effects', () => {
450451
store.refreshState();
451452

452453
actions$.next(
453-
actions.cardVisibilityChanged({
454-
enteredCards: new Set(),
455-
exitedCards: new Set(),
456-
})
454+
actions.cardVisibilityChanged({enteredCards: [], exitedCards: []})
457455
);
458456

459457
expect(fetchTimeSeriesSpy).not.toHaveBeenCalled();
@@ -474,15 +472,16 @@ describe('metrics effects', () => {
474472
loadState: DataLoadState.NOT_LOADED,
475473
});
476474

475+
const card1ElementId = nextElementId();
477476
store.overrideSelector(
478477
selectors.getVisibleCardIdSet,
479478
new Set<string>([])
480479
);
481480
store.refreshState();
482481
actions$.next(
483482
actions.cardVisibilityChanged({
484-
enteredCards: new Set(),
485-
exitedCards: new Set(['card1']),
483+
enteredCards: [],
484+
exitedCards: [{elementId: card1ElementId, cardId: 'card1'}],
486485
})
487486
);
488487

@@ -496,8 +495,8 @@ describe('metrics effects', () => {
496495
store.refreshState();
497496
actions$.next(
498497
actions.cardVisibilityChanged({
499-
enteredCards: new Set(['card1']),
500-
exitedCards: new Set(),
498+
enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
499+
exitedCards: [],
501500
})
502501
);
503502

@@ -530,15 +529,16 @@ describe('metrics effects', () => {
530529
});
531530

532531
// Initial load.
532+
const card1ElementId = nextElementId();
533533
store.overrideSelector(
534534
selectors.getVisibleCardIdSet,
535535
new Set(['card1'])
536536
);
537537
store.refreshState();
538538
actions$.next(
539539
actions.cardVisibilityChanged({
540-
enteredCards: new Set(['card1']),
541-
exitedCards: new Set(),
540+
enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
541+
exitedCards: [],
542542
})
543543
);
544544

@@ -550,8 +550,8 @@ describe('metrics effects', () => {
550550
store.refreshState();
551551
actions$.next(
552552
actions.cardVisibilityChanged({
553-
enteredCards: new Set(),
554-
exitedCards: new Set(['card1']),
553+
enteredCards: [],
554+
exitedCards: [{elementId: card1ElementId, cardId: 'card1'}],
555555
})
556556
);
557557

@@ -566,8 +566,8 @@ describe('metrics effects', () => {
566566
store.refreshState();
567567
actions$.next(
568568
actions.cardVisibilityChanged({
569-
enteredCards: new Set(['card1']),
570-
exitedCards: new Set(),
569+
enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
570+
exitedCards: [],
571571
})
572572
);
573573

@@ -628,8 +628,11 @@ describe('metrics effects', () => {
628628
store.refreshState();
629629
actions$.next(
630630
actions.cardVisibilityChanged({
631-
enteredCards: new Set(['card1', 'card2']),
632-
exitedCards: new Set(),
631+
enteredCards: [
632+
{elementId: nextElementId(), cardId: 'card1'},
633+
{elementId: nextElementId(), cardId: 'card2'},
634+
],
635+
exitedCards: [],
633636
})
634637
);
635638

@@ -673,8 +676,8 @@ describe('metrics effects', () => {
673676
store.refreshState();
674677
actions$.next(
675678
actions.cardVisibilityChanged({
676-
enteredCards: new Set(['card1']),
677-
exitedCards: new Set(),
679+
enteredCards: [{elementId: nextElementId(), cardId: 'card1'}],
680+
exitedCards: [],
678681
})
679682
);
680683

‎tensorboard/webapp/metrics/store/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ tf_ts_library(
2222
"//tensorboard/webapp/metrics/data_source",
2323
"//tensorboard/webapp/persistent_settings",
2424
"//tensorboard/webapp/types",
25+
"//tensorboard/webapp/util:dom",
2526
"//tensorboard/webapp/util:lang",
2627
"//tensorboard/webapp/util:ngrx",
2728
"//tensorboard/webapp/util:types",
@@ -66,6 +67,7 @@ tf_ts_library(
6667
"//tensorboard/webapp/metrics:internal_types",
6768
"//tensorboard/webapp/metrics/data_source",
6869
"//tensorboard/webapp/types",
70+
"//tensorboard/webapp/util:dom",
6971
],
7072
)
7173

@@ -91,6 +93,7 @@ tf_ts_library(
9193
"//tensorboard/webapp/metrics/data_source",
9294
"//tensorboard/webapp/persistent_settings",
9395
"//tensorboard/webapp/types",
96+
"//tensorboard/webapp/util:dom",
9497
"@npm//@types/jasmine",
9598
],
9699
)

‎tensorboard/webapp/metrics/store/metrics_reducers.ts‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {RouteKind} from '../../app_routing/types';
2020
import * as coreActions from '../../core/actions';
2121
import {globalSettingsLoaded} from '../../persistent_settings';
2222
import {DataLoadState} from '../../types/data';
23+
import {ElementId} from '../../util/dom';
2324
import {mapObjectValues} from '../../util/lang';
2425
import {composeReducers} from '../../util/ngrx';
2526
import * as actions from '../actions';
@@ -259,7 +260,7 @@ const {initialState, reducers: routeContextReducer} = createRouteContextedState<
259260
},
260261
settings: METRICS_SETTINGS_DEFAULT,
261262
settingOverrides: {},
262-
visibleCards: new Set<CardId>(),
263+
visibleCardMap: new Map<ElementId, CardId>(),
263264
},
264265

265266
/** onRouteIdChanged */
@@ -269,7 +270,7 @@ const {initialState, reducers: routeContextReducer} = createRouteContextedState<
269270
// Reset visible cards in case we resume a route that was left dirty.
270271
// Since visibility tracking is async, the state may not have received
271272
// 'exited card' updates when it was cached by the router.
272-
visibleCards: new Set<CardId>(),
273+
visibleCardMap: new Map<ElementId, CardId>(),
273274
};
274275
}
275276
);
@@ -753,25 +754,24 @@ const reducer = createReducer(
753754
return {...state, tagGroupExpanded};
754755
}),
755756
on(actions.cardVisibilityChanged, (state, {enteredCards, exitedCards}) => {
756-
if (enteredCards.size === 0 && exitedCards.size === 0) {
757+
if (!enteredCards.length && !exitedCards.length) {
757758
return state;
758759
}
759760

760-
const visibleCards = new Set(state.visibleCards);
761-
enteredCards.forEach((cardId) => {
762-
visibleCards.add(cardId);
763-
});
764-
exitedCards.forEach((cardId) => {
765-
visibleCards.delete(cardId);
766-
767-
if (enteredCards.has(cardId)) {
761+
const visibleCardMap = new Map(state.visibleCardMap);
762+
enteredCards.forEach(({elementId, cardId}) => {
763+
const existingCardId = visibleCardMap.get(elementId) ?? null;
764+
if (existingCardId !== null && existingCardId !== cardId) {
768765
throw new Error(
769-
`A 'cardVisibilityChanged' with an invalid ` +
770-
`payload contains overlapping sets`
766+
`A DOM element cannot be reused for more than 1 unique card metadata`
771767
);
772768
}
769+
visibleCardMap.set(elementId, cardId);
770+
});
771+
exitedCards.forEach(({elementId}) => {
772+
visibleCardMap.delete(elementId);
773773
});
774-
return {...state, visibleCards};
774+
return {...state, visibleCardMap};
775775
}),
776776
on(actions.cardPinStateToggled, (state, {cardId}) => {
777777
const isPinnedCopy = state.pinnedCardToOriginal.has(cardId);

‎tensorboard/webapp/metrics/store/metrics_reducers_test.ts‎

Lines changed: 60 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {RouteKind} from '../../app_routing/types';
1818
import * as coreActions from '../../core/actions';
1919
import {globalSettingsLoaded} from '../../persistent_settings';
2020
import {DataLoadState} from '../../types/data';
21+
import {nextElementId} from '../../util/dom';
2122
import * as actions from '../actions';
2223
import {
2324
PluginType,
@@ -501,7 +502,10 @@ describe('metrics reducers', () => {
501502
describe('route id changes', () => {
502503
it('resets data when mounting a new route', () => {
503504
const prevState = buildMetricsState({
504-
visibleCards: new Set(['card1', 'card2']),
505+
visibleCardMap: new Map([
506+
[nextElementId(), 'card1'],
507+
[nextElementId(), 'card2'],
508+
]),
505509
});
506510

507511
const navigateFrom1to2 = routingActions.navigated({
@@ -529,9 +533,9 @@ describe('metrics reducers', () => {
529533
nextState = reducers(nextState, navigateFrom2to1);
530534

531535
const expectedState = buildMetricsState({
532-
visibleCards: new Set(),
536+
visibleCardMap: new Map(),
533537
});
534-
expect(nextState.visibleCards).toEqual(expectedState.visibleCards);
538+
expect(nextState.visibleCardMap).toEqual(expectedState.visibleCardMap);
535539
});
536540
});
537541

@@ -1224,26 +1228,29 @@ describe('metrics reducers', () => {
12241228
describe('card visibility', () => {
12251229
it('no-ops when nothing is changed', () => {
12261230
const beforeState = buildMetricsState({
1227-
visibleCards: new Set(['card1']),
1231+
visibleCardMap: new Map([[nextElementId(), 'card1']]),
12281232
});
12291233

12301234
const action = actions.cardVisibilityChanged({
1231-
enteredCards: new Set(),
1232-
exitedCards: new Set(),
1235+
enteredCards: [],
1236+
exitedCards: [],
12331237
});
12341238
const nextState = reducers(beforeState, action);
1235-
expect(nextState.visibleCards).toEqual(new Set(['card1']));
1239+
expect(nextState.visibleCardMap).toEqual(
1240+
new Map([[jasmine.any(Symbol), 'card1']])
1241+
);
12361242
expect(nextState).toBe(beforeState);
12371243
});
12381244

12391245
it('handles bad payloads', () => {
1246+
const existingElementId = nextElementId();
12401247
const beforeState = buildMetricsState({
1241-
visibleCards: new Set(['card1']),
1248+
visibleCardMap: new Map([[existingElementId, 'card1']]),
12421249
});
12431250

12441251
const action = actions.cardVisibilityChanged({
1245-
enteredCards: new Set(['duplicateCard']),
1246-
exitedCards: new Set(['duplicateCard']),
1252+
enteredCards: [{elementId: existingElementId, cardId: 'card2'}],
1253+
exitedCards: [],
12471254
});
12481255
let nextState = beforeState;
12491256
expect(() => {
@@ -1253,19 +1260,57 @@ describe('metrics reducers', () => {
12531260
});
12541261

12551262
it('handles adding and removing cards', () => {
1263+
const existingElementIds = [nextElementId(), nextElementId()];
12561264
const beforeState = buildMetricsState({
1257-
visibleCards: new Set(['existingCard1', 'existingCard2']),
1265+
visibleCardMap: new Map([
1266+
[existingElementIds[0], 'existingCard1'],
1267+
[existingElementIds[1], 'existingCard2'],
1268+
]),
12581269
});
12591270

1271+
const newCard1ElementId = nextElementId();
12601272
const action = actions.cardVisibilityChanged({
1261-
enteredCards: new Set(['existingCard1', 'newCard1']),
1262-
exitedCards: new Set(['existingCard2', 'newCard2']),
1273+
enteredCards: [
1274+
{elementId: existingElementIds[0], cardId: 'existingCard1'},
1275+
{elementId: newCard1ElementId, cardId: 'newCard1'},
1276+
],
1277+
exitedCards: [
1278+
{elementId: existingElementIds[1], cardId: 'existingCard2'},
1279+
{elementId: nextElementId(), cardId: 'newCard2'},
1280+
],
12631281
});
12641282
const nextState = reducers(beforeState, action);
1265-
expect(nextState.visibleCards).toEqual(
1266-
new Set(['existingCard1', 'newCard1'])
1283+
expect(nextState.visibleCardMap).toEqual(
1284+
new Map([
1285+
[existingElementIds[0], 'existingCard1'],
1286+
[newCard1ElementId, 'newCard1'],
1287+
])
12671288
);
12681289
});
1290+
1291+
it(
1292+
'marks a card as visible when it enters and exits on different ' +
1293+
'elements',
1294+
() => {
1295+
const existingElementIds = [nextElementId(), nextElementId()];
1296+
const beforeState = buildMetricsState({
1297+
visibleCardMap: new Map(),
1298+
});
1299+
1300+
const action = actions.cardVisibilityChanged({
1301+
enteredCards: [
1302+
{elementId: existingElementIds[0], cardId: 'duplicateCard'},
1303+
],
1304+
exitedCards: [
1305+
{elementId: existingElementIds[1], cardId: 'duplicateCard'},
1306+
],
1307+
});
1308+
const nextState = reducers(beforeState, action);
1309+
expect(nextState.visibleCardMap).toEqual(
1310+
new Map([[existingElementIds[0], 'duplicateCard']])
1311+
);
1312+
}
1313+
);
12691314
});
12701315

12711316
describe('cardPinStateToggled', () => {

0 commit comments

Comments
 (0)