From e2985f7eec68c714a156c65742fd78ef2fdb1c0a Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Mon, 9 Oct 2023 11:13:25 -0700
Subject: [PATCH] JS: Remove dead state transition validation code

Soooo, because `this.state` was initialised to `null` and
not 'start', this code was always actually dead.
`validateStateTransition` had no branch for `oldState` being
null, so always returned `false`. This meant `this.state`
was *never* set to current state, and so in all callbacks,
`oldState` was always `null`!

I validated this again by console.logging `oldState` and
`newState` inside `validateStateTransition`. Here's the output
for when a full build is performed:

```
null -> waiting
null -> fetching
null -> unknown
null -> building
null -> failed
null -> built
null -> launching
```

And for when launching a previously built image
```
null -> launching
null -> ready
```

I also put one just above where `this.state` is set, and that
code was never called.

I was hoping to remove all this code anyway, as it doesn't
actually validate nor test anything - that's really done by
unit tests and integration tests. I thought this would hide
bugs - but turns out, it was a bug itself, and hid nothing.

This also changes the signature of the callback for
`onChangeState`, dropping `oldState` and `newState`. `oldState`
was always `null`, and in our code, we actually don't use
`newState` *at all*. The one place that was conditional on
`oldState` being null is now unconditional, because `oldState`
was always `null`.

Ref https://github.com/jupyterhub/binderhub/issues/774
---
 binderhub/static/js/index.js              | 22 ++++--------------
 js/packages/binderhub-client/lib/index.js | 28 +----------------------
 2 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/binderhub/static/js/index.js b/binderhub/static/js/index.js
index c29f0b420..3eb83ddc1 100644
--- a/binderhub/static/js/index.js
+++ b/binderhub/static/js/index.js
@@ -1,14 +1,4 @@
 /* If this file gets over 200 lines of code long (not counting docs / comments), start using a framework
-  State transitions that are valid are:
-  start -> waiting
-  start -> built
-  start -> failed
-  waiting -> building
-  waiting -> failed
-  building -> pushing
-  building -> failed
-  pushing -> built
-  pushing -> failed
 */
 import ClipboardJS from 'clipboard';
 import 'event-source-polyfill';
@@ -54,7 +44,7 @@ function build(providerSpec, log, fitAddon, path, pathType) {
   const buildEndpointUrl = new URL("build", new URL(BASE_URL, window.location.href));
   const image = new BinderRepository(providerSpec, buildEndpointUrl, buildToken);
 
-  image.onStateChange('*', function(oldState, newState, data) {
+  image.onStateChange('*', function(data) {
     if (data.message !== undefined) {
       log.writeAndStore(data.message);
       fitAddon.fit();
@@ -92,16 +82,14 @@ function build(providerSpec, log, fitAddon, path, pathType) {
     image.close();
   });
 
-  image.onStateChange('built', function(oldState) {
-    if (oldState === null) {
-      $('#phase-already-built').removeClass('hidden');
-      $('#phase-launching').removeClass('hidden');
-    }
+  image.onStateChange('built', function() {
+    $('#phase-already-built').removeClass('hidden');
+    $('#phase-launching').removeClass('hidden');
     $('#phase-launching').removeClass('hidden');
     updateFavicon(BASE_URL + "favicon_success.ico");
   });
 
-  image.onStateChange('ready', function(oldState, newState, data) {
+  image.onStateChange('ready', function(data) {
     image.close();
     // If data.url is an absolute URL, it'll be used. Else, it'll be interpreted
     // relative to current page's URL.
diff --git a/js/packages/binderhub-client/lib/index.js b/js/packages/binderhub-client/lib/index.js
index 4261e4a7a..fa9c66f56 100644
--- a/js/packages/binderhub-client/lib/index.js
+++ b/js/packages/binderhub-client/lib/index.js
@@ -33,7 +33,6 @@ export class BinderRepository {
       this.buildUrl.searchParams.append("build_token", buildToken);
     }
     this.callbacks = {};
-    this.state = null;
   }
 
   /**
@@ -128,39 +127,14 @@ export class BinderRepository {
     }
   }
 
-  /**
-   * @param {string} oldState Old state the building process was in
-   * @param {string} newState New state the building process is in
-   * @returns True if transition from oldState to newState is valid, False otherwise
-   */
-  validateStateTransition(oldState, newState) {
-    if (oldState === "start") {
-      return (
-        newState === "waiting" || newState === "built" || newState === "failed"
-      );
-    } else if (oldState === "waiting") {
-      return newState === "building" || newState === "failed";
-    } else if (oldState === "building") {
-      return newState === "pushing" || newState === "failed";
-    } else if (oldState === "pushing") {
-      return newState === "built" || newState === "failed";
-    } else {
-      return false;
-    }
-  }
-
   _changeState(state, data) {
     [state, "*"].map(key => {
       const callbacks = this.callbacks[key];
       if (callbacks) {
         for (let i = 0; i < callbacks.length; i++) {
-          callbacks[i](this.state, state || this.state, data);
+          callbacks[i](data);
         }
       }
     });
-
-    if (state && this.validateStateTransition(this.state, state)) {
-      this.state = state;
-    }
   }
 }