timeseries: improve card observer visibility logic with element ids #5234
Conversation
| elementToCardIdMap.set(element, this.cardId); | ||
| elementIdMap.set(element, nextElementId()); |
There was a problem hiding this comment.
Instead of two different Map, consider using a data structure like {elementId: string; cardId: string} then reading it would be much more easier. Logically, when cardId is null, elementId should never be null and as currently typed, this is hard to guarantee.
tensorboard/webapp/util/dom.ts
Outdated
| /** | ||
| * An opaque id intended to refer to exactly one specific DOM Element. | ||
| */ | ||
| export type ElementId = number; |
There was a problem hiding this comment.
Will not mention this more but consider using Symbol if you want it to be opaque and want to be able to do !elementId to check nullness instead of elementId ?? null which weakly leaks the fact that ElementId is a number.
There was a problem hiding this comment.
Done as Symbol.
Still using an incrementing number inside the symbol, so that when printed in the console, it's easier to glance at things like:
entered [Symbol(1), Symbol(2)], exited [Symbol(3)]
| elementToCardIdMap.set(element, this.cardId); | ||
| elementIdMap.set(element, nextElementId()); |
tensorboard/webapp/util/dom.ts
Outdated
| /** | ||
| * An opaque id intended to refer to exactly one specific DOM Element. | ||
| */ | ||
| export type ElementId = number; |
There was a problem hiding this comment.
Done as Symbol.
Still using an incrementing number inside the symbol, so that when printed in the console, it's easier to glance at things like:
entered [Symbol(1), Symbol(2)], exited [Symbol(3)]
…ensorflow#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.
…ensorflow#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.
2 participants
The Time Series dashboard has been under the assumption that
getVisibleCardIdsaccurately 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:
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.