fix: Settings ignores an endpoint check once the same "Save" is tapped again (closes #698)

Each RPC and Blockscout "Save" in Settings now counts its taps. A check
that answers after a newer tap of the same "Save" stores nothing and
leaves the flash message alone. Before this change, the older check's
error or refusal replaced the newer tap's "Saved.".

The save also checks for a newer tap at two points. A tap made while the
save waits for the lock means nothing is stored. A tap made during the
write means no flash message is shown.

Model: opus-5-5

README.md

@@ -2282,6 +2282,8 @@ view would leave a wallet one click from deletion.
     - Either "Save" stores its URL only if the input still shows that URL when
       the endpoint answers. Otherwise a flash message says that the URL was not
       saved
+    - If the user taps the same "Save" again before the endpoint answers, the
+      earlier tap stores nothing and shows no flash message
     - Etherscan:
         - "Check recipients on Etherscan" checkbox, unticked until the user
           ticks it

src/popup/views/settings.js

@@ -50,6 +50,11 @@ let versionClickTimer = null;
 // not the one in `state`.
 let shownNetwork = null;
 
+// Each endpoint "Save" counts its taps here, under the field that it stores. A
+// tap stores its URL and shows a flash message only while it is the latest tap
+// of its "Save".
+const saveTaps = { rpcUrl: 0, blockscoutUrl: 0 };
+
 // One row per site origin, however many addresses it appears under, each with
 // an [x] that hands it to onRemove.
 function renderSiteList(containerId, origins, onRemove) {
@@ -347,10 +352,16 @@ function showNewSettings() {
 // shows `url`, nothing is saved. The flash message then says that the URL was
 // changed during the check. The save compares the input and sets `url` in
 // `state` in one step. So a redraw after the comparison shows `url`.
-async function saveEndpoint(field, input, url, networkId) {
+//
+// `tap` is the number of the "Save" tap that checked `url`. If the user taps
+// the same "Save" again before this save holds the lock, nothing is stored. If
+// the user taps the same "Save" again before the write ends, no flash message
+// is shown.
+async function saveEndpoint(field, input, url, networkId, tap) {
     let refused = false;
     let inputChanged = false;
     await saveState((stored) => {
+        if (saveTaps[field] !== tap) return false;
         refused =
             state.networkId !== networkId || stored.networkId !== networkId;
         inputChanged = $(input).value.trim() !== url;
@@ -358,6 +369,7 @@ async function saveEndpoint(field, input, url, networkId) {
         state[field] = url;
         stored[field] = url;
     });
+    if (saveTaps[field] !== tap) return;
     if (refused) {
         showFlash("The URL was not saved because the network changed.");
     } else if (inputChanged) {
@@ -375,6 +387,8 @@ function init(_ctx) {
     showPhrase.init();
 
     $("btn-save-rpc").addEventListener("click", async () => {
+        saveTaps.rpcUrl++;
+        const tap = saveTaps.rpcUrl;
         const url = $("settings-rpc").value.trim();
         if (!url) {
             showFlash("Please enter an RPC URL.");
@@ -395,6 +409,8 @@ function init(_ctx) {
                 }),
             });
             const json = await resp.json();
+            // The user has tapped this "Save" again since the check began.
+            if (tap !== saveTaps.rpcUrl) return;
             if (json.error) {
                 log.errorf("RPC validation error:", json.error);
                 showFlash("Endpoint returned an error.");
@@ -408,6 +424,7 @@ function init(_ctx) {
             // Cancelled by the popup closing, not failed: see pageClosed in
             // src/popup/index.js.
             if (ctx.pageClosed.aborted) return;
+            if (tap !== saveTaps.rpcUrl) return;
             // Not the error's message: fetch puts the whole URL, password and
             // key included, in the message of the error it throws for a URL
             // with a user name and password or one it cannot parse.
@@ -415,10 +432,12 @@ function init(_ctx) {
             showFlash("Could not reach endpoint.");
             return;
         }
-        await saveEndpoint("rpcUrl", "settings-rpc", url, net.id);
+        await saveEndpoint("rpcUrl", "settings-rpc", url, net.id, tap);
     });
 
     $("btn-save-blockscout").addEventListener("click", async () => {
+        saveTaps.blockscoutUrl++;
+        const tap = saveTaps.blockscoutUrl;
         const url = $("settings-blockscout").value.trim();
         if (!url) {
             showFlash("Please enter a Blockscout API URL.");
@@ -428,18 +447,26 @@ function init(_ctx) {
         showFlash("Testing endpoint...");
         try {
             const resp = await debugFetch(url + "/stats");
+            if (tap !== saveTaps.blockscoutUrl) return;
             if (!resp.ok) {
                 showFlash("Endpoint returned HTTP " + resp.status + ".");
                 return;
             }
         } catch {
             if (ctx.pageClosed.aborted) return;
+            if (tap !== saveTaps.blockscoutUrl) return;
             // Not the error's message, as for the RPC check above.
             log.errorf("Blockscout validation failed:", urlOrigin(url));
             showFlash("Could not reach endpoint.");
             return;
         }
-        await saveEndpoint("blockscoutUrl", "settings-blockscout", url, net.id);
+        await saveEndpoint(
+            "blockscoutUrl",
+            "settings-blockscout",
+            url,
+            net.id,
+            tap,
+        );
     });
 
     // Each change is made inside the save, holding the lock. So

src/shared/chainSwitch.js

@@ -53,31 +53,18 @@ function clearPopupCaches() {
 // change. So the switch also resets these fields in the stored record:
 //
 // - every address's balance and token balances;
-// - the time of the last balance refresh;
+// - the time of the last balance refresh.
-// - the fraud contracts.
 //
-// A balance or a fraud contract that the background or another page stored
+// A balance that the background or another page stored since this page's last
-// since this page's last save is then not kept under the new network
+// save is then not kept under the new network
-// (https://git.eeqj.de/sneak/AutistMask/issues/653,
+// (https://git.eeqj.de/sneak/AutistMask/issues/653).
-// https://git.eeqj.de/sneak/AutistMask/issues/692).
-//
-// Before the switch, `state` takes the tracked tokens that another page stored
-// since this page's last save (takeStoredTrackedTokens() in state.js). The
-// switch then puts them away under the previous network, the network they were
-// added on. When the stored record is on another network, a switch saved
-// elsewhere has not reached this page yet. The stored record's tokens are then
-// that network's, and none are taken.
 async function onChainSwitch(newNetworkId) {
-    const { state, saveState, takeStoredTrackedTokens } = require("./state");
+    const { state, saveState } = require("./state");
     let net = null;
     await saveState((stored) => {
         if (state.networkId === newNetworkId) return false;
-        if (stored.networkId === state.networkId) {
-            takeStoredTrackedTokens(stored);
-        }
         net = applyChainSwitchFields(state, newNetworkId);
         stored.lastBalanceRefresh = 0;
-        stored.fraudContracts = [];
         for (const wallet of stored.wallets) {
             for (const addr of wallet.addresses) {
                 addr.balance = "0";

src/shared/state.js

@@ -126,10 +126,10 @@ function currentNetwork() {
 }
 
 // The persisted fields as this page held them when its last loadState()
-// finished, or when its last successful saveState() began, with what this page
+// finished, or when its last successful saveState() began, with any network
-// has taken from the stored record since (takeStoredNetwork() and
+// switch saved elsewhere that this page has taken since (takeStoredNetwork()).
-// takeStoredTrackedTokens()). saveState() diffs the live state against this to
+// saveState() diffs the live state against this to find only the fields THIS
-// find only the fields THIS page changed since.
+// page changed since.
 //
 // Deep-cloned, not a reference: callers mutate persisted objects and arrays
 // in place (state.wallets.push(...)), and a reference baseline would mutate
@@ -615,28 +615,6 @@ function takeStoredNetwork(stored) {
     copyChainSwitchFields(baseline, structuredClone(stored));
 }
 
-// Give this page the tracked tokens of `stored`, a record on this page's
-// network that a save of this page has just read (onChainSwitch() in
-// src/shared/chainSwitch.js calls this from `beforeWrite`):
-//
-// - A token that another page tracked or removed since this page's last save
-//   is tracked or removed in `state` too.
-// - A token that this page tracked or removed since then stays as this page
-//   has it.
-//
-// `baseline` takes the stored list, so the save stores the list in `state`
-// whenever the two differ.
-function takeStoredTrackedTokens(stored) {
-    rawState.trackedTokens = mergeListByIdentity(
-        baseline.trackedTokens,
-        rawState.trackedTokens,
-        stored.trackedTokens,
-        (token) => token.address.toLowerCase(),
-        (base, ours) => ours,
-    );
-    baseline.trackedTokens = structuredClone(stored.trackedTokens);
-}
-
 // Rejects with StateUnusableError for a stored record this build cannot make
 // sense of. Nothing is assigned and `loaded` stays false in that case, so a
 // caller that ignores the rejection gets StateNotLoadedError on the first
@@ -683,7 +661,6 @@ module.exports = {
     saveState,
     onSaveFailure,
     takeStoredNetwork,
-    takeStoredTrackedTokens,
     loadState,
     currentAddress,
     currentNetwork,

tests/settingsEndpointSave.test.js

@@ -167,3 +167,227 @@ test.each([
         );
     },
 );
+
+// This answer passes both endpoint checks on Ethereum Mainnet.
+const PASSING_ANSWER = {
+    ok: true,
+    json: async () => ({ result: NETWORKS.mainnet.chainId }),
+};
+
+// Both endpoint checks read this answer as an error. The RPC check reads the
+// answer's `error`. The Blockscout check reads the answer's `ok`.
+const ERROR_ANSWER = {
+    ok: false,
+    status: 500,
+    json: async () => ({ error: { code: -32603, message: "Internal error" } }),
+};
+
+// The user types a URL in Settings and taps "Save". Before the endpoint answers,
+// the user types a second URL and taps "Save" again. The second check passes
+// first. The second URL is stored, and the flash message reads "Saved.". The
+// first check then ends in one of three ways:
+//
+// - It passes.
+// - It fails, because fetch cannot reach the endpoint.
+// - It gets an answer that holds an error.
+//
+// None of the three changes the stored URL or the flash message
+// (https://git.eeqj.de/sneak/AutistMask/issues/698).
+test.each([
+    {
+        button: "btn-save-rpc",
+        input: "settings-rpc",
+        field: "rpcUrl",
+        first: "passes",
+    },
+    {
+        button: "btn-save-rpc",
+        input: "settings-rpc",
+        field: "rpcUrl",
+        first: "fails",
+    },
+    {
+        button: "btn-save-rpc",
+        input: "settings-rpc",
+        field: "rpcUrl",
+        first: "answers with an error",
+    },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+        first: "passes",
+    },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+        first: "fails",
+    },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+        first: "answers with an error",
+    },
+])(
+    "$button keeps the second tap's result when the first check $first last",
+    async ({ button, input, field, first }) => {
+        const page = await bootPopup(unversionedValidProfile());
+        await page.click("btn-settings");
+        const checks = [];
+        globalThis.fetch.mockImplementation(
+            () =>
+                new Promise((resolve, reject) => {
+                    checks.push({ resolve, reject });
+                }),
+        );
+        const secondUrl = "https://second.example.invalid";
+
+        page.node(input).value = "https://first.example.invalid";
+        const firstTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        page.node(input).value = secondUrl;
+        const secondTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        checks[1].resolve(PASSING_ANSWER);
+        await Promise.all(secondTap);
+        await page.settle();
+        expect(page.text("flash-msg")).toBe("Saved.");
+        if (first === "passes") {
+            checks[0].resolve(PASSING_ANSWER);
+        } else if (first === "answers with an error") {
+            checks[0].resolve(ERROR_ANSWER);
+        } else {
+            // fetch rejects when it cannot reach the endpoint.
+            checks[0].reject(new TypeError("Failed to fetch"));
+        }
+        await Promise.all(firstTap);
+        await page.settle();
+
+        expect(checks).toHaveLength(2);
+        expect(page.storage.read("autistmask")[field]).toBe(secondUrl);
+        expect(page.text("flash-msg")).toBe("Saved.");
+    },
+);
+
+// The user types a URL in Settings and taps "Save". Another write holds the
+// lock. So the save waits for the lock once the check passes. The user then
+// taps "Save" again on the same URL. When the lock is free, the first save
+// stores nothing and shows no flash message. The second check then passes, and
+// the second save stores the URL.
+test.each([
+    { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+    },
+])(
+    "$button stores nothing for an earlier tap still waiting for the lock",
+    async ({ button, input, field }) => {
+        const page = await bootPopup(unversionedValidProfile());
+        await page.click("btn-settings");
+        const storedUrl = page.value(input);
+        const checks = [];
+        globalThis.fetch.mockImplementation(
+            () =>
+                new Promise((resolve) => {
+                    checks.push(resolve);
+                }),
+        );
+        const typedUrl = "https://typed.example.invalid";
+
+        page.node(input).value = typedUrl;
+        const firstTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        let releaseLock;
+        const lockHeld = navigator.locks.request(
+            "autistmask",
+            () =>
+                new Promise((resolve) => {
+                    releaseLock = resolve;
+                }),
+        );
+        checks[0](PASSING_ANSWER);
+        await page.settle();
+        const secondTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        releaseLock();
+        await lockHeld;
+        await Promise.all(firstTap);
+        await page.settle();
+
+        expect(page.storage.read("autistmask")[field]).toBe(storedUrl);
+        expect(page.text("flash-msg")).toBe("Testing endpoint...");
+
+        checks[1](PASSING_ANSWER);
+        await Promise.all(secondTap);
+        await page.settle();
+
+        expect(checks).toHaveLength(2);
+        expect(page.storage.read("autistmask")[field]).toBe(typedUrl);
+        expect(page.text("flash-msg")).toBe("Saved.");
+    },
+);
+
+// The user taps "Save", and the check passes. The save then begins its storage
+// write, and this test keeps the write waiting. The user taps "Save" again, and
+// the second check cannot reach the endpoint. When the write ends, the flash
+// message still reads "Could not reach endpoint.". The write stores the URL
+// anyway, because the write began before the second tap.
+test.each([
+    { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+    },
+])(
+    "$button keeps the second tap's message when tapped again during the write",
+    async ({ button, input, field }) => {
+        const page = await bootPopup(unversionedValidProfile());
+        await page.click("btn-settings");
+        const checks = [];
+        globalThis.fetch.mockImplementation(
+            () =>
+                new Promise((resolve, reject) => {
+                    checks.push({ resolve, reject });
+                }),
+        );
+        const typedUrl = "https://typed.example.invalid";
+
+        page.node(input).value = typedUrl;
+        const firstTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        const { set } = page.storage.local;
+        const write = set.getMockImplementation();
+        let writeBegan = false;
+        let releaseWrite;
+        const writeHeld = new Promise((resolve) => {
+            releaseWrite = resolve;
+        });
+        set.mockImplementationOnce(async (items) => {
+            writeBegan = true;
+            await writeHeld;
+            await write(items);
+        });
+        checks[0].resolve(PASSING_ANSWER);
+        await page.settle();
+        expect(writeBegan).toBe(true);
+        const secondTap = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        // fetch rejects when it cannot reach the endpoint.
+        checks[1].reject(new TypeError("Failed to fetch"));
+        await Promise.all(secondTap);
+        await page.settle();
+        expect(page.text("flash-msg")).toBe("Could not reach endpoint.");
+        releaseWrite();
+        await Promise.all(firstTap);
+        await page.settle();
+
+        expect(checks).toHaveLength(2);
+        expect(page.storage.read("autistmask")[field]).toBe(typedUrl);
+        expect(page.text("flash-msg")).toBe("Could not reach endpoint.");
+    },
+);

tests/trackedTokensPerNetwork.test.js

@@ -5,9 +5,6 @@
 // a contract lookup that answers after one. Driven through the real popup over
 // a stored profile; the same switch made by the background is in
 // tests/coldWorkerChainSwitch.test.js.
-//
-// The tests of what another page stored before a switch in Settings cover a
-// fraud contract too (https://git.eeqj.de/sneak/AutistMask/issues/692).
 
 const {
     bootPopup,
@@ -181,8 +178,9 @@ test("a token added by contract address on Sepolia is tracked on Sepolia only",
 });
 
 // Another popup page tracks DAI on Ethereum Mainnet after this page's last
-// save. The switch to Sepolia puts DAI away under Ethereum Mainnet.
+// save. The switch's save writes only the token lists that this page changed.
-test("a token another page tracked before a switch in Settings stays tracked on Ethereum Mainnet", async () => {
+// So DAI stays in `trackedTokens`, the active network's list.
+test("a token another page tracked before a switch in Settings stays in trackedTokens", async () => {
     const boot = await bootPopup(unversionedValidProfile());
     expect(boot.pageErrors).toEqual([]);
     boot.storage.write("autistmask", {
@@ -195,51 +193,10 @@ test("a token another page tracked before a switch in Settings stays tracked on
 
     const stored = boot.storage.read("autistmask");
     expect(stored.networkId).toBe("sepolia");
-    expect(stored.trackedTokens).toEqual([]);
+    expect(stored.trackedTokens).toEqual([MAINNET_TOKEN]);
-    expect(stored.networkTrackedTokens.mainnet).toEqual([MAINNET_TOKEN]);
-});
-
-// Another popup page stops tracking DAI on Ethereum Mainnet after this page's
-// last save. This page still holds DAI. The switch to Sepolia does not store
-// DAI under Ethereum Mainnet again.
-test("a token another page removed before a switch in Settings stays removed", async () => {
-    const boot = await bootPopup(
-        unversionedValidProfile({ trackedTokens: [MAINNET_TOKEN] }),
-    );
-    expect(boot.pageErrors).toEqual([]);
-    boot.storage.write("autistmask", {
-        ...boot.storage.read("autistmask"),
-        trackedTokens: [],
-    });
-
-    await boot.click("btn-settings");
-    await chooseNetwork(boot, "sepolia");
-
-    const stored = boot.storage.read("autistmask");
-    expect(stored.networkId).toBe("sepolia");
-    expect(stored.trackedTokens).toEqual([]);
     expect(stored.networkTrackedTokens.mainnet).toEqual([]);
 });
 
-// Another popup page records a fraud contract on Ethereum Mainnet after this
-// page's last save. The switch to Sepolia drops that fraud contract, as the
-// switch drops this page's own.
-test("a fraud contract another page recorded before a switch in Settings is not kept on Sepolia", async () => {
-    const boot = await bootPopup(unversionedValidProfile());
-    expect(boot.pageErrors).toEqual([]);
-    boot.storage.write("autistmask", {
-        ...boot.storage.read("autistmask"),
-        fraudContracts: [NEW_TOKEN_ADDRESS],
-    });
-
-    await boot.click("btn-settings");
-    await chooseNetwork(boot, "sepolia");
-
-    const stored = boot.storage.read("autistmask");
-    expect(stored.networkId).toBe("sepolia");
-    expect(stored.fraudContracts).toEqual([]);
-});
-
 // The contract lookup on SettingsAddToken answers on Ethereum Mainnet while the
 // switch to Sepolia waits for the lock. The page adds the token to Ethereum
 // Mainnet's list. The switch then keeps that list for Ethereum Mainnet.