fix: render the view "Back" lands on after the popup is reopened (closes #268)

src/popup/index.js

@@ -9,16 +9,17 @@ const {
     $,
     showView,
     updateDebugBanner,
-    setRenderMain,
+    setBackRenderer,
     pushCurrentView,
     goBack,
     clearViewStack,
 } = require("./views/helpers");
 const { applyTheme } = require("./theme");
-// Views that can be fully re-rendered from persisted state. All others fall
-// back to the nearest restorable parent; see the module for why the
-// secret-bearing views are absent.
-const { RESTORABLE_VIEWS } = require("./restorableViews");
+// Renders a view the popup lands on without having navigated to it forward:
+// on restore here, and on Back. Only the views that can be fully re-rendered
+// from persisted state (RESTORABLE_VIEWS, src/popup/restorableViews.js) go
+// through it; anything else falls back to the nearest restorable parent.
+const { renderView, makeBackRenderer } = require("./viewRouter");
 
 const home = require("./views/home");
 const welcome = require("./views/welcome");
@@ -108,91 +109,22 @@ const ctx = {
     },
 };
 
-function needsAddress(view) {
-    return (
-        view === "address" ||
-        view === "address-token" ||
-        view === "receive" ||
-        view === "transaction"
-    );
-}
-
-function hasValidAddress() {
-    return (
-        state.selectedWallet !== null &&
-        state.selectedAddress !== null &&
-        state.wallets[state.selectedWallet] &&
-        state.wallets[state.selectedWallet].addresses[state.selectedAddress]
-    );
-}
+// The view modules the router renders through, keyed as it expects them.
+const viewModules = {
+    main: { show: () => fallbackView() },
+    addressDetail,
+    addressToken,
+    receive,
+    settings,
+    settingsAddToken,
+    confirmTx,
+    transactionDetail,
+    txStatus,
+};
 
 function restoreView() {
-    const view = state.currentView;
-    if (!view || !RESTORABLE_VIEWS.has(view)) {
-        return fallbackView();
-    }
-
-    if (needsAddress(view) && !hasValidAddress()) {
-        return fallbackView();
-    }
-
-    if (view === "address-token" && !state.selectedToken) {
-        return fallbackView();
-    }
-
-    switch (view) {
-        case "address":
-            addressDetail.show();
-            break;
-        case "address-token":
-            addressToken.show();
-            break;
-        case "receive":
-            receive.show();
-            break;
-        case "settings":
-            settings.show();
-            break;
-        case "settings-addtoken":
-            settingsAddToken.show();
-            break;
-        case "confirm-tx":
-            if (state.viewData && state.viewData.pendingTx) {
-                confirmTx.restore();
-            } else {
-                fallbackView();
-            }
-            break;
-        case "transaction":
-            if (state.viewData && state.viewData.tx) {
-                transactionDetail.render();
-            } else {
-                fallbackView();
-            }
-            break;
-        case "wait-tx":
-            // Resumes the receipt poll from the persisted broadcast time.
-            if (!txStatus.restoreWait()) {
-                fallbackView();
-            }
-            break;
-        case "success-tx":
-            if (state.viewData && state.viewData.hash) {
-                txStatus.renderSuccess();
-            } else {
-                fallbackView();
-            }
-            break;
-        case "error-tx":
-            if (state.viewData && state.viewData.message) {
-                txStatus.renderError();
-            } else {
-                fallbackView();
-            }
-            break;
-        default:
-            fallbackView();
-            break;
+    if (!renderView(state.currentView, state, viewModules)) {
+        fallbackView();
     }
 }
 
@@ -247,7 +179,7 @@ async function init() {
         settings.show();
     });
 
-    setRenderMain(renderWalletList);
+    setBackRenderer(makeBackRenderer(state, viewModules));
 
     welcome.init(ctx);
     addWallet.init(ctx);

src/popup/viewRouter.js

@@ -0,0 +1,167 @@
+// Rendering a view the popup lands on without having navigated to it
+// forward: on restore, and on Back. In both cases the view may never have
+// been rendered in this page load — a reopened popup renders only the
+// wallet list and the view it restores onto, so every other view is still
+// the blank static template from index.html — so unhiding it is not enough.
+//
+// Forward navigation renders as it goes and must NOT come through here:
+// rendering a second time would re-fetch and clobber whatever the view has
+// in flight.
+//
+// The view modules are injected and nothing here touches the DOM, so the
+// dispatch and its data guards can be tested directly; src/popup/index.js
+// cannot be required outside a browser.
+
+const { RESTORABLE_VIEWS } = require("./restorableViews");
+
+// The views this page load has rendered.
+//
+// The Back path cannot otherwise tell its two cases apart. A view the popup
+// never rendered is still the blank template from index.html and has to be
+// rendered; a view already on the page must NOT be rendered again, because
+// a second render re-fetches and overwrites whatever the user has typed
+// into it and not yet saved.
+//
+// Registration is showView() in views/helpers.js, which is the last thing
+// every render path runs — restoreView()'s, the Back path's, and every
+// forward show(). That is the point of putting it there rather than in the
+// individual views: a view added later registers itself with no one having
+// to remember it, so this cannot decay.
+//
+// Module scope is page-load scope: the popup loads this module once per
+// page load, and a reopened popup gets a fresh, empty set — which is
+// exactly the state that makes the Back path render.
+const renderedViews = new Set();
+
+function markViewRendered(view) {
+    if (view) renderedViews.add(view);
+}
+
+// Begin a fresh page-load scope. The popup gets one by being loaded; the
+// unit tests, which simulate several page loads against one module
+// instance, ask for one.
+function resetRenderedViews() {
+    renderedViews.clear();
+}
+
+// Home is the exception: Back re-renders it every time, which is what the
+// popup did before this router existed (index.js registered
+// renderWalletList() as setRenderMain(), and goBack() called it on every
+// Back onto "main"). It must stay that way — the wallet list has to reflect
+// what changed while the user was away from it, such as a wallet renamed or
+// an address removed in Settings — and Home holds no unsaved input to lose.
+const ALWAYS_RENDER_ON_BACK = new Set(["main"]);
+
+// Views that render an address the user picked and cannot be rendered
+// without one.
+const ADDRESS_VIEWS = new Set([
+    "address",
+    "address-token",
+    "receive",
+    "transaction",
+]);
+
+function needsAddress(view) {
+    return ADDRESS_VIEWS.has(view);
+}
+
+function hasValidAddress(state) {
+    return Boolean(
+        state.selectedWallet !== null &&
+        state.selectedAddress !== null &&
+        state.wallets[state.selectedWallet] &&
+        state.wallets[state.selectedWallet].addresses[state.selectedAddress],
+    );
+}
+
+// Render `view` from persisted state. Each view module shows itself, so a
+// true return means the view is both rendered and on screen.
+//
+// Returns false when the view is not one the popup renders from state, or
+// when the state it would render is gone — a token no longer selected, a
+// transaction no longer persisted. The caller falls back rather than
+// putting an empty template on screen.
+function renderView(view, state, views) {
+    if (!view || !RESTORABLE_VIEWS.has(view)) return false;
+    if (needsAddress(view) && !hasValidAddress(state)) return false;
+    if (view === "address-token" && !state.selectedToken) return false;
+
+    const data = state.viewData || {};
+    switch (view) {
+        case "main":
+            views.main.show();
+            return true;
+        case "address":
+            views.addressDetail.show();
+            return true;
+        case "address-token":
+            views.addressToken.show();
+            return true;
+        case "receive":
+            views.receive.show();
+            return true;
+        case "settings":
+            views.settings.show();
+            return true;
+        case "settings-addtoken":
+            views.settingsAddToken.show();
+            return true;
+        case "confirm-tx":
+            if (!data.pendingTx) return false;
+            views.confirmTx.restore();
+            return true;
+        case "transaction":
+            if (!data.tx) return false;
+            views.transactionDetail.render();
+            return true;
+        case "wait-tx":
+            // Resumes the receipt poll from the persisted broadcast time,
+            // and answers false when there is nothing resumable left.
+            return Boolean(views.txStatus.restoreWait());
+        case "success-tx":
+            if (!data.hash) return false;
+            views.txStatus.renderSuccess();
+            return true;
+        case "error-tx":
+            if (!data.message) return false;
+            views.txStatus.renderError();
+            return true;
+        default:
+            return false;
+    }
+}
+
+// The Back-path renderer, registered with setBackRenderer() in
+// views/helpers.js.
+//
+// Returns false — leaving goBack() to unhide the view, as it always did —
+// in the two cases where the view is known to be on the page already:
+//
+//   - It is not one the popup renders from persisted state. The restored
+//     stack is filtered against RESTORABLE_VIEWS, so such a view can only
+//     be on the stack from this page load, where forward navigation
+//     rendered it on the way in.
+//   - This page load has rendered it. Re-rendering would re-fetch and
+//     clobber what it holds; Home is rendered anyway, see above.
+//
+// What is left is the case the router exists for: a view on the stack that
+// this page load has never rendered, whose template is still blank.
+function makeBackRenderer(state, views) {
+    return function renderBack(view) {
+        if (!RESTORABLE_VIEWS.has(view)) return false;
+        if (renderedViews.has(view) && !ALWAYS_RENDER_ON_BACK.has(view)) {
+            return false;
+        }
+        if (!renderView(view, state, views)) {
+            views.main.show();
+        }
+        return true;
+    };
+}
+
+module.exports = {
+    renderView,
+    makeBackRenderer,
+    markViewRendered,
+    resetRenderedViews,
+};

src/popup/views/helpers.js

@@ -7,6 +7,7 @@ const {
     getAddressValueUsd,
 } = require("../../shared/prices");
 const { state, saveState, currentNetwork } = require("../../shared/state");
+const { markViewRendered } = require("../viewRouter");
 
 // When views are added, removed, or transitions between them change,
 // update the view-navigation documentation in README.md to match.
@@ -76,6 +77,10 @@ function showView(name) {
     }
     clearFlash();
     state.currentView = name;
+    // A view's show() ends here, so this is where the Back path learns the
+    // view is no longer the blank template from index.html and must not be
+    // rendered a second time. See viewRouter.js.
+    markViewRendered(name);
     saveState();
     updateDebugBanner(name);
 }
@@ -111,12 +116,19 @@ function updateDebugBanner(viewName) {
     }
 }
 
-// Callback to re-render the main/home view when navigating back to it.
-// Set once by index.js via setRenderMain().
-let _renderMain = null;
+// Callback that renders a view being navigated BACK onto. Set once by
+// index.js via setBackRenderer(), which routes the view through the same
+// per-view render and data guards restoreView() uses.
+//
+// It answers true when it took the navigation — the view is rendered and
+// shown, or its backing data was gone and it fell back — and false for a
+// view the popup does not render from persisted state. Those can only be
+// on the stack from this page load, because the stack is filtered on load,
+// so they have already been rendered and only need unhiding.
+let _renderBack = null;
 
-function setRenderMain(fn) {
-    _renderMain = fn;
+function setBackRenderer(fn) {
+    _renderBack = fn;
 }
 
 // Push the current view onto the navigation stack so goBack() can
@@ -136,9 +148,11 @@ function goBack() {
     } else {
         target = "main";
     }
-    if (target === "main" && _renderMain) {
-        _renderMain();
-    }
+    // A popped view is landed on, not navigated to. If the popup has been
+    // closed and reopened since the view was pushed, nothing has ever
+    // rendered it in this page load and its template is still blank, so it
+    // has to be rendered here rather than merely unhidden.
+    if (_renderBack && _renderBack(target)) return;
     showView(target);
 }
 
@@ -470,7 +484,7 @@ module.exports = {
     showView,
     onViewLeave,
     updateDebugBanner,
-    setRenderMain,
+    setBackRenderer,
     pushCurrentView,
     goBack,
     clearViewStack,