diff --git a/README.md b/README.md index d6aeafd..f9bffb2 100644 --- a/README.md +++ b/README.md @@ -842,7 +842,12 @@ on ConfirmTx, DeleteWallet, ApproveTx and ApproveSign. - "Hide fake tokens impersonating a known symbol" checkbox - "Hide tokens with fewer than 1,000 holders" checkbox - "Hide transactions from detected fraud contracts" checkbox - - "Hide dust transactions below N gwei" checkbox + threshold input + - "Hide dust transactions below N gwei" checkbox + threshold input. The + threshold is plain decimal digits, a whole number of gwei, zero or + greater (zero hides nothing). Anything else — a fraction, a negative, + a value carrying its unit, hex (`0x10`) or exponent (`1e3`) notation — + is refused with a flash message and the field snaps back to the stored + threshold, so a number the user did not type is never stored. - Allowed Sites: list with remove buttons - Denied Sites: list with remove buttons - About: project link, license, author, version, release date, and the diff --git a/TODO.md b/TODO.md index 50a2de8..1f173c6 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,11 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: The dust threshold field now explains a rejection instead of + snapping back in silence, with the parse in a pure, unit-tested module that + accepts plain decimal digits only — hex and exponent notation are refused + rather than read as 16 and 1000 + ([#233](https://git.eeqj.de/sneak/AutistMask/issues/233)). - 2026-08-12: WaitTx lifecycle: a receipt and the 60-second timeout can no longer both render on one tick, no timer or in-flight lookup outlives its wait, a failed receipt lookup no longer counts as a timeout (but six in a row diff --git a/src/popup/dustThreshold.js b/src/popup/dustThreshold.js new file mode 100644 index 0000000..7b64c60 --- /dev/null +++ b/src/popup/dustThreshold.js @@ -0,0 +1,42 @@ +// Parsing for the dust threshold field in Settings. +// +// Pure: no DOM, no state, so the accepted set can be unit tested directly +// instead of through the settings view. +// +// Accepted input is plain decimal digits only, meaning a whole number of +// gwei, zero or greater. Zero is a real setting: it hides nothing. +// +// Deliberately rejected, not coerced: +// "" nothing to save +// "-1" a negative threshold has no meaning +// "1.5" fractional gwei is not a threshold the filter can use +// "100 gwei" the unit is already printed beside the field +// "0x10" hex, which Number() would silently read as 16 +// "1e3" exponent notation, which Number() would silently read as 1000 +// +// The last two are the reason this is a digit test and not a Number() test. +// Number() accepts both, and accepting them would put a number in the field +// that the user did not type — the same silent substitution the visible +// rejection message exists to end. + +// Must render on ONE line of #flash-msg, whose reserved height +// (min-h-[1.25rem]) is exactly one line at text-xs. A string long enough to +// wrap to two lines pushes the settings view down, which the No Layout Shift +// policy forbids. Do not lengthen this without re-running the layout test in +// tests/e2e/run.js, which measures the flash line and goes red on a shift. +const DUST_THRESHOLD_MESSAGE = + "Please enter a whole number of gwei, zero or greater."; + +// Returns the threshold in gwei, or null if the input is not one. +function parseDustThresholdGwei(raw) { + if (typeof raw !== "string") return null; + const trimmed = raw.trim(); + if (!/^[0-9]+$/.test(trimmed)) return null; + const val = Number(trimmed); + // A run of digits long enough to exceed Number's exact integer range + // would round on the way in, so it is not a threshold we can store. + if (!Number.isSafeInteger(val)) return null; + return val; +} + +module.exports = { DUST_THRESHOLD_MESSAGE, parseDustThresholdGwei }; diff --git a/src/popup/views/settings.js b/src/popup/views/settings.js index f3ca461..2d1baa3 100644 --- a/src/popup/views/settings.js +++ b/src/popup/views/settings.js @@ -9,6 +9,10 @@ const { pushCurrentView, } = require("./helpers"); const { applyTheme } = require("../theme"); +const { + DUST_THRESHOLD_MESSAGE, + parseDustThresholdGwei, +} = require("../dustThreshold"); const { state, saveState, currentNetwork } = require("../../shared/state"); const { NETWORKS, SUPPORTED_CHAIN_IDS } = require("../../shared/networks"); const { onChainSwitch } = require("../../shared/chainSwitch"); @@ -329,13 +333,14 @@ function init(ctx) { $("settings-dust-threshold").value = state.dustThresholdGwei; $("settings-dust-threshold").addEventListener("change", async () => { - const raw = $("settings-dust-threshold").value.trim(); - const val = Number(raw); - // 0 is accepted and means "hide nothing". Empty, negative, - // fractional and non-numeric input is rejected outright rather than - // coerced, and the field is put back to the stored threshold so it - // never shows a value the wallet is not using. - if (raw !== "" && Number.isInteger(val) && val >= 0) { + const val = parseDustThresholdGwei($("settings-dust-threshold").value); + // Rejected input is never coerced. The field is put back to the + // stored threshold so it never shows a value the wallet is not + // using, and the message says what the field wants so the snap-back + // is explained rather than silent. + if (val === null) { + showFlash(DUST_THRESHOLD_MESSAGE); + } else { state.dustThresholdGwei = val; await saveState(); } diff --git a/tests/dustThreshold.test.js b/tests/dustThreshold.test.js new file mode 100644 index 0000000..8a858d7 --- /dev/null +++ b/tests/dustThreshold.test.js @@ -0,0 +1,243 @@ +// Tests for the dust threshold field in Settings (issue #233). +// +// Two halves: what the parse accepts, and what the settings view does with a +// rejection. The view half runs against the real change handler with the DOM +// helpers stubbed out, because the bug was not in the parse — it was that a +// rejection said nothing. + +const { + DUST_THRESHOLD_MESSAGE, + parseDustThresholdGwei, +} = require("../src/popup/dustThreshold"); + +describe("parsing the dust threshold", () => { + test("accepts a whole number of gwei", () => { + expect(parseDustThresholdGwei("100000")).toBe(100000); + expect(parseDustThresholdGwei("1")).toBe(1); + }); + + // Zero is a real setting, not an empty field: it hides nothing. + test("accepts zero", () => { + expect(parseDustThresholdGwei("0")).toBe(0); + }); + + test("accepts surrounding whitespace", () => { + expect(parseDustThresholdGwei(" 250 ")).toBe(250); + }); + + test("rejects an empty field", () => { + expect(parseDustThresholdGwei("")).toBe(null); + expect(parseDustThresholdGwei(" ")).toBe(null); + }); + + test("rejects a negative threshold", () => { + expect(parseDustThresholdGwei("-1")).toBe(null); + }); + + // parseInt used to read this as 1, which is not what was typed. + test("rejects a fractional value", () => { + expect(parseDustThresholdGwei("1.5")).toBe(null); + expect(parseDustThresholdGwei("1.0")).toBe(null); + }); + + // parseInt used to read this as 100. The unit is printed beside the + // field already. + test("rejects a value carrying its unit", () => { + expect(parseDustThresholdGwei("100 gwei")).toBe(null); + }); + + // Number() reads this as 16. Storing 16 for a field that was told to + // want a whole number of gwei would be the same silent substitution the + // message exists to end. + test("rejects hex notation", () => { + expect(parseDustThresholdGwei("0x10")).toBe(null); + }); + + // Number() reads this as 1000. + test("rejects exponent notation", () => { + expect(parseDustThresholdGwei("1e3")).toBe(null); + }); + + test("rejects other non-numeric input", () => { + expect(parseDustThresholdGwei("lots")).toBe(null); + expect(parseDustThresholdGwei("+5")).toBe(null); + expect(parseDustThresholdGwei("Infinity")).toBe(null); + expect(parseDustThresholdGwei(undefined)).toBe(null); + expect(parseDustThresholdGwei(5)).toBe(null); + }); + + // Beyond 2^53 the digits would round on the way in, so the stored + // threshold would not be the one typed. + test("rejects a value too large to hold exactly", () => { + expect(parseDustThresholdGwei("9007199254740993")).toBe(null); + }); +}); + +describe("the rejection message", () => { + // README, Language & Labeling: error messages are full sentences. + test("is a full sentence naming the constraint", () => { + expect(DUST_THRESHOLD_MESSAGE).toMatch(/^[A-Z].*\.$/); + expect(DUST_THRESHOLD_MESSAGE).toContain("whole number of gwei"); + expect(DUST_THRESHOLD_MESSAGE).toContain("zero or greater"); + }); +}); + +describe("the flash line the message is shown in", () => { + const fs = require("fs"); + const path = require("path"); + + const POPUP_HTML = fs.readFileSync( + path.join(__dirname, "..", "src", "popup", "index.html"), + "utf8", + ); + + // This asserts only that the reservation exists in the markup. It does + // NOT and CANNOT assert that the message fits inside it: jest runs on + // the node environment here, with no layout engine, so every rendered + // height is zero. An earlier version of this block claimed to pin the + // No Layout Shift policy with this regex, and it passed at any message + // length, including one that wrapped to two lines and pushed the + // settings view down 12px. + // + // The assertion that actually measures — empty line vs. the message, + // real Chromium, documented 360x600 popup — is + // "a rejected dust threshold shifts no layout (#233)" in + // tests/e2e/run.js, run by make test-e2e. It is not in make check + // because REPO_POLICIES.md caps make test at 20 seconds and a browser + // suite does not fit; run it before changing the wording. + test("reserves its height in the markup", () => { + const flashLine = POPUP_HTML.match( + / { + let elements; + let flashes; + let saves; + let state; + + // A stand-in for one DOM node: enough of an element for init() to set + // properties on it and hang listeners off it. + function fakeElement() { + return { + value: "", + checked: false, + textContent: "", + href: "", + style: {}, + dataset: {}, + classList: { add() {}, remove() {} }, + listeners: {}, + addEventListener(event, handler) { + this.listeners[event] = handler; + }, + querySelectorAll: () => [], + }; + } + + function loadSettingsView() { + elements = {}; + flashes = []; + saves = 0; + + jest.resetModules(); + + jest.doMock("../src/popup/views/helpers", () => ({ + $: (id) => (elements[id] ||= fakeElement()), + showView: () => {}, + updateDebugBanner: () => {}, + showFlash: (msg) => flashes.push(msg), + escapeHtml: (s) => s, + flashCopyFeedback: () => {}, + goBack: () => {}, + pushCurrentView: () => {}, + onViewLeave: () => {}, + VIEWS: [], + })); + + state = require("../src/shared/state").state; + state.dustThresholdGwei = 100000; + + const settings = require("../src/popup/views/settings"); + settings.init({}); + return elements["settings-dust-threshold"]; + } + + beforeEach(() => { + globalThis.chrome = { + runtime: { sendMessage: () => {} }, + storage: { + local: { + get: async () => ({}), + set: async () => { + saves++; + }, + }, + }, + }; + }); + + afterEach(() => { + jest.dontMock("../src/popup/views/helpers"); + delete globalThis.chrome; + }); + + async function change(field, typed) { + field.value = typed; + await field.listeners.change(); + } + + test("a valid value is stored and says nothing", async () => { + const field = loadSettingsView(); + + await change(field, "250"); + + expect(state.dustThresholdGwei).toBe(250); + expect(field.value).toBe(250); + expect(flashes).toEqual([]); + expect(saves).toBe(1); + }); + + test("a rejected value shows the message and is not stored", async () => { + const field = loadSettingsView(); + + await change(field, "1.5"); + + expect(state.dustThresholdGwei).toBe(100000); + expect(flashes).toEqual([DUST_THRESHOLD_MESSAGE]); + expect(saves).toBe(0); + }); + + // The snap-back is the behaviour the message explains, so it stays. + test("a rejected value still resyncs the field to what is stored", async () => { + const field = loadSettingsView(); + + await change(field, "100 gwei"); + + expect(field.value).toBe(100000); + }); + + test("every rejected notation gets the same one message", async () => { + for (const typed of ["", "-1", "1.5", "100 gwei", "0x10", "1e3"]) { + const field = loadSettingsView(); + + await change(field, typed); + + expect(flashes).toEqual([DUST_THRESHOLD_MESSAGE]); + expect(state.dustThresholdGwei).toBe(100000); + } + }); + + test("zero is accepted, not treated as an empty field", async () => { + const field = loadSettingsView(); + + await change(field, "0"); + + expect(state.dustThresholdGwei).toBe(0); + expect(flashes).toEqual([]); + }); +}); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index bbdd3b5..37f8169 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -19,6 +19,7 @@ const { visible, } = require("./harness"); const { STUB_TOKEN, STUB_TX_HASH } = require("./network"); +const { DUST_THRESHOLD_MESSAGE } = require("../../src/popup/dustThreshold"); const TEST_TIMEOUT_MS = 120000; @@ -398,6 +399,135 @@ test("reopening the popup never lands on the phrase screen (#161)", async (env) assertWiped(st, env.phrase, "after reopening the popup"); }); +// ------------------------------------------------ dust threshold (#233) + +// The popup size README documents the UI as designed for. Pages in this +// context otherwise get Playwright's 1280x720 default, at which the flash +// line has room for any plausible message and never wraps — measuring +// there would pass for every string and prove nothing. +const POPUP_VIEWPORT = { width: 360, height: 600 }; + +// Everything below the flash line that must not move when it fills, plus +// the height of the line itself. Runs in the page. +// +// Positions are in document coordinates, not viewport coordinates: +// tabbing out of the field to fire "change" scrolls the popup, and a +// getBoundingClientRect().top read across that scroll reports a thousand +// pixels of movement that is the scroll, not a layout shift. +function measureFlashLine() { + const top = (id) => + document.getElementById(id).getBoundingClientRect().top + + window.scrollY; + return { + text: document.getElementById("flash-msg").textContent, + flashHeight: document + .getElementById("flash-msg") + .getBoundingClientRect().height, + settingsTop: top("view-settings"), + fieldTop: top("settings-dust-threshold"), + }; +} + +// Polling one evaluate() rather than waitForFunction() plus a second +// round trip to measure: showFlash() clears the line again after 2s, and +// measuring in a separate call can land after that and read an empty +// line — which would pass however long the message is. Here the text +// check and the geometry come from the same page task, so what is +// measured is always the filled line. Missing the 2s window entirely +// throws; it cannot go green. +async function waitForFilledFlashLine(page) { + const deadline = Date.now() + 15000; + for (;;) { + const m = await page.evaluate(measureFlashLine); + if (m.text.length > 0) return m; + if (Date.now() > deadline) { + throw new Error("the flash line never filled"); + } + await sleep(25); + } +} + +// README, No Layout Shift: the rejection message goes into #flash-msg, +// whose min-h-[1.25rem] reserves exactly ONE line at text-xs. Reserving +// the space is not enough on its own — a message too long for one line +// wraps and pushes everything below it down anyway, which is what the +// first version of this change shipped: 75 characters, 32px, the settings +// view and the threshold field 12px lower than with an empty line. +// +// So this measures rather than inspects markup. It is the only assertion +// in the repo that can see the wording grow: the unit suite runs on the +// node environment with no layout engine, where every height is zero (see +// the note in tests/dustThreshold.test.js). Lengthen +// DUST_THRESHOLD_MESSAGE past one line and this test goes red. +test("a rejected dust threshold shifts no layout (#233)", async (env) => { + const page = await openPopup(env.ctx, env.popupUrl); + try { + await page.setViewportSize(POPUP_VIEWPORT); + await openSettings(page); + + const before = await page.evaluate(measureFlashLine); + assert( + before.text === "", + "the flash line was not empty at the baseline measurement: " + + JSON.stringify(before.text), + ); + + // "change" fires on blur, not on typing, so fill() alone is not + // enough — it only dispatches "input". + await page.fill("#settings-dust-threshold", "1.5"); + await page.locator("#settings-dust-threshold").press("Tab"); + + const after = await waitForFilledFlashLine(page); + + // Printed pass or fail: the numbers are the evidence, and a + // silent assertion would leave the reader taking this on trust. + console.log( + "# dust threshold flash: " + + after.text.length + + " chars, line height " + + before.flashHeight + + " -> " + + after.flashHeight + + ", view-settings top " + + before.settingsTop + + " -> " + + after.settingsTop + + ", field top " + + before.fieldTop + + " -> " + + after.fieldTop, + ); + + assert( + after.text === DUST_THRESHOLD_MESSAGE, + "the field flashed something other than DUST_THRESHOLD_MESSAGE: " + + JSON.stringify(after.text), + ); + assert( + after.flashHeight === before.flashHeight, + "the message does not fit the reserved line: " + + before.flashHeight + + "px empty vs " + + after.flashHeight + + "px with the message. Shorten DUST_THRESHOLD_MESSAGE", + ); + assert( + after.settingsTop === before.settingsTop, + "the settings view moved " + + (after.settingsTop - before.settingsTop) + + "px when the message appeared", + ); + assert( + after.fieldTop === before.fieldTop, + "the dust threshold field moved " + + (after.fieldTop - before.fieldTop) + + "px when the message appeared", + ); + } finally { + await page.close(); + } +}); + // ---------------------------------------------------------------- runner async function main() {