fix: render the view "Back" lands on after the popup is reopened (closes #268)
All checks were successful
check / check (push) Successful in 55s

goBack() only unhid its target. A reopened popup renders the wallet list
and the one view restoreView() lands on, so every other view is still the
blank static template from index.html: pressing Back from Settings onto an
address showed an empty address line and no balances, and the same held for
address-token, receive, confirm-tx and transaction.

The per-view dispatch and its data guards move out of restoreView() into
src/popup/viewRouter.js, and goBack() now routes a popped view through the
same code by way of a renderer index.js registers with setBackRenderer().
A view whose backing state is gone falls back to Home the way the restore
does, rather than showing an empty template.

The Back path renders only a view this page load has not rendered yet.
viewRouter.js keeps a page-load-scoped set of rendered views, written by
showView() — the last thing every render path runs, forward navigation and
the restore alike, so a view added later registers itself rather than
needing to be remembered. Back onto a view already in that set unhides it
and nothing more, exactly as it does for a view outside RESTORABLE_VIEWS:
rendering a second time would re-fetch and overwrite what the view holds,
such as an edit typed into Settings and not yet saved. Home stays the
exception and is re-rendered on every Back, as it was when goBack() called
the renderWalletList() registered through setRenderMain(), so the wallet
list reflects what changed while the user was away from it.

tests/backNavigation.test.js drives the real goBack() over the reproduction
and over each of address-token, receive, confirm-tx, transaction, success-tx
and error-tx, with and without their backing state, and pins that a view
this page load already rendered — a live-session view, or Settings revisited
— is only unhidden, while Home still re-renders. tests/e2e/run.js adds three
cases against the real popup in a real browser, because make check cannot
see a blank view: a real close and reopen then Back onto the address screen
and onto Receive, and an in-session Back onto Settings that must keep
unsaved input. All three were demonstrated failing first.
This commit is contained in:
2026-08-12 10:41:26 +00:00
committed by sneak
parent 9dcd875dd4
commit 588d5fd8bd
7 changed files with 738 additions and 98 deletions

View File

@@ -0,0 +1,329 @@
// Back after reopening the popup (#268).
//
// A reopened popup renders the wallet list and the one view it restores
// onto; every other view is still the blank static template from
// index.html. goBack() used to only unhide its target, so Back landed on
// that blank template for any view the popup had not rendered in this page
// load. These tests drive the real goBack() with the real router wired to
// recording view modules, so what is asserted is which view render ran —
// the thing that was missing.
//
// The rendering itself is asserted against the real popup in a real
// browser by tests/e2e/run.js; here the DOM is a stub, because goBack()
// only needs showView() to work.
const els = new Map();
function fakeEl() {
return {
textContent: "",
innerHTML: "",
classList: {
toggle() {},
add() {},
remove() {},
contains: () => false,
},
remove() {},
};
}
globalThis.document = {
getElementById(id) {
if (!els.has(id)) els.set(id, fakeEl());
return els.get(id);
},
};
// helpers.js pulls in state.js, which reads chrome.storage.local at load.
globalThis.chrome = {
storage: { local: { get: async () => ({}), set: async () => {} } },
};
const {
showView,
goBack,
setBackRenderer,
pushCurrentView,
} = require("../src/popup/views/helpers");
const {
makeBackRenderer,
markViewRendered,
resetRenderedViews,
} = require("../src/popup/viewRouter");
const { state } = require("../src/shared/state");
const ADDRESS = "0x1111111111111111111111111111111111111111";
const TOKEN = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";
let calls;
// Stand-ins for the view modules. Each records itself and then shows its
// view, which is what every real view render ends with — so the assertions
// can tell "rendered and shown" apart from "merely unhidden".
function recorder(name, view) {
return () => {
calls.push(name);
showView(view);
};
}
function makeViews() {
return {
main: { show: recorder("main", "main") },
addressDetail: { show: recorder("addressDetail", "address") },
addressToken: { show: recorder("addressToken", "address-token") },
receive: { show: recorder("receive", "receive") },
settings: { show: recorder("settings", "settings") },
settingsAddToken: {
show: recorder("settingsAddToken", "settings-addtoken"),
},
confirmTx: { restore: recorder("confirmTx", "confirm-tx") },
transactionDetail: {
render: recorder("transactionDetail", "transaction"),
},
txStatus: {
restoreWait: () => {
calls.push("waitTx");
showView("wait-tx");
return true;
},
renderSuccess: recorder("successTx", "success-tx"),
renderError: recorder("errorTx", "error-tx"),
},
};
}
// The popup as it stands just after a reopen: one wallet with one address,
// the view the popup restored onto, and the stack behind it.
//
// A reopen is a fresh page load, so the record of what has been rendered
// starts empty — that emptiness is what makes the Back path render at all.
// Returns the view modules so a test can drive forward navigation through
// the same recorders the router renders through.
function reopenedOn(view, stack, extra) {
calls = [];
resetRenderedViews();
state.wallets = [
{
name: "Wallet 1",
addresses: [{ address: ADDRESS, balance: "0", tokenBalances: [] }],
},
];
state.selectedWallet = 0;
state.selectedAddress = 0;
state.selectedToken = null;
state.viewData = null;
state.currentView = view;
state.viewStack = stack.slice();
Object.assign(state, extra || {});
// Restoring onto a view renders it, so the reopened popup has that one
// view on the page and nothing else.
markViewRendered(view);
const views = makeViews();
setBackRenderer(makeBackRenderer(state, views));
return views;
}
// The reproduction from the issue, step for step.
describe("Back onto a view the reopened popup never rendered", () => {
test("Back from settings renders the address detail underneath", () => {
reopenedOn("settings", ["main", "address"]);
goBack();
expect(calls).toEqual(["addressDetail"]);
expect(state.currentView).toBe("address");
expect(state.viewStack).toEqual(["main"]);
});
test("Back onto the token detail renders it", () => {
reopenedOn("settings", ["main", "address", "address-token"], {
selectedToken: TOKEN,
});
goBack();
expect(calls).toEqual(["addressToken"]);
expect(state.currentView).toBe("address-token");
});
test("Back onto Receive renders it", () => {
reopenedOn("settings", ["main", "address", "receive"]);
goBack();
expect(calls).toEqual(["receive"]);
expect(state.currentView).toBe("receive");
});
test("Back onto the transaction detail renders it", () => {
reopenedOn("settings", ["main", "transaction"], {
viewData: { tx: { hash: "0xdead" } },
});
goBack();
expect(calls).toEqual(["transactionDetail"]);
expect(state.currentView).toBe("transaction");
});
test("Back onto the transaction confirmation restores it", () => {
reopenedOn("settings", ["main", "confirm-tx"], {
viewData: { pendingTx: { to: ADDRESS, amount: "1" } },
});
goBack();
expect(calls).toEqual(["confirmTx"]);
expect(state.currentView).toBe("confirm-tx");
});
test("Back onto the success screen renders it", () => {
reopenedOn("settings", ["main", "success-tx"], {
viewData: { hash: "0xdead" },
});
goBack();
expect(calls).toEqual(["successTx"]);
expect(state.currentView).toBe("success-tx");
});
test("Back onto the failure screen renders it", () => {
reopenedOn("settings", ["main", "error-tx"], {
viewData: { message: "execution reverted" },
});
goBack();
expect(calls).toEqual(["errorTx"]);
expect(state.currentView).toBe("error-tx");
});
test("Back onto Home renders the wallet list", () => {
reopenedOn("settings", ["main"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("Back with an empty stack renders Home", () => {
reopenedOn("settings", []);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
});
// The guards are restoreView()'s, so a popped view whose backing data is
// gone lands on Home rather than on an empty template.
describe("Back onto a view whose backing data is gone", () => {
test("the token detail with no token selected falls back to Home", () => {
reopenedOn("settings", ["main", "address-token"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("the transaction detail with no transaction falls back to Home", () => {
reopenedOn("settings", ["main", "transaction"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("the confirmation with no pending transaction falls back to Home", () => {
reopenedOn("settings", ["main", "confirm-tx"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("an address view with no address selected falls back to Home", () => {
reopenedOn("settings", ["main", "receive"], {
selectedAddress: null,
});
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("the success screen with no transaction hash falls back to Home", () => {
reopenedOn("settings", ["main", "success-tx"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("the failure screen with no message falls back to Home", () => {
reopenedOn("settings", ["main", "error-tx"]);
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
test("a wait that can no longer be resumed falls back to Home", () => {
reopenedOn("settings", ["main", "wait-tx"]);
const views = makeViews();
views.txStatus.restoreWait = () => false;
setBackRenderer(makeBackRenderer(state, views));
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
});
// Forward navigation renders as it goes, and a second render would re-fetch
// and clobber whatever the view holds — an unsaved edit, a request in
// flight. So the Back path renders only a view this page load has never
// rendered, and merely unhides every other one: the views it does not
// render from persisted state, and the views already on the page.
describe("what the Back path leaves alone", () => {
test("forward navigation renders nothing by itself", () => {
reopenedOn("address", ["main"]);
pushCurrentView();
showView("send");
expect(calls).toEqual([]);
expect(state.viewStack).toEqual(["main", "address"]);
});
test("Back onto a live-session view only unhides it", () => {
reopenedOn("confirm-tx", ["main", "address", "send"]);
goBack();
expect(calls).toEqual([]);
expect(state.currentView).toBe("send");
});
test("Back renders its target exactly once", () => {
reopenedOn("settings", ["main", "address"]);
goBack();
expect(calls.filter((c) => c === "addressDetail")).toHaveLength(1);
});
test("Back onto a view this page load already rendered only unhides it", () => {
const views = reopenedOn("main", []);
pushCurrentView();
views.addressDetail.show();
pushCurrentView();
views.settings.show();
calls = [];
goBack();
expect(calls).toEqual([]);
expect(state.currentView).toBe("address");
});
// The unit mirror of the regression the browser suite pins: Settings
// reassigns its fields from persisted state on every render, so a
// re-render on the way back discards an edit the user has not saved.
test("Back onto Settings visited earlier in this page load does not re-render it", () => {
const views = reopenedOn("main", []);
pushCurrentView();
views.settings.show();
pushCurrentView();
views.settingsAddToken.show();
calls = [];
goBack();
expect(calls).toEqual([]);
expect(state.currentView).toBe("settings");
});
// Home is the deliberate exception, unchanged from the popup's
// behaviour before the router existed: it re-renders on every Back so
// the wallet list reflects what changed while the user was away.
test("Back onto Home renders it again even when it is already on the page", () => {
const views = reopenedOn("main", []);
pushCurrentView();
views.addressDetail.show();
calls = [];
goBack();
expect(calls).toEqual(["main"]);
expect(state.currentView).toBe("main");
});
});

View File

@@ -419,6 +419,172 @@ test("reopening the popup never lands on the phrase screen (#161)", async (env)
assertWiped(st, env.phrase, "after reopening the popup");
});
// ------------------------------- Back after reopening the popup (#268)
// A reopened popup renders the wallet list and the view it restores onto,
// and nothing else: every other screen is still the blank static template
// from index.html. Back used to only unhide its target, which is why these
// have to run against the real popup — the template is present and
// well-formed, so only its emptiness distinguishes the defect, and only a
// real reopen produces it.
// Everything the address screen must have on it, read out of the DOM.
function addressScreenState(page) {
return page.evaluate(() => {
const line = document.getElementById("address-line");
const balances = document.getElementById("address-balances");
return {
hidden: document
.getElementById("view-address")
.classList.contains("hidden"),
line: line ? line.innerText.trim() : "",
balances: balances ? balances.innerText.trim() : "",
};
});
}
// Close and reopen the page rather than reload it: that is what the toolbar
// popup does, and it is the only thing that produces the unrendered views.
async function reopenPopup(env, restoredView) {
await env.page.close();
env.page = await openPopup(env.ctx, env.popupUrl);
await visible(env.page, restoredView);
}
// The reproduction from the issue, step for step.
test("Back after reopening the popup renders the address screen (#268)", async (env) => {
await openAddressDetail(env.page);
const before = await addressScreenState(env.page);
assert(
before.line.length > 0,
"the address screen was blank to begin with",
);
await env.page.click("#btn-settings");
await visible(env.page, "#view-settings");
await reopenPopup(env, "#view-settings");
await env.page.click("#btn-settings-back");
await visible(env.page, "#view-address");
const after = await addressScreenState(env.page);
assert(
after.line === before.line,
"the address line reads " +
JSON.stringify(after.line) +
", expected " +
JSON.stringify(before.line),
);
assert(
after.balances.includes("ETH"),
"the balances read " + JSON.stringify(after.balances),
);
});
// The same defect one screen further in. Receive holds the address twice
// over — as text and as the QR code the sender scans — and a blank one is
// worse than a missing screen.
// Everything the Receive screen must have on it. The QR code is read as
// pixels, not as an element: the blank template carries the canvas too, a
// default 300x150 one with nothing drawn on it and every pixel fully
// transparent. A drawn QR paints an opaque background across the whole
// canvas, so a single opaque pixel is the whole question.
function receiveScreenState(page) {
return page.evaluate(() => {
const block = document.getElementById("receive-address-block");
const canvas = document.getElementById("receive-qr");
const px = canvas
.getContext("2d")
.getImageData(0, 0, canvas.width, canvas.height).data;
let opaque = 0;
for (let i = 3; i < px.length; i += 4) {
if (px[i] > 0) opaque += 1;
}
return {
address: block.dataset.full || "",
text: block.innerText.trim(),
qrOpaquePixels: opaque,
};
});
}
test("Back after reopening the popup renders the Receive screen (#268)", async (env) => {
await openAddressDetail(env.page);
await env.page.click("#btn-receive");
await visible(env.page, "#view-receive");
const before = await receiveScreenState(env.page);
assert(
/^0x[0-9a-fA-F]{40}$/.test(before.address),
"Receive showed no address to begin with: " +
JSON.stringify(before.address),
);
await env.page.click("#btn-settings");
await visible(env.page, "#view-settings");
await reopenPopup(env, "#view-settings");
await env.page.click("#btn-settings-back");
await visible(env.page, "#view-receive");
const shown = await receiveScreenState(env.page);
assert(
shown.address === before.address,
"Receive shows " +
JSON.stringify(shown.address) +
", expected " +
JSON.stringify(before.address),
);
assert(
shown.text.includes(before.address),
"the Receive address is not on screen: " + JSON.stringify(shown.text),
);
assert(shown.qrOpaquePixels > 0, "Receive shows an unpainted QR code");
// Leave the suite where it found it.
await env.page.click("#btn-receive-back");
await visible(env.page, "#view-address");
await env.page.click("#btn-address-back");
await visible(env.page, "#view-main");
});
// The other half of the requirement: Back renders a screen this page load
// never rendered, and must NOT re-render one it already has on screen.
// settings.show() reassigns #settings-rpc from persisted state, so
// re-rendering Settings on the way back would silently revert whatever the
// user typed and had not saved yet — and they could then press Save and
// store the value they believed they had replaced. No reopen here: this is
// an ordinary in-session forward-and-back, which is exactly why the render
// must not happen.
test("Back onto Settings keeps unsaved input (#268)", async (env) => {
await visible(env.page, "#view-main");
await env.page.click("#btn-settings");
await visible(env.page, "#view-settings");
const typed = "https://rpc.example.invalid/unsaved";
await env.page.fill("#settings-rpc", typed);
await env.page.click("#btn-settings-add-token");
await visible(env.page, "#view-settings-addtoken");
await env.page.click("#btn-settings-addtoken-back");
await visible(env.page, "#view-settings");
const kept = await env.page.inputValue("#settings-rpc");
assert(
kept === typed,
"the unsaved RPC URL reads " +
JSON.stringify(kept) +
", expected " +
JSON.stringify(typed),
);
// Leave the suite where it found it. The typed value was never saved,
// and Settings reloads the field from state next time it renders.
await env.page.click("#btn-settings-back");
await visible(env.page, "#view-main");
});
// -------------------------------------------- address removal (#162)
// Number of address rows across every wallet in the list, counted in the DOM