fix: a token lookup that answers after the user left AddToken no longer changes the screen (closes #603)

When the contract lookup on AddToken answered, the popup always went to AddressDetail, even after the user had opened another screen. The token is still added when the network has not changed. The popup now goes to AddressDetail only while AddToken is the screen showing. It tells this from state.currentView, as Send does. The status line is cleared once the token is added, so Back onto AddToken no longer shows "Looking up token...".

Model: opus-5-5

Dockerfile

@@ -10,7 +10,6 @@ WORKDIR /app
 
 COPY script/ script/
 COPY package.json yarn.lock ./
-# yarn warns about the workspaces field in eslint and babel-plugin-istanbul; no newer release drops it.
 RUN script/bootstrap
 
 COPY . .

eslint.config.js

@@ -153,7 +153,8 @@ module.exports = [
         },
     },
 
-    // The files in script/lib/ run on plain node too, and are never bundled.
+    // The helpers the script/ entrypoints call: plain node programs too, run
+    // from a shell script rather than from yarn, and never bundled.
     {
         files: ["script/lib/**/*.js"],
         languageOptions: {

package.json

@@ -6,8 +6,8 @@
     "license": "GPL-3.0",
     "private": true,
     "scripts": {
-        "test": "node script/lib/jest.js --maxWorkers=3",
-        "test:verbose": "node script/lib/jest.js --maxWorkers=3 --verbose",
+        "test": "jest --forceExit --maxWorkers=3",
+        "test:verbose": "jest --forceExit --maxWorkers=3 --verbose",
         "build": "node build.js",
         "lint": "eslint . && prettier --check .",
         "fmt": "prettier --write .",

script/bootstrap

@@ -118,7 +118,6 @@ ensure_yarn() {
     fi
 }
 
-# yarn warns about the workspaces field in eslint and babel-plugin-istanbul; no newer release drops it.
 install_js_deps() {
     if missing yarn && [ -s "$HOME/.nvm/nvm.sh" ]; then
         nvm_sh "nvm use $NODE_VERSION >/dev/null && cd \"$ROOT\" && \

script/lib/jest.js

@@ -1,40 +0,0 @@
-// Runs jest with the arguments it is given, and fails the run when jest had to
-// force-stop the worker process that ran a test file. Jest itself only prints a
-// warning when that happens, and its run still passes. The warning means a test
-// left a timer or another open handle running. The test scripts in package.json
-// run jest through this file.
-//
-// Jest runs inside this process and writes to the terminal directly, so its
-// output looks the same as a plain jest run. This file only checks each write
-// to stderr for the warning below.
-
-"use strict";
-
-// The start of the warning that jest prints on stderr after it force-stops a
-// worker process.
-const WORKER_WARNING = "A worker process has failed to exit gracefully";
-
-let forceStopped = false;
-
-const writeStderr = process.stderr.write.bind(process.stderr);
-process.stderr.write = (chunk, ...rest) => {
-    if (String(chunk).includes(WORKER_WARNING)) {
-        forceStopped = true;
-    }
-    return writeStderr(chunk, ...rest);
-};
-
-// Runs after jest has finished, as the process exits.
-process.on("exit", () => {
-    if (forceStopped) {
-        console.error(
-            "script/lib/jest.js: failing the run, because jest force-stopped " +
-                "a worker process. A test left a timer or another open " +
-                "handle running.",
-        );
-        process.exitCode = 1;
-    }
-});
-
-// Jest's own command-line program. It reads its arguments from process.argv.
-require("jest/bin/jest");

src/popup/views/addToken.js

@@ -5,12 +5,7 @@ const { lookupTokenInfo } = require("../../shared/balances");
 const { isScamAddress } = require("../../shared/scamlist");
 const { log } = require("../../shared/log");
 
-// Counts the times AddToken has opened. A contract lookup started before
-// AddToken was last opened changes neither the screen nor the status line.
-let addTokenScreenOpenings = 0;
-
 function show() {
-    addTokenScreenOpenings++;
     $("add-token-address").value = "";
     $("add-token-info").textContent = "";
     $("add-token-info").style.visibility = "hidden";
@@ -60,7 +55,6 @@ function init(ctx) {
         // The network the lookup asks, which Settings may switch away from
         // before it answers, as for a transaction in confirmTx.js.
         const chainId = currentNetwork().chainId;
-        const opening = addTokenScreenOpenings;
         try {
             const info = await lookupTokenInfo(
                 contractAddr,
@@ -86,15 +80,11 @@ function init(ctx) {
             });
             await saveState();
             ctx.doRefreshAndRender();
-            // If the lookup answers after AddToken was opened again, the
-            // token is still added, but the screen and the status line stay
-            // as they are.
-            if (addTokenScreenOpenings !== opening) return;
             // The lookup is over, so a later Back onto AddToken shows no
             // status.
             infoEl.textContent = "";
             infoEl.style.visibility = "hidden";
-            // If the lookup answers while another screen shows, the token is
+            // If the user left AddToken while the lookup ran, the token is
             // still added but the screen does not change.
             if (state.currentView !== "add-token") return;
             // Pop the stack (back to address detail) and re-render it
@@ -117,7 +107,6 @@ function init(ctx) {
                     ? detail
                     : "Could not add the token.",
             );
-            if (addTokenScreenOpenings !== opening) return;
             infoEl.textContent = "";
             infoEl.style.visibility = "hidden";
         }

tests/addTokenLateAnswer.test.js

@@ -18,9 +18,6 @@ const TOKEN = {
     name: "Test Token",
 };
 
-// A second contract address, typed after AddToken was opened again.
-const OTHER_TOKEN_ADDRESS = "0xBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-
 afterEach(() => {
     cleanupPopup();
 });
@@ -75,7 +72,7 @@ test("an answer after the user opened Settings adds the token and Settings stays
     expect(boot.storage.read("autistmask").trackedTokens).toEqual([TOKEN]);
 });
 
-test("Back from Settings onto AddToken after the lookup answered shows no lookup status", async () => {
+test("Back onto AddToken after that answer shows no lookup status", async () => {
     const boot = await openAddToken();
     const answerLookup = addWithPendingLookup(boot);
     expect(boot.text("add-token-info")).toBe("Looking up token...");
@@ -88,44 +85,6 @@ test("Back from Settings onto AddToken after the lookup answered shows no lookup
     expect(boot.node("add-token-info").style.visibility).toBe("hidden");
 });
 
-test("an answer after AddToken was opened again adds the token and leaves AddToken and its status line as they are", async () => {
-    const boot = await openAddToken();
-    const answerFirstLookup = addWithPendingLookup(boot);
-    await boot.click("btn-add-token-back");
-    await boot.click("btn-add-token");
-    expect(boot.visibleViews()).toEqual(["add-token"]);
-    boot.node("add-token-address").value = OTHER_TOKEN_ADDRESS;
-    // The second lookup is never answered.
-    addWithPendingLookup(boot);
-
-    await answerFirstLookup();
-    expect(boot.visibleViews()).toEqual(["add-token"]);
-    expect(boot.text("add-token-info")).toBe("Looking up token...");
-    expect(boot.storage.read("autistmask").trackedTokens).toEqual([TOKEN]);
-});
-
-test("a lookup that fails after AddToken was opened again leaves the status line as it is", async () => {
-    const boot = await openAddToken();
-    const { lookupTokenInfo } = require("../src/shared/balances");
-    let failFirstLookup;
-    lookupTokenInfo.mockImplementationOnce(
-        () =>
-            new Promise((_, reject) => {
-                failFirstLookup = reject;
-            }),
-    );
-    const firstAdd = boot.click("btn-add-token-confirm");
-    await boot.click("btn-add-token-back");
-    await boot.click("btn-add-token");
-    boot.node("add-token-address").value = OTHER_TOKEN_ADDRESS;
-    // The second lookup is never answered.
-    addWithPendingLookup(boot);
-
-    failFirstLookup(new Error("lookup failed"));
-    await firstAdd;
-    expect(boot.text("add-token-info")).toBe("Looking up token...");
-});
-
 test("an answer while AddToken still shows goes to AddressDetail with the token listed", async () => {
     const boot = await openAddToken();
     const answerLookup = addWithPendingLookup(boot);

tests/addressKey.test.js

@@ -161,7 +161,7 @@ function click(element) {
 
 // show() starts the fee estimate without awaiting it; this lets it settle.
 async function settle() {
-    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
+    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
 }
 
 // ------------------------------------------------------------ harness
@@ -188,11 +188,7 @@ let sent;
 // handler to the buttons it drew, as the browser's fresh list would have.
 let plusButton;
 
-// showFlash() starts a timer that clears the message one or two seconds later.
-// Each test here runs on fake timers and drops them in afterEach, so that timer
-// never outlives the test.
 beforeEach(() => {
-    jest.useFakeTimers();
     approvalDetails = null;
     sent = [];
     mockNode.broadcast = [];
@@ -243,7 +239,6 @@ beforeEach(() => {
 });
 
 afterEach(() => {
-    jest.useRealTimers();
     jest.restoreAllMocks();
 });
 

tests/backgroundApproval.test.js

@@ -537,18 +537,7 @@ async function settleIncludingRejections() {
     await new Promise((resolve) => setImmediate(resolve));
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
     jest.resetModules();
 });
@@ -1894,6 +1883,18 @@ describe("a closed approval window leaves the request pending", () => {
         return bg.actionPopups[bg.actionPopups.length - 1];
     }
 
+    // The timer that keeps the worker running while an approval is pending is
+    // stubbed, so no test here leaves that timer running when it ends.
+    let keepRunningTimer;
+    beforeEach(() => {
+        keepRunningTimer = jest
+            .spyOn(global, "setInterval")
+            .mockImplementation(() => 1);
+    });
+    afterEach(() => {
+        keepRunningTimer.mockRestore();
+    });
+
     test("the toolbar popup opens a closed transaction approval, and approving it there sends", async () => {
         const bg = loadBackground();
         const pending = bg.requestTx();

tests/backgroundStateIsolation.test.js

@@ -103,18 +103,7 @@ function deferred() {
     return { promise, resolve };
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/chainSwitchGate.test.js

@@ -56,18 +56,7 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/coldWorkerChainSwitch.test.js

@@ -64,18 +64,7 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/coldWorkerSendTransaction.test.js

@@ -76,18 +76,7 @@ async function settle() {
     for (let i = 0; i < 60; i++) await Promise.resolve();
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/connectedSitesWorkerRestart.test.js

@@ -57,18 +57,7 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
-// While an approval is pending, the background starts a 20-second timer to
-// keep itself running. The timer is stubbed and never starts, so no test in
-// this file leaves it running.
-let keepRunningTimer;
-beforeEach(() => {
-    keepRunningTimer = jest
-        .spyOn(global, "setInterval")
-        .mockImplementation(() => 1);
-});
-
 afterEach(() => {
-    keepRunningTimer.mockRestore();
     delete global.chrome;
     jest.resetModules();
 });

tests/deleteWalletLostPassword.test.js

@@ -102,17 +102,6 @@ function makeDocument() {
 
 // ------------------------------------------------------------ harness
 
-// showFlash() starts a timer that clears the message two seconds later. Each
-// test here runs on fake timers and drops them in afterEach, so that timer
-// never outlives the test.
-beforeEach(() => {
-    jest.useFakeTimers();
-});
-
-afterEach(() => {
-    jest.useRealTimers();
-});
-
 function wallet(name, secret, addresses) {
     return {
         type: "hd",

tests/sendAmountRule.test.js

@@ -131,14 +131,10 @@ async function review(amount, token) {
 async function confirm(amount, token) {
     const txInfo = await review("0.5", token);
     confirmTx.show({ ...txInfo, amount });
-    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
+    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
 }
 
-// showFlash() starts a timer that clears the message two seconds later. Each
-// test here runs on fake timers and drops them in afterEach, so that timer
-// never outlives the test.
 beforeEach(() => {
-    jest.useFakeTimers();
     elements.clear();
     state.wallets = [
         {
@@ -171,10 +167,6 @@ beforeEach(() => {
     state.currentView = null;
 });
 
-afterEach(() => {
-    jest.useRealTimers();
-});
-
 describe.each([
     ["1e-3", "ETH"],
     ["1e-3", "TOK"],

tests/sendMax.test.js

@@ -277,18 +277,14 @@ async function review() {
 
 // show() starts the fee estimate without awaiting it; this lets it settle.
 async function settle() {
-    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
+    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
 }
 
 function canSend() {
     return !el("btn-confirm-send").disabled;
 }
 
-// showFlash() starts a timer that clears the message two seconds later. Each
-// test here runs on fake timers and drops them in afterEach, so that timer
-// never outlives the test.
 beforeEach(() => {
-    jest.useFakeTimers();
     elements.clear();
     log.errorf.mockClear();
     pageClosed = new AbortController();
@@ -301,10 +297,6 @@ beforeEach(() => {
     mockNode.broadcast = [];
 });
 
-afterEach(() => {
-    jest.useRealTimers();
-});
-
 describe("Max on an ETH send", () => {
     test("fills in the exact balance minus the fee reserve", async () => {
         await refreshWith(BALANCE_WEI);

tests/walletDefects.test.js

@@ -357,11 +357,6 @@ describe("every way to a signature or the private key refuses a defective wallet
     });
 
     beforeEach(() => {
-        // showFlash() starts a timer that clears the message two seconds
-        // later. Each test here runs on fake timers and drops them in
-        // afterEach, so that timer never outlives the test.
-        jest.useFakeTimers();
-
         const broken = brokenXprvWallet();
         // A balance, so that no Send button's zero-balance refusal can stand
         // in for the defect check.
@@ -403,7 +398,6 @@ describe("every way to a signature or the private key refuses a defective wallet
     });
 
     afterEach(() => {
-        jest.useRealTimers();
         state.wallets = [];
         state.activeAddress = null;
         state.selectedWallet = null;