feat(GUI): reset webview after navigating away #1422

Shou · 2017-05-12T15:45:11Z

We reload and reset the webview to its original URL when the user
navigates away from the success screen.

Changelog-Entry: Reset webview after navigating away from success
screen.

lurch · 2017-05-12T18:20:34Z

lib/gui/index.html

@@ -57,7 +57,8 @@
        isFinish: state.is('success')
      }">
      <safe-webview
-        src="'https://etcher.io/success-banner/'"></safe-webview>
+        src="'https://etcher.io/'"


My mistake, that's for testing.

lurch · 2017-05-12T18:21:38Z

lib/gui/index.html

@@ -57,7 +57,8 @@
        isFinish: state.is('success')
      }">
      <safe-webview
-        src="'https://etcher.io/success-banner/'"></safe-webview>
+        src="'https://etcher.io/'"
+        on-state-change="state.onStateChange"></safe-webview>


Out of curiosity, what triggers this on-state-change to happen?

It happens whenever the $state changes, e.g. user navigating to the 'settings', 'success', or back to the 'main' page.

Does that mean that changing from the 'flashing' page to the 'finished' page will also trigger a state-change, and thus a user-visible delay while the webview reloads?
Perhaps it only makes sense to reload the webview when leaving the 'finished' page? (because IMHO merely going back and forth to the settings page shouldn't cause a reload)

Oh sorry, that's what it does. So onStateChange is a higher-order function that passes the state changes to its argument function, and in there we check if the state change is from 'success' before reloading, visible in the comment code block below.

It looks weird that the safe-webview is exposing this attribute. Can this all be handled internally?

Ahhhh, thanks for the explanation. So I guess it's more like onLeaveSuccessPage than onStateChange? ;-)

@jviotti It's the 'cleanest' way I've found so far, given the React/Angular mixing situation we're in. Basically, the React element isn't re-rendered when the attributes change, so we can't just pass an Angular variable and cause a re-render whenever that changes, and I suspect we won't want to do that anyway because if attribute changes caused a re-render it would refresh the webview. I'll look into if there's a better way though!

@lurch onStateChange is what it says on the tin, but way it's used is more like onLeaveSuccessPage because the first-class function we pass to onStateChange checks if the state change is from 'success', so we could call the function onStateChange takes onLeaveSuccessPage.

lurch · 2017-05-12T18:22:40Z

lib/gui/components/safe-webview/safe-webview.js

+      if (this.props.onStateChange) {
+        this.props.onStateChange((event, toState, toParams, fromState) => {
+          if (fromState.name === 'success' && this.state.shouldLoad) {
+            this.refs.webview.src = this.props.src;


I guess it could also check that this.refs.webview.src != this.props.src, to avoid an unnecessary reload() ?

My thought here was in case the page has been updated, I assume some people leave Etcher open constantly to burn many cards, so this would make them see updated pages.

@alexandrosm Can we tell from the analytics how many people leave Etcher open for an extended period of time? (my gut instinct would be not many people at all) How important is it that users always see the 'latest' banner, rather than the banner that they downloaded when they started Etcher?

jviotti · 2017-05-15T14:26:17Z

lib/gui/app.js


  /**
   * @param {string} state - state page
   */
  this.is = $state.is;

+  /**
+   * @param {function} func - function to pass $rootScope to


This is missing the @summary, @public, @example, etc. Also function should be Function

Also function should be Function

Does / should eslint pick that up?

jviotti · 2017-05-15T14:27:51Z

lib/gui/app.js

@@ -298,11 +298,18 @@ app.controller('HeaderController', function(SelectionStateModel, OSOpenExternalS

 });

-app.controller('StateController', function($state) {
+app.controller('StateController', function($rootScope, $state) {

  /**
   * @param {string} state - state page


Looks like I failed to catch it last time, but this should also include all the JSDoc tags mentioned below.

jviotti · 2017-05-15T14:29:14Z

lib/gui/index.html

@@ -57,7 +57,8 @@
        isFinish: state.is('success')
      }">
      <safe-webview
-        src="'https://etcher.io/success-banner/'"></safe-webview>
+        src="'https://etcher.io/'"
+        on-state-change="state.onStateChange"></safe-webview>


It looks weird that the safe-webview is exposing this attribute. Can this all be handled internally?

jviotti · 2017-05-15T14:29:37Z

lib/gui/app.js

+  /**
+   * @param {function} func - function to pass $rootScope to
+   */
+  this.onStateChange = (func) => {


If this function is called multiple times, then multiple event listeners will be registered for this event.

Shou · 2017-05-17T17:25:16Z

What do you think of the way this is handled now?

lurch · 2017-05-17T17:47:32Z

lib/gui/components/safe-webview/safe-webview.js

   */
  componentDidMount() {

-    // There is no element to add events to if 'shouldLoad' is false.
-    if (this.state.shouldLoad) {
+    // Events React is unaware of have to be handled manually


minor nit, but could you change this to "Events that React is ...", otherwise "Events React" might sound like another framework ;)

jviotti · 2017-05-18T02:57:17Z

Sorry for the huge delay on this. I'll do a review tomorrow :)

jviotti · 2017-05-18T17:41:14Z

lib/gui/components/safe-webview/safe-webview.js

+    // We set the 'etcher-version' GET parameter here.
+    url.searchParams.set('etcher-version', packageJSON.version);
+
+    return url;


Can we do url.href here, given that its the only property we use?

jviotti · 2017-05-18T17:41:34Z

lib/gui/components/safe-webview/safe-webview.js

+    const url = new URL(this.props.src);
+
+    // We set the 'etcher-version' GET parameter here.
+    url.searchParams.set('etcher-version', packageJSON.version);


Can we extract etcher-version to a constant at the top of this component?

jviotti · 2017-05-18T17:44:38Z

Minor comment (and doesn't need to be addressed here), but since the safe webview element is just a single file, can we move it to lib/gui/components/safe-webview.js?

jviotti · 2017-05-18T17:49:47Z

I don't think this is working. I'm doing the following:

Check the URL of the webview to https://etcher.io
Flash an image
In the webview, click a link on https://etcher.io (e.g. the changelog)
Wait for it to load
Click "Flash Another"
Flash the image again

I now see the webview on the changelog page, so it hasn't been reset.

Shou · 2017-05-22T16:13:04Z

@jviotti I looked into it and it has to do with the reloading being too fast and rolling back the URL change. However, I went further, and it seems like the caching is a bit aggressive here and the reloading has no effect. I believe we can disable the caching as you can see with the closing PR here.

jviotti · 2017-05-23T13:48:49Z

I don't think I get it. Where do we pass --disable-http-cache switch? Also, can we disable caching just for the webview, but keep it enabled for the actual main window?

Shou · 2017-05-24T12:37:31Z

@jviotti Here's a working implementation. The webview now uses a specific session where the cache is disabled, so yes that's possible and done. Unfortunately we can't set the 'partition' property directly in the React attributes because it's an unknown attribute to React, and it strips unknown attributes. There's a big issue about it. This is why the src setting was moved to componentDidMount.

jviotti · 2017-05-24T13:20:51Z

lib/gui/components/safe-webview/safe-webview.js

@@ -28,10 +28,15 @@ const packageJSON = require('../../../../package.json');
 const MODULE_NAME = 'Etcher.Components.SafeWebview';
 const angularSafeWebview = angular.module(MODULE_NAME, []);

+const VERSION_PARAM = 'etcher-version';


Can we document this constant?

jviotti · 2017-05-24T13:22:07Z

lib/gui/components/safe-webview/safe-webview.js

-      this.refs.webview.removeEventListener('did-get-response-details', this.didGetResponseDetails);
-      this.refs.webview.removeEventListener('new-window', this.constructor.newWindow);
-      this.refs.webview.removeEventListener('console-message', this.constructor.consoleMessage);
+      const DECISECOND = 100;


Can we describe what this variable is for instead? e.g. WEBVIEW_RELOAD_TIMEOUT_MS or something like that?

jviotti · 2017-05-24T13:23:34Z

lib/gui/components/safe-webview/safe-webview.js

+  makeURL() {
+    const url = new URL(this.props.src);
+
+    // We set the 'etcher-version' GET parameter here.


Can we update this comment to not hard-code the string etcher-version?

jviotti · 2017-05-24T13:25:01Z

lib/gui/components/safe-webview/safe-webview.js

-      this.refs.webview.addEventListener('new-window', this.constructor.newWindow);
-      this.refs.webview.addEventListener('console-message', this.constructor.consoleMessage);
+    // Use the 'success-banner' session
+    this.refs.webview.partition = 'persist:success-banner';


Looks like we're duplicating this same string here and in the session definition, which means it might get out of sync. Can we declare it only in one place (e.g. with a constant)?

Actually, should we define the electron session in this file instead?

electron.session is only available in the main process so I don't think it's possible unfortunately.

Also what's a good location to share this constant? lib/shared/constants.js maybe?

You can do electron.remote.session from the renderer process.

Let's move 'persist:success-banner' to a constant, so we don't type it twice here, and when declaring the session.

jviotti · 2017-05-24T13:31:15Z

I'm getting this when clicking "Flash Another", after navigating to another page in the webview:

I still keep seeing the new page after a retry though. The webview is not getting reset.

Shou · 2017-05-24T15:38:22Z

Strange, I'm not getting any errors, but I can confirm that the resetting bug was back though. This commit should fix it.

lurch · 2017-05-24T15:46:54Z

lib/gui/components/safe-webview/safe-webview.js

@@ -95,15 +101,14 @@ class SafeWebview extends react.PureComponent {
  componentWillReceiveProps(nextProps) {
    if (this.props.currentStateName === 'success' && nextProps.currentStateName !== 'success') {

-      // Reset URL to initial
-      this.refs.webview.src = this.makeURL();
+      // Only reset the URL to initial if it has changed, otherwise reload the page,


Hahaha, isn't that what I originally suggested? ;)

Yeah, if only I had listened lol

lurch · 2017-05-24T23:04:35Z

Unit-tests are failing with:

> [email protected] jslint /etcher
> eslint lib tests scripts bin versionist.conf.js
/etcher/lib/gui/components/safe-webview/safe-webview.js
  106:7  error  Unexpected negated condition  no-negated-condition
✖ 1 problem (1 error, 0 warnings)

jviotti · 2017-05-26T00:51:12Z

lib/gui/components/safe-webview/safe-webview.js

   */
-  componentWillUnmount() {
+  componentWillReceiveProps(nextProps) {
+    if (this.props.currentStateName === 'success' && nextProps.currentStateName !== 'success') {


This component is hard-coding a state name that is set outside of the component (in the Angular.js app), which doesn't look right. It looks like we should make the component take a boolean attribute to determine when it should be refreshed, and move the fact that in our use case that depends on the success state outside of this component.

Yeah true, it should be as agnostic as possible to the rest of the program.

jviotti · 2017-05-26T00:51:52Z

lib/gui/etcher.js

@@ -39,6 +39,13 @@ electron.app.on('ready', () => {
  // No menu bar
  electron.Menu.setApplicationMenu(null);

+  // Make a persistent electron session called 'success-banner'
+  electron.session.fromPartition('persist:success-banner', {


I thing I said this before, but can we move this to the safe webview component by using electron.remote.session?

Yeah I forgot about that, we should be able to use that.

Shou · 2017-05-30T12:31:39Z

Updated

jviotti · 2017-05-30T14:57:44Z

lib/gui/index.html

      }">
      <safe-webview
-        src="'https://etcher.io/success-banner/'"></safe-webview>
+        src="'https://etcher.io/success-banner/'"
+        refresh-now="state.previousName === 'success'"></safe-webview>


I'm going to test this later, but what happens if this always evaluates to true? Will the webview keep refreshing?

I made sure to check if the previous prop was false and the current one true, so that a refresh is only triggered when the change from false → true happens and not true → true. React will sometimes trigger componentWillReceiveProps() when it's not just a prop update, so that's also why I took this precaution.

jviotti · 2017-05-30T14:59:19Z

lib/gui/app.js

-app.controller('StateController', function($state) {
+app.controller('StateController', function($rootScope) {
+
+  $rootScope.$on('$stateChangeSuccess', (event, toState, toParams, fromState) => {


This is probably pedantic, but a standard practice is to register a handler for the $destroy event of the controller and un-register any registered event.

jviotti · 2017-05-30T15:05:12Z

The webview now gets successfully reset, and the uncaught error I was getting is also gone. Great work @Shou ! The only thing is that timeout errors are still printed inline in a scary way:

Can we catch these error and send a nice "Success banner timeout error" Mixpanel event instead?

lurch · 2017-05-30T23:29:51Z

lib/gui/components/safe-webview/safe-webview.js

-      this.refs.webview.removeEventListener('new-window', this.constructor.newWindow);
-      this.refs.webview.removeEventListener('console-message', this.constructor.consoleMessage);
+      } else {
+        this.refs.webview.src = this.makeURL();


Should we assign the result of this.makeURL() to a const variable, to avoid calling the function multiple times?

lurch · 2017-05-30T23:32:23Z

lib/gui/components/safe-webview/safe-webview.js

+    this.refs.webview.removeEventListener('did-fail-load', this.didFailLoad);
+    this.refs.webview.removeEventListener('did-get-response-details', this.didGetResponseDetails);
+    this.refs.webview.removeEventListener('new-window', this.constructor.newWindow);
+    this.refs.webview.removeEventListener('console-message', this.constructor.consoleMessage);


Any (easy) way to avoid the "repetition" between the addEventListener and removeEventListener calls?

lurch · 2017-05-30T23:36:51Z

lib/gui/components/safe-webview/safe-webview.js

+   * @function
+   * @public
+   *
+   * @returns {URL} url object


Is this @returns docstring now incorrect?

lurch · 2017-05-30T23:37:42Z

lib/gui/components/safe-webview/safe-webview.js

   */
  componentDidMount() {

-    // There is no element to add events to if 'shouldLoad' is false.
-    if (this.state.shouldLoad) {
+    // Events React is unaware of have to be handled manually


Could you add the that in here too? ;-)

lurch · 2017-05-30T23:43:57Z

I think this PR also needs to be rebased on master to prevent the Ubuntu 12.04 Linux x86 builds failing issue...

We reload and reset the webview to its original URL when the user navigates away from the success screen. Changelog-Entry: Reset webview after navigating away from success screen.

and moving the session creation to SafeWebview

Shou · 2017-05-31T16:04:02Z

I've followed the advice above now. @jviotti I'm still a bit bewildered by that error since I've never gotten it myself, but I suspect it's an error that can happen on the Etcher homepage, which is forwarded over console.error and caught by Corvus inside Etcher, which means everything's working as intended, as banner pages are only supposed to use the console for Etcher-related debugging and errors intended to reach Corvus.

lurch · 2017-05-31T16:39:03Z

Yeah, I also wonder if the error might be coming from the Etcher homepage itself, as it's very javascript-heavy. @Shou is it possible to redirect any JS errors created by the pages displayed inside the webview into a blackhole, so that they don't end up spamming the DevTools console?

jviotti · 2017-06-01T19:51:38Z

I agree with @lurch. Let's prevent any error from the banner from spamming DevTools, however please catch this particular timeout error from the Etcher side (from the webview) and transform it into a "Success banner timeout" Mixpanel event, since I'd like to know how often that happens.

Shou · 2017-06-06T13:22:46Z

So I've changed it from accepting all console messages to only specific JSON objects containing type and message now, what do you think?

lurch · 2017-06-06T13:32:44Z

lib/gui/components/safe-webview.js

@@ -196,16 +196,28 @@ class SafeWebview extends react.PureComponent {
  /**
   * @summary Forward console messages from the webview to analytics module
   * @param {Event} event - event object
+   *
+   * @example
+   * console.log({


Should this example be using consoleMessage rather than console.log ?

Ahhh, so it'd actually be in the banner page that we do the console.log shown here, and consoleMessage then forwards that to our analytics services? Now I understand :-)
(so perhaps the example could clarify that?)

Also, would it be worth the analytics-logging clarifying (perhaps by adding an extra property?) that these are messages being generated by whatever JS is running in the page being displayed in the webview, and not messages being generated by the core Etcher application itself?

lurch · 2017-06-06T13:44:21Z

lib/gui/components/safe-webview.js

+      } else if (data.type === ERROR_LEVEL) {
+        analytics.logException(data.message);
+      }
+    });


So, just to check I'm understanding this diff correctly:
consoleMessage takes an event object, which is expected to have a (possibly JSON-encoded) .message attribute? (and also previously a .level attribute, which is now ignored)
And then if the JSON message is decoded correctly (i.e. into data), it's then expected to have both .type and .message attributes?

Which I guess means the expected usage is something like:

consoleMessage({level:0, message: "{type: 0, message: 'Hello, World!'}"});

jviotti · 2017-06-06T14:33:20Z

lib/gui/components/safe-webview.js

+   * });
+   */
+  static consoleMessage(event) {
+    const LOG_LEVEL = 0;


Can we re-use our robot existing commands? See https://github.com/resin-io/etcher/blob/master/lib/shared/robot/index.js#L27. We only have "error" and "log" at the moment, but we could also add a "warning" one.

Unless there's an explicit need for 'warnings', it might suffice to just stick with "error" and "log"?

True, that should be more than enough.

jviotti · 2017-06-06T14:34:04Z

lib/gui/components/safe-webview.js

+    const ERROR_LEVEL = 2;
+
+    _.attempt(() => {
+      const data = JSON.parse(event.message);


Let's also re-use our robot module to parse messages.

Nice thinking @jviotti 👍

jviotti · 2017-06-06T14:35:10Z

lib/gui/components/safe-webview.js

+    // Make a persistent electron session for the webview
+    electron.remote.session.fromPartition(ELECTRON_SESSION, {
+
+      // Disable the cache for the session


Can we extend this message to describe why we want to disable the cache?

Shou · 2017-06-07T10:16:53Z

Here we go. Let me know what you think.

lurch · 2017-06-07T11:07:45Z

lib/gui/components/safe-webview.js

   * });
+   *
+   * // or with robot
+   * robot.printError('Good night!');


But the robot isn't exposed to the webview, is it?

Nope, but maybe we could/should?

I don't think we should (for security reasons, it makes sense to keep the webview as isolated as possible). And therefore this robot.printError example probably isn't useful?

Oh, I don't mean through injection, more like decoupling robot from Etcher into its own module, but I guess if/until then I'll just remove this example.

lurch · 2017-06-07T11:08:50Z

lib/gui/components/safe-webview.js

-        analytics.logException(data.message);
-      }
-    });
+    if (command === robot.COMMAND.ERROR) {


This should be using else if ?

lurch · 2017-06-07T11:11:03Z

lib/gui/components/safe-webview.js

+    const {
+      command,
+      data
+    } = _.attempt(robot.parseMessage, event.message);


It's up to @jviotti which style he prefers, but why 'unpack' the object here, instead of just checking e.g. parsedMessage.command instead of command later on?

Although according to https://github.com/resin-io/etcher/blob/master/lib/shared/robot/README.md you're 'supposed' to use robot.getCommand(message) and robot.getData(message) ;-)
And it looks like you can use robot.isMessage to avoid using _.attempt.

lurch · 2017-06-07T11:14:37Z

lib/gui/components/safe-webview.js

   * console.log({
-   *   type: 0,
-   *   message: 'Hello, World!'
+   *   command: 'error',


Given that the Robot constants aren't available to the webview, perhaps the examples could show both 'error' and 'log' being used?
And maybe the @summary should be "Forward specially-formatted console messages..."?

Shou · 2017-06-07T13:43:19Z

How about now?

lurch · 2017-06-07T14:13:08Z

Looks great :-)

jviotti · 2017-06-08T15:43:09Z

Works amazingly well!

lurch reviewed May 12, 2017

View reviewed changes

jviotti suggested changes May 15, 2017

View reviewed changes

Shou force-pushed the webview-refresh branch 4 times, most recently from a6dad66 to 8096137 Compare May 17, 2017 16:51

lurch reviewed May 17, 2017

View reviewed changes

jviotti reviewed May 18, 2017

View reviewed changes

jviotti suggested changes May 24, 2017

View reviewed changes

lurch reviewed May 24, 2017

View reviewed changes

jviotti suggested changes May 26, 2017

View reviewed changes

Shou force-pushed the webview-refresh branch from d31ce55 to 862e527 Compare May 30, 2017 13:00

jviotti reviewed May 30, 2017

View reviewed changes

lurch reviewed May 30, 2017

View reviewed changes

feat(GUI): reset webview after navigating away

72fdf50

We reload and reset the webview to its original URL when the user navigates away from the success screen. Changelog-Entry: Reset webview after navigating away from success screen.

Shou added 2 commits May 31, 2017 16:45

fix linter not equals complaint

aae9fea

StateController refactor to agnostify SafeWebview

717a0f0

and moving the session creation to SafeWebview

Shou force-pushed the webview-refresh branch from 862e527 to cf11dc3 Compare May 31, 2017 15:46

remove component folder, refactor events and url, session constant

77a978a

Shou force-pushed the webview-refresh branch from cf11dc3 to 77a978a Compare May 31, 2017 15:54

only accept specific json objects from webview console

245dcef

lurch reviewed Jun 6, 2017

View reviewed changes

jviotti reviewed Jun 6, 2017

View reviewed changes

use robot

0b688ab

lurch reviewed Jun 7, 2017

View reviewed changes

extensive usage of robot

74766fb

lurch requested a review from jviotti June 7, 2017 14:13

jviotti approved these changes Jun 8, 2017

View reviewed changes

jviotti merged commit 31a2389 into master Jun 8, 2017

jviotti deleted the webview-refresh branch June 8, 2017 15:43

lurch mentioned this pull request Jun 28, 2017

feat: add analytic events balena-io/etcher-homepage#75

Merged

feat(GUI): reset webview after navigating away #1422

feat(GUI): reset webview after navigating away #1422

Conversation

Shou commented May 12, 2017

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

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

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

Shou commented May 17, 2017

Choose a reason for hiding this comment

jviotti commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviotti commented May 18, 2017

jviotti commented May 18, 2017

Shou commented May 22, 2017

jviotti commented May 23, 2017

Shou commented May 24, 2017 • edited Loading

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

Shou May 24, 2017 • edited Loading

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

jviotti commented May 24, 2017

Shou commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lurch commented May 24, 2017

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

Shou commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviotti May 30, 2017 • edited Loading

Choose a reason for hiding this comment

jviotti commented May 30, 2017

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

lurch commented May 30, 2017

Shou commented May 31, 2017

lurch commented May 31, 2017

jviotti commented Jun 1, 2017

Shou commented Jun 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shou commented Jun 7, 2017

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

Shou commented May 24, 2017 •

edited

Loading

Shou May 24, 2017 •

edited

Loading

jviotti May 30, 2017 •

edited

Loading