diff --git a/README.md b/README.md index e20ba2b..2ee60cc 100644 --- a/README.md +++ b/README.md @@ -2279,6 +2279,9 @@ view would leave a wallet one click from deletion. no user data and is made whether or not "Use Blockscout" is ticked. - If the network changes before either "Save" stores its URL, the URL is not saved and a flash message says so + - 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 - Etherscan: - "Check recipients on Etherscan" checkbox, unticked until the user ticks it diff --git a/src/popup/views/settings.js b/src/popup/views/settings.js index 4b52632..8dec61c 100644 --- a/src/popup/views/settings.js +++ b/src/popup/views/settings.js @@ -339,17 +339,31 @@ function showNewSettings() { // Another page may have stored a different URL since then. So `url` is set in // the stored record too. "Saved." then means that the stored record holds // `url`. -async function saveEndpoint(field, url, networkId) { +// +// `input` is the id of the input that the user typed `url` in. When another +// page changes the network, "Use Blockscout" or "Check recipients on +// Etherscan", this page draws Settings again. A redraw during the endpoint +// check puts the URL from `state` back in the input. So if the input no longer +// 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) { let refused = false; + let inputChanged = false; await saveState((stored) => { refused = state.networkId !== networkId || stored.networkId !== networkId; - if (refused) return false; + inputChanged = $(input).value.trim() !== url; + if (refused || inputChanged) return false; state[field] = url; stored[field] = url; }); if (refused) { showFlash("The URL was not saved because the network changed."); + } else if (inputChanged) { + showFlash( + "The URL was not saved because it was changed during the check.", + ); } else { showFlash("Saved."); } @@ -401,7 +415,7 @@ function init(_ctx) { showFlash("Could not reach endpoint."); return; } - await saveEndpoint("rpcUrl", url, net.id); + await saveEndpoint("rpcUrl", "settings-rpc", url, net.id); }); $("btn-save-blockscout").addEventListener("click", async () => { @@ -425,7 +439,7 @@ function init(_ctx) { showFlash("Could not reach endpoint."); return; } - await saveEndpoint("blockscoutUrl", url, net.id); + await saveEndpoint("blockscoutUrl", "settings-blockscout", url, net.id); }); // Each change is made inside the save, holding the lock. So diff --git a/tests/settingsEndpointSave.test.js b/tests/settingsEndpointSave.test.js index 066a06f..d770061 100644 --- a/tests/settingsEndpointSave.test.js +++ b/tests/settingsEndpointSave.test.js @@ -47,3 +47,123 @@ test.each([ expect(page.text("flash-msg")).toBe("Saved."); }, ); + +// The user types a URL in Settings and taps "Save". While the check waits for +// the endpoint's answer, another page turns "Use Blockscout" on. This page +// follows that change and draws Settings again, with the stored URL in the +// input. The check then passes. The typed URL is not stored. The flash message +// says that the URL was changed during the check +// (https://git.eeqj.de/sneak/AutistMask/issues/695). +test.each([ + { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" }, + { + button: "btn-save-blockscout", + input: "settings-blockscout", + field: "blockscoutUrl", + }, +])( + "$button stores nothing when Settings is drawn again during the check", + async ({ button, input, field }) => { + const page = await bootPopup(unversionedValidProfile()); + await page.click("btn-settings"); + const storedUrl = page.value(input); + let answer; + globalThis.fetch.mockReturnValue( + new Promise((resolve) => { + answer = resolve; + }), + ); + + page.node(input).value = "https://typed.example.invalid"; + const saving = page.node(button).listeners.click.map((fn) => fn()); + await page.settle(); + // The browser announces this write to this page. + await page.storage.local.set({ + autistmask: { + ...page.storage.read("autistmask"), + useBlockscout: true, + }, + }); + await page.settle(); + expect(page.node("settings-use-blockscout").checked).toBe(true); + // This answer passes both endpoint checks on Ethereum Mainnet. + answer({ + ok: true, + json: async () => ({ result: NETWORKS.mainnet.chainId }), + }); + await Promise.all(saving); + await page.settle(); + + expect(page.storage.read("autistmask")[field]).toBe(storedUrl); + expect(page.value(input)).toBe(storedUrl); + expect(page.text("flash-msg")).toBe( + "The URL was not saved because it was changed during the check.", + ); + }, +); + +// As above, but another write holds the lock. So this page's follow of the +// change to "Use Blockscout" waits for the lock. The endpoint answers while the +// input still shows the typed URL. The save then asks for the lock, after the +// follow did. So the follow draws Settings again before the save compares the +// input. +test.each([ + { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" }, + { + button: "btn-save-blockscout", + input: "settings-blockscout", + field: "blockscoutUrl", + }, +])( + "$button stores nothing when Settings is drawn again while the save waits for the lock", + async ({ button, input, field }) => { + const page = await bootPopup(unversionedValidProfile()); + await page.click("btn-settings"); + const storedUrl = page.value(input); + let answer; + globalThis.fetch.mockReturnValue( + new Promise((resolve) => { + answer = resolve; + }), + ); + const typedUrl = "https://typed.example.invalid"; + + page.node(input).value = typedUrl; + const saving = page.node(button).listeners.click.map((fn) => fn()); + await page.settle(); + let releaseLock; + const lockHeld = navigator.locks.request( + "autistmask", + () => + new Promise((resolve) => { + releaseLock = resolve; + }), + ); + // The browser announces this write to this page. + await page.storage.local.set({ + autistmask: { + ...page.storage.read("autistmask"), + useBlockscout: true, + }, + }); + await page.settle(); + // This answer passes both endpoint checks on Ethereum Mainnet. + answer({ + ok: true, + json: async () => ({ result: NETWORKS.mainnet.chainId }), + }); + await page.settle(); + expect(page.value(input)).toBe(typedUrl); + releaseLock(); + await lockHeld; + await Promise.all(saving); + await page.settle(); + + expect(page.node("settings-use-blockscout").checked).toBe(true); + expect(page.storage.read("autistmask")[field]).toBe(storedUrl); + expect(page.value(input)).toBe(storedUrl); + expect(page.text("flash-msg")).toBe( + "The URL was not saved because it was changed during the check.", + ); + }, +);