[Global pins] Load saved pins in the localStorage #6821

roseayeon · 2024-04-03T18:14:18Z

Motivation for features / changes

Following #6819 [Global pins] To extend pinned card info available globally , this PR load saved pins in the localStorage.

Technical description of changes

3e5b052 Introduced new action calledmetricsUnresolvedPinnedCardsAdded.
- When metricsUnresolvedPinnedCardsAdded is dispatched, the reducer adds the provided card info to the state. unresolvedImportedPinnedCards.
aae9be4 Added loadSavedPins$ effect in the metrics/effects/index.ts
- When coreActions.pluginsListingLoaded is triggered, it loads saved scalar pins in the localStorage and dispatch metricsUnresolvedPinnedCardsAdded with saved pinned cards.
Added getShouldPersistSettings() checking logic (metioned in [Global pins] Saving pins to localStorage #6819 (comment))
- 2dc49aa To use selector globally, added persistent_settings_selectors to the webapp/selectors.
- Add checking getShouldPersistSettings in the loadSavedPins$ and addOrRemovePin$.

Screenshots of UI changes (or N/A)

N/A

Detailed steps to verify changes work correctly (as executed by you)

Unit test pass & TAP presubmit pass

Alternate designs / implementations considered (or N/A)

N.A

roseayeon · 2024-04-03T18:22:03Z

tensorboard/webapp/metrics/effects/index.ts

+  );
+
+  private readonly loadSavedPins$ = this.actions$.pipe(
+    ofType(initAction, coreActions.pluginsListingLoaded),


QQ. I would like to call loadSavedPin$ when the metrics effect initializes. However, I discovered that initAction is primarily used for testing purposes. Is there a coreActions (or anything else) that is suitable for my use case?

I think initAction is good enough. You might not even need to observe coreActions.pluginsListingLoaded.

(It's not generally intended for initAction to be for testing purposes only, even if it may appear that way).

The requirement is to dispatch metricsUnresolvedPinnedCardsAdded before stateRehydratedFromUrl, right? pluginsListingLoaded doesn't actually guarantee this and is unsafe to use here (we can also verify this with dev console logs).

The metrics effect initAction will be safe to use as all effect initialization will happen in an earlier event loop iteration than stateRehydratedFromUrl because of the delay placed before it (btw this file also shows examples of initAction usage). For the current purposes, it might be most appropriate to listen to the App Routing initAction.

Thanks for comments! I thought of listening to App Routing initAction, but I thought that considering other initActions might be more confusing for maintenance. I think the init action of the metrics effect and leaving a comment that it should be dispatched before stateRehydratedFromUrl will be sufficient for now. WDYT?

bmd3k

I patched and tested it and gave a quick review. Code is clear and the feature seems to work as expected.

I would recommend having someone from your team take a more thorough look, though.

bmd3k · 2024-04-03T20:52:42Z

tensorboard/webapp/metrics/effects/index.ts

+  );
+
+  private readonly loadSavedPins$ = this.actions$.pipe(
+    ofType(initAction, coreActions.pluginsListingLoaded),


I think initAction is good enough. You might not even need to observe coreActions.pluginsListingLoaded.

(It's not generally intended for initAction to be for testing purposes only, even if it may appear that way).

bmd3k · 2024-04-03T21:01:09Z

tensorboard/webapp/metrics/actions/index.ts

@@ -271,5 +272,10 @@ export const metricsHideEmptyCardsToggled = createAction(
  '[Metrics] Hide Empty Cards Changed'
 );

+export const metricsUnresolvedPinnedCardsAdded = createAction(


I think it would be worth mentioning in the action name and descrxiption that this is meant specifically for pinned cards loaded from local storage.

Maybe: metricsUnresolvedPinnedCardsFromLocalStorageAdded

bmd3k · 2024-04-03T21:10:59Z

tensorboard/webapp/metrics/effects/index.ts

+      ([, enableGlobalPins, shouldPersistSettings]) =>
+        enableGlobalPins && shouldPersistSettings
+    ),
+    map(() => {


map() might not be the appropriate operation since you're not actually returning anything used later. Maybe tap()? It doesn't matter very much, though.

Same for addOrRemovePin$.

+1. I think tap makes it much clearer that our intent is to produce side effect, not use the value. It's also what ngrx docs uses and I think there are benefits to aligning with the docs if possible.

hoonji

Looks great for the most part!

hoonji · 2024-04-04T07:06:10Z

tensorboard/webapp/metrics/effects/index.ts

+  );
+
+  private readonly loadSavedPins$ = this.actions$.pipe(
+    ofType(initAction, coreActions.pluginsListingLoaded),


The requirement is to dispatch metricsUnresolvedPinnedCardsAdded before stateRehydratedFromUrl, right? pluginsListingLoaded doesn't actually guarantee this and is unsafe to use here (we can also verify this with dev console logs).

The metrics effect initAction will be safe to use as all effect initialization will happen in an earlier event loop iteration than stateRehydratedFromUrl because of the delay placed before it (btw this file also shows examples of initAction usage). For the current purposes, it might be most appropriate to listen to the App Routing initAction.

hoonji · 2024-04-04T07:13:47Z

tensorboard/webapp/metrics/effects/index.ts

+      ([, enableGlobalPins, shouldPersistSettings]) =>
+        enableGlobalPins && shouldPersistSettings
+    ),
+    map(() => {


+1. I think tap makes it much clearer that our intent is to produce side effect, not use the value. It's also what ngrx docs uses and I think there are benefits to aligning with the docs if possible.

hoonji · 2024-04-04T07:16:03Z

tensorboard/webapp/metrics/effects/index.ts

-        this.addOrRemovePin$
+        this.addOrRemovePin$,
+        /**
+         * Subscribes to: dashboard shown.


Reminder to edit this if we change the loadSavedPins$ trigger

hoonji · 2024-04-04T07:18:37Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -4471,5 +4471,20 @@ describe('metrics reducers', () => {
        expect(state3.isSlideoutMenuOpen).toBe(false);
      });
    });
+
+    describe('#metricsUnresolvedPinnedCardsAdded', () => {
+      it('add unresolved pinned card to unresolvedImportedPinnedCards', () => {


nit: it "adds" unresolved pinned card...

roseayeon

Thanks for the review! Applied

## Motivation for features / changes Following #6821 As pin data is accumulated, unwanted pins can easily crowd the dashboard. Therefore, this PR adds a button that allows users to conveniently clear all pins at once. ## Technical description of changes * 57c6a47 Introduced new action called `metricsClearAllPinnedCards` * When `metricsClearAllPinnedCards` is dispatched, the reducer removed all pinned cards in the state. * a04c2d7 Added `clear all pins` button in the pinned view container. * The button will be shown if there's at least one pinned card. * f0c3a54 Implemented `removeAllScalarPins` method in the saved pins data source. * This method removes stored pins from local storage. * e97b110 Added `removeAllPins` effect in the `metrics/effects/index.ts` * When `metricsClearAllPinnedCards` action is called, this effect will call `removeAllScalarPins` method defined in the saved pin data source. * c0edd37 guarded UI feature (button) with feature flag and added related tests. ## Screenshots of UI changes (or N/A) ### Light mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/a0793e8b-7fea-45f0-b2d8-4e742ca40918) ### Dark mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/581468a1-1a9c-4ab4-b70a-13c5a5b168f6) ## Detailed steps to verify changes work correctly (as executed by you) Unit test pass & cl TAP presubmit pass ## Alternate designs / implementations considered (or N/A) I also considered a feature to select a pinned card and remove the selected pinned card, but I thought that this would be the same as pressing the 'unpin' button individually from the user's perspective. So I implemented a 'clear all pins' button that removes all pinned cards. Any feedback is appreciated!

## Motivation for features / changes Following tensorflow#6819 [Global pins] To extend pinned card info available globally , this PR load saved pins in the localStorage. ## Technical description of changes * 3e5b052 Introduced new action called`metricsUnresolvedPinnedCardsAdded`. * When `metricsUnresolvedPinnedCardsAdded` is dispatched, the reducer adds the provided card info to the `state. unresolvedImportedPinnedCards`. * aae9be4 Added `loadSavedPins$` effect in the `metrics/effects/index.ts` * When `coreActions.pluginsListingLoaded` is triggered, it loads saved scalar pins in the localStorage and dispatch `metricsUnresolvedPinnedCardsAdded` with saved pinned cards. * Added `getShouldPersistSettings()` checking logic (metioned in tensorflow#6819 (comment)) * 2dc49aa To use selector globally, added `persistent_settings_selectors` to the `webapp/selectors`. * Add checking `getShouldPersistSettings` in the `loadSavedPins$` and `addOrRemovePin$`. ## Screenshots of UI changes (or N/A) N/A ## Detailed steps to verify changes work correctly (as executed by you) * Unit test pass & TAP presubmit pass ## Alternate designs / implementations considered (or N/A) N.A

## Motivation for features / changes Following tensorflow#6821 As pin data is accumulated, unwanted pins can easily crowd the dashboard. Therefore, this PR adds a button that allows users to conveniently clear all pins at once. ## Technical description of changes * 57c6a47 Introduced new action called `metricsClearAllPinnedCards` * When `metricsClearAllPinnedCards` is dispatched, the reducer removed all pinned cards in the state. * a04c2d7 Added `clear all pins` button in the pinned view container. * The button will be shown if there's at least one pinned card. * f0c3a54 Implemented `removeAllScalarPins` method in the saved pins data source. * This method removes stored pins from local storage. * e97b110 Added `removeAllPins` effect in the `metrics/effects/index.ts` * When `metricsClearAllPinnedCards` action is called, this effect will call `removeAllScalarPins` method defined in the saved pin data source. * c0edd37 guarded UI feature (button) with feature flag and added related tests. ## Screenshots of UI changes (or N/A) ### Light mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/a0793e8b-7fea-45f0-b2d8-4e742ca40918) ### Dark mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/581468a1-1a9c-4ab4-b70a-13c5a5b168f6) ## Detailed steps to verify changes work correctly (as executed by you) Unit test pass & cl TAP presubmit pass ## Alternate designs / implementations considered (or N/A) I also considered a feature to select a pinned card and remove the selected pinned card, but I thought that this would be the same as pressing the 'unpin' button individually from the user's perspective. So I implemented a 'clear all pins' button that removes all pinned cards. Any feedback is appreciated!

roseayeon added 5 commits April 3, 2024 16:12

define metricsUnresolvedPinnedCardsAdded action and reducer

3e5b052

add loadSavedPins in metrics effect

aae9be4

add persistent_settings_selectors to the webapp/selectors

2dc49aa

apply getShouldPersistSettings to the metrics_effect

a6b64bf

fix lint error

a41e144

roseayeon commented Apr 3, 2024

View reviewed changes

roseayeon requested review from hoonji and bmd3k April 3, 2024 18:43

bmd3k approved these changes Apr 3, 2024

View reviewed changes

hoonji requested changes Apr 4, 2024

View reviewed changes

resolve comments - use tap instead of map, change action name

d638512

roseayeon force-pushed the global-pin-2 branch from ef5285f to d638512 Compare April 4, 2024 08:36

roseayeon commented Apr 4, 2024

View reviewed changes

roseayeon requested a review from hoonji April 4, 2024 08:39

hoonji approved these changes Apr 4, 2024

View reviewed changes

roseayeon merged commit 9424314 into tensorflow:master Apr 4, 2024
13 checks passed

roseayeon deleted the global-pin-2 branch April 5, 2024 04:52

roseayeon mentioned this pull request Apr 11, 2024

[Global pins] Add clear all pins button #6828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Global pins] Load saved pins in the localStorage #6821

[Global pins] Load saved pins in the localStorage #6821

roseayeon commented Apr 3, 2024

roseayeon Apr 3, 2024

bmd3k Apr 3, 2024

hoonji Apr 4, 2024

roseayeon Apr 4, 2024 •

edited

Loading

bmd3k left a comment

bmd3k Apr 3, 2024

bmd3k Apr 3, 2024

bmd3k Apr 3, 2024

hoonji Apr 4, 2024

roseayeon Apr 4, 2024

hoonji left a comment

hoonji Apr 4, 2024

hoonji Apr 4, 2024

hoonji Apr 4, 2024

hoonji Apr 4, 2024

roseayeon left a comment

[Global pins] Load saved pins in the localStorage #6821

[Global pins] Load saved pins in the localStorage #6821

Conversation

roseayeon commented Apr 3, 2024

Motivation for features / changes

Technical description of changes

Screenshots of UI changes (or N/A)

Detailed steps to verify changes work correctly (as executed by you)

Alternate designs / implementations considered (or N/A)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roseayeon Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roseayeon left a comment

Choose a reason for hiding this comment

roseayeon Apr 4, 2024 •

edited

Loading