From c6a1f9724766df0844955f2cd1266f77ffde80e1 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 11:29:09 +0200 Subject: [PATCH] fix: explain a rejected dust threshold instead of silently snapping back (closes #233) The dust-threshold field was the only validated input in Settings that rejected without saying anything: the value silently changed back to the stored one with no explanation. It now flashes "Please enter a whole number of gwei, zero or greater." alongside the existing resync, matching the idiom the RPC URL field already uses. The parse moves to its own module and accepts plain decimal digits only, zero or greater. Hex and exponent notation are refused rather than accepted: Number() reads "0x10" as 16 and "1e3" as 1000, neither of which the previous parseInt produced, and storing a number the user did not type is the same silent substitution this change exists to remove. The message must fit one line of the reserved flash area -- a wrapped message pushes the settings view down, which the No Layout Shift policy forbids. That is pinned by an end-to-end test measuring the rendered line height and the position of the elements below it, in a single round trip because the flash clears after two seconds. --- README.md | 7 +- TODO.md | 5 + src/popup/dustThreshold.js | 42 +++++++ src/popup/views/settings.js | 19 +-- tests/dustThreshold.test.js | 243 ++++++++++++++++++++++++++++++++++++ tests/e2e/run.js | 130 +++++++++++++++++++ 6 files changed, 438 insertions(+), 8 deletions(-) create mode 100644 src/popup/dustThreshold.js create mode 100644 tests/dustThreshold.test.js diff --git a/README.md b/README.md index f94a9de..0081049 100644 --- a/README.md +++ b/README.md @@ -857,7 +857,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 0e177a1..8094a90 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: Approval verification became an allowlist — transaction type restricted to 0/1/2 so an EIP-7702 delegation can no longer ride along on an approved transfer, every consequential field compared, the artifact 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 29a87f9..c6a4c8a 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; @@ -493,6 +494,135 @@ test("confirming removes the address and returns Home (#162)", async (env) => { ); }); +// ------------------------------------------------ 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() {