Update ngrx store for pinned cards from storage #4220

psybuzz · 2020-10-06T21:01:17Z

Diffbase: #4219
Followup: #4221

Adds ngrx reducer logic and types to support storing
pinned cards that originate from URL storage. Before
'tagMetadataLoaded' creates real cards, TensorBoard is now
able to keep track of pinned cards from the URL, in the ngrx
store.

A followup will implement the actual deeplink provider.

Googlers, see test sync cl/335717460

psybuzz · 2020-10-08T14:39:24Z

As per offline discussion, added comments to hopefully explain things, and renamed:
CardInStorage --> CardUniqueInfo
PrePinnedCards --> UnresolvedImportedPinnedCards

Ready for review

stephanwlee · 2020-10-07T17:49:13Z

tensorboard/webapp/metrics/types.ts

+/**
+ * The normal representation of a card, for storage layers.
+ */
+export interface CardInStorage {
+  tag: string;
+  runId?: string;
+  sample?: number;
+}
+
+/**
+ * The state after deserializing a URL for hydration.
+ */
+export interface URLDeserializedState {
+  metrics: {
+    pinnedCards: CardInStorage[];
+  };
+}


These verbiage is very confusing to me. What is the storage referring to?

The state after deserializing a URL for hydration

What does this mean? You serialize a state into URL safe string and that is called "dehydrating" in some vernacular but the sentence does not make sense whichever way I read it:

"The state ... for hydration" <- state is not for hydration? Object from hydration may make sense

"The state ... a URL for hydration" <- URL is not for hydration...

What am I missing?

Reworded.

The original intent was to be parsed as:

[ [the state] [after] [ [deserializing a url] [for hydration] ] ]

which I realize is awkward :)

stephanwlee · 2020-10-07T17:59:27Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+ * Returns an identifier useful for comparing a card in storage with a real card
+ * with loaded metadata.
+ */
+function hashCardInStorage(


serializedCard instead please? Also, specify the return type.

hashCard does not make much sense and word storage is not that descriptive.

From the usage, it looks like the word "hash" is referring to unique identifier. Can we just say identifier then?

Added return types.

How about serializeCardUniqueInfo ?

tensorboard/webapp/metrics/types.ts

stephanwlee · 2020-10-08T15:52:20Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+ * Returns an identifier useful for comparing a card in storage with a real card
+ * with loaded metadata.
+ */
+function hashCardUniqueInfo(


word hash can be a verb but it confused me at first. Maybe getCardUniqueInfoHash?

stephanwlee · 2020-10-08T15:52:46Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+  tag: string,
+  runId?: string | null,
+  sample?: number
+) {


specify the return type of these pure utils.

stephanwlee · 2020-10-08T16:58:04Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

+      ]);
+    });
+
+    it('does not populate pre-pinned store with already pinned cards', () => {


s/pre-pinned/unresolvedImported

Thanks, I missed all these 'pre-pinned' sites

tensorboard/webapp/metrics/store/metrics_reducers.ts

stephanwlee · 2020-10-08T17:06:17Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

+        routeKind: RouteKind.EXPERIMENT,
+        partialState: {
+          metrics: {
+            pinnedCards: [{tag: 'accuracy'}],


also test a case where pinnedCards are just duplicates.

Good one, done

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

stephanwlee · 2020-10-08T17:07:34Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

      );
    });

+    it('automatically pins cards from pre-pinned storage', () => {


add a case where the unresolvedImportedPinnedCards are inconsistent with the cardMetadata.

for instance, missing step/runId for image based tag.

stephanwlee · 2020-10-12T17:42:12Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+    // We need to include previous unresolved imported pins, because the new
+    // hydrated state might not include them. For example, navigating from
+    // experiment A (with pins) --> B --> A, we want to ensure that rehydration
+    // does not drop the old pins on A.


old pins -> old unresolved pins.

stephanwlee · 2020-10-12T21:34:46Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+      const nextCardList = newCardIds.length
+        ? [...state.cardList, ...newCardIds]
+        : state.cardList;


can't you spread zero length array? const nextCardList = [...state.cardList, ...newCardIds] should suffice?

Done.

Ah, the intent was to do a small optimization and avoid returning a different reference in case there are no new cards. However, I see that cardMetadataMap will get a new reference anyways, so perhaps we ought to do a full optimization later if needed.

stephanwlee · 2020-10-12T21:38:28Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

+      );
+      expect(nextState.cardToPinnedCopy).toEqual(
+        new Map([['card1', pinnedCopyId]])
+      );


assert that unresolvedImportedPinnedCards is empty.

tensorboard/webapp/metrics/types.ts

stephanwlee · 2020-10-12T23:19:24Z

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts

Not a big comment person but let's codified in the comment that the contract is not to sanitize/clip/validate unresolved cards.

psybuzz

Thanks for reviewing!

psybuzz · 2020-10-12T23:29:32Z

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts

AppRoutingEffects recently added logic to preserve URL hash upon the initial 'navigated' action (see [1]). While this fixed cases where the URL needs to be read upon initial page load, it does not address an upcoming case needed for deeplinking pins in Time Series. Flow: pin a card, DeepLinkProvider serializes `?pinnedCards=` in the URL which drops `#timeseries`, reload the tab, the selected dashboard is no longer Time Series. Expected: Time Series remains selected upon tab reload. This change overcomes this issue the DeepLinkProvider by preserving the hash upon same-RouteId navigations. Manually checked internal TB embedders that navigating to a different route id still clears the hash. Googlers, see sync cl/335688801 Followup: #4220 [1] #4213

Diffbase: #4220 When cards are pinned/un-pinned in Time Series, their state is now reflected in the URL, while the user is on a route of kind EXPERIMENT or COMPARE_EXPERIMENTS. Pinned cards will persist across tab reloads. This behavior can be configured per-route via updating the use of CoreDeepLinkProvider.

googlebot added the cla: yes label Oct 6, 2020

psybuzz force-pushed the psybuzz-urlpins2 branch from 1af823e to 13c76e5 Compare October 6, 2020 21:05

psybuzz mentioned this pull request Oct 6, 2020

feat: persist pinned cards in URL with DeepLinkProvider #4221

Merged

psybuzz marked this pull request as ready for review October 6, 2020 22:21

psybuzz requested a review from stephanwlee October 6, 2020 22:22

psybuzz mentioned this pull request Oct 6, 2020

AppRouting should preserve hash on same-RouteId navigations #4219

Merged

stephanwlee reviewed Oct 8, 2020

View reviewed changes

psybuzz force-pushed the psybuzz-urlpins2 branch from 081ab94 to 648c89b Compare October 9, 2020 22:39

psybuzz requested a review from stephanwlee October 9, 2020 22:40

stephanwlee reviewed Oct 12, 2020

View reviewed changes

psybuzz requested a review from stephanwlee October 12, 2020 23:00

stephanwlee approved these changes Oct 12, 2020

View reviewed changes

psybuzz commented Oct 12, 2020

View reviewed changes

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts Outdated

Copy link

Contributor Author

psybuzz Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Base automatically changed from psybuzz-urlpins to master October 12, 2020 23:30

psybuzz added 5 commits October 12, 2020 16:32

Update ngrx store for pinned cards from storage

85b6345

rename and comment

aeaac00

ac2

ac1b6a9

ac3

34ba0a9

ac4

ebe623b

psybuzz force-pushed the psybuzz-urlpins2 branch from cdead2f to ebe623b Compare October 12, 2020 23:32

psybuzz merged commit 7da5f74 into master Oct 13, 2020

psybuzz deleted the psybuzz-urlpins2 branch October 13, 2020 00:54

Update ngrx store for pinned cards from storage #4220

Update ngrx store for pinned cards from storage #4220

Uh oh!

Conversation

psybuzz commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psybuzz commented Oct 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psybuzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

psybuzz commented Oct 6, 2020 •

edited

Loading