Introduce alert snackbar UI #4244

psybuzz · 2020-10-16T00:06:57Z

Diffbase: #4221
Followup: #4245

Introduces an alert snackbar UI, which surfaces application
errors to the user in the corner of the screen.

Technical design discussion:

On the surface, there are many ways to architect an alert/error
system in ngrx, where our goal is to allow any feature in TB or
a TB embedder to trigger alerts in the snackbar UI.

Creating an 'Alert' feature store, whose reducer listens to other
features' actions would work for TB, but not embedders, since
TB core does not have access to ngrx actions from external
features.

One way to workaround this composability problem is to force
embedders to override the Alerts module and composeReducers.
Upon receiving an embedder-specific error, the embedder's
Alerts module can set the latest error accordingly. However,
this approach assumes that a reducer can determine whether
an action should produce an alert or not, based on the action's
payload.

In the followup, the 'cardPinStateToggled' action provides a
counterexample: it should only trigger an alert if there are too
many current pinned cards.

One way to workaround this is to introduce AlertEffects as a
layer that listens to actions from all features, and converts them
into generic 'alertReported' actions (assuming effects can be
composed, similar to reducers). However, this leads us to a world
where AlertModule depends heavily on other features. For
example, if AlertModule depends on feature F, it becomes
difficult to create a TB application / embedder that needs Alerts
without needing F. This is a design constraint we'd like to follow,
as it gives flexibility in bundling TB with only a subset of the
current features.

One might imagine that specific features can do the translation
themselves, using specific effects to convert actions into
'alertReported' actions. However, this synchronous dispatch of
composite actions goes against a best practice. In other
scenarios, it is known to lead to hard-to-reason-about in-between
states, and makes undoing or reverting state changes difficult
due to the processing of multiple actions.

Thus, we finally land on the existing patch. Essentially, it hides
away the composite action using Angular's DI framework. The
actions that should trigger alerts are registered once, and are
dispatched by the AlertEffects, rather than specific features.

Googlers, see test sync cl/338344258

stephanwlee

This PR introduces the concept of error "details", a string which
is defined in a TS module that will be surfaced in the UI.

This did not sufficiently describe your design choice; it looks like the change is recommending putting what should be localized in the action payload which kind of means that, for instance, there is a network error or whatever, you would be firing an action, presumably synchronously. That is conceptually regarding an action as a mutation/command, not event.

Are you willfully making the exception here? Does it need to be be done that way?

stephanwlee · 2020-10-16T05:19:07Z

tensorboard/webapp/app_module.ts

 import {MatIconModule} from './mat_icon_module';
 import {PluginsModule} from './plugins/plugins_module';
 import {ROOT_REDUCERS, loggerMetaReducerFactory} from './reducer_config';
-import {ReloaderModule} from './reloader/reloader_module';


Is this an unused import cleanup?

Yes. Although not related to the change, it is a small drive-by that fixes a lint warning upon sync, since this file is changed.

psybuzz · 2020-10-20T03:18:49Z

In the followup, #4245, this approach requires us to include 'canCreateNewPins' and 'wasPinned' to the cardPinStateToggled action payload.

Overall, my impression of this approach is that it's slightly more complex than it might deserve to be. I would be fine with this approach, or doing the simpler composite-action (noting that it would be an exception to our best practice. Also open to any other ideas, if we have any.

stephanwlee · 2020-10-20T15:57:32Z

tensorboard/webapp/alert/actions/BUILD

+package(default_visibility = ["//tensorboard:internal"])
+
+tf_ts_library(
+    name = "actions",


make sure this is only usable by the effects.

Visibility reduced to alerts/.

tensorboard/webapp/alert/effects/index.ts

stephanwlee · 2020-10-20T16:10:38Z

tensorboard/webapp/alert/types.ts

+ * An alert structure used when creating newly reported alerts.
+ */
+export interface AlertReport {
+  details: string;


I do not think this is sufficient to migrate existing code. We will need to have an optional actionType for an alert that can have actions (you may also want to have localized string for those actions).

I would like to add further fields such as actionType in a separate PR, and keep this one scoped to things that will be used in the followup. Existing embedders will also want the snackbar to have an 'x' close button, which I believe may require upgrading the snackbar.open to .openFromComponent.

stephanwlee · 2020-10-20T16:11:09Z

tensorboard/webapp/alert/types.ts

+ * An alert structure used when creating newly reported alerts.
+ */
+export interface AlertReport {
+  details: string;


details -> localizedMessage?

stephanwlee · 2020-10-20T16:14:42Z

tensorboard/webapp/reducer_config.ts

+  // return !isDevMode()
+  //   ? (reducer) => (state, action) => {
+  //       return reducer(state, action);
+  //     }
+  //   : logger;


revert please.

Whoops, done.

stephanwlee · 2020-10-20T16:15:10Z

tensorboard/webapp/app_container.ng.html

 <main #main>
  <router-outlet></router-outlet>
 </main>
+<alert-snackbar></alert-snackbar>


Hmm, this is not great for composability but I have no good recommendation here.

Agreed. Ideally these below (hash-storage, page-title, settings-polymer-interop) might also be added in some other way. I haven't thought of a way I like yet, but we could imagine that a more composable approach could be applied to all of them at once in the future.

stephanwlee · 2020-10-20T16:15:33Z

tensorboard/webapp/alert/views/alert_snackbar_test.ts

+    store = TestBed.inject<Store<State>>(Store) as MockStore<State>;
+  });
+
+  it('should open the snackbar on each alert', () => {


omit the world "should" in test; it is redundant and not needed.

stephanwlee

LGTM. I don't think we immediately in this PR require the actionable snackbar so I will be okay if you want to do a follow up.

stephanwlee · 2020-10-20T22:26:26Z

tensorboard/webapp/alert/types.ts

+ * An alert structure used when creating newly reported alerts.
+ */
+export interface AlertReport {
+  localizedMessage: string;


I may have left a comment in the other PR but I don't think I find any comment.

How would you plan on supporting different actions in the snackbar? i.e., in some cases, we want to have button to retry sending a request. In order to replace our current error handling, we would need that facility. Without it, we will have two ways to achieve the same thing.

I'd like to do this in a separate PR, if that works for you. This is what I had in mind:

extend AlertReport with a new field:

export interface AlertReport { localizedMessage: string; retryCallback?: (store: Store) => void; }

used like so:

AlertActionModule.registerActionAlerts([ { localizedMessage: "Fetch failed", retryCallback: (store) => { // ... store.dispatch(fetchAction(...)); } }, ])

with the AlertSnackbarContainer using .openFromComponent, instead of .open,
to open an AlertSnackbarComponent, which contains:

// template <div *ngIf="!!alertInfo.retryCallback" (click)="onRetryClicked">Retry</div> // component @Input() alertInfo: AlertInfo onRetryClicked() { this.alertInfo.retryCallback(); }

I wish it to be more generic than just retryCallback (and it, I don't think should be a callback since it opens the API for just about anything) but I will look forward to seeing the change afterwards.

stephanwlee · 2020-10-20T22:27:45Z

tensorboard/webapp/alert/views/alert_snackbar_container.ts

+        tap((alert) => {
+          this.showAlert(alert!.localizedMessage);
+        })


maybe do this in the subscribe instead.

psybuzz

Thanks for the review!

psybuzz · 2020-10-21T09:31:23Z

tensorboard/webapp/alert/types.ts

+ * An alert structure used when creating newly reported alerts.
+ */
+export interface AlertReport {
+  localizedMessage: string;


I'd like to do this in a separate PR, if that works for you. This is what I had in mind:

extend AlertReport with a new field:

export interface AlertReport { localizedMessage: string; retryCallback?: (store: Store) => void; }

used like so:

AlertActionModule.registerActionAlerts([ { localizedMessage: "Fetch failed", retryCallback: (store) => { // ... store.dispatch(fetchAction(...)); } }, ])

with the AlertSnackbarContainer using .openFromComponent, instead of .open,
to open an AlertSnackbarComponent, which contains:

// template <div *ngIf="!!alertInfo.retryCallback" (click)="onRetryClicked">Retry</div> // component @Input() alertInfo: AlertInfo onRetryClicked() { this.alertInfo.retryCallback(); }

psybuzz · 2020-10-21T09:31:27Z

tensorboard/webapp/alert/views/alert_snackbar_container.ts

+        tap((alert) => {
+          this.showAlert(alert!.localizedMessage);
+        })


stephanwlee · 2020-10-21T17:02:04Z

tensorboard/webapp/alert/types.ts

+ * An alert structure used when creating newly reported alerts.
+ */
+export interface AlertReport {
+  localizedMessage: string;


I wish it to be more generic than just retryCallback (and it, I don't think should be a callback since it opens the API for just about anything) but I will look forward to seeing the change afterwards.

Diffbase: #4244 Prevents the "toggle pin" button from creating a new pin, if we already have the max number of pins allowed. Unresolved pins still count towards the quota. This enforced limit prevents the URL from overflowing, see #4242

feat: persist pinned cards in URL with DeepLinkProvider

7ad061e

google-cla bot added the cla: yes label Oct 16, 2020

psybuzz mentioned this pull request Oct 16, 2020

[time series] prevent pinning cards from exceeding limit #4245

Merged

psybuzz marked this pull request as ready for review October 16, 2020 00:10

psybuzz requested a review from stephanwlee October 16, 2020 00:10

psybuzz force-pushed the psybuzz-urlpins4 branch from 3da73d9 to 655ba3a Compare October 16, 2020 00:13

stephanwlee reviewed Oct 16, 2020

View reviewed changes

psybuzz added 2 commits October 16, 2020 13:31

ac

29d4cd2

Introduce alert snackbar

0c2c195

psybuzz force-pushed the psybuzz-urlpins4 branch from 655ba3a to 0c2c195 Compare October 20, 2020 02:55

psybuzz changed the title ~~Introduce error snackbar UI~~ Introduce alert snackbar UI Oct 20, 2020

psybuzz requested a review from stephanwlee October 20, 2020 03:15

stephanwlee reviewed Oct 20, 2020

View reviewed changes

ac

db3a01b

psybuzz requested a review from stephanwlee October 20, 2020 22:11

stephanwlee approved these changes Oct 20, 2020

View reviewed changes

ac2

9827774

psybuzz commented Oct 21, 2020

View reviewed changes

stephanwlee approved these changes Oct 21, 2020

View reviewed changes

ac3

eb2d027

Base automatically changed from psybuzz-urlpins3 to master October 22, 2020 01:16

psybuzz merged commit cf53ff6 into master Oct 22, 2020

psybuzz deleted the psybuzz-urlpins4 branch October 22, 2020 02:48

Introduce alert snackbar UI #4244

Introduce alert snackbar UI #4244

Uh oh!

Conversation

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

Uh oh!

Uh oh!

stephanwlee left a comment

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!

psybuzz commented Oct 20, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanwlee left a comment

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!

psybuzz left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

psybuzz commented Oct 16, 2020 •

edited

Loading