fix: Back from Settings never lands on Settings itself (closes #481)

The recovery phrase and delete wallet screens take themselves off the
Back stack when left, so Settings, one of those screens, then the
settings gear leaves Settings under the Settings now showing. A reopened
popup restores the same stack, cut at the screen the gear left. Back
then showed Settings again and seemed to do nothing.

goBack() now skips any entry for the screen already showing before it
pops its target. Jest tests drive the gear and then Back for the
recovery phrase screen, once and twice over, for both delete screens,
and after a reopen.

Model: opus-5-5

README.md

@@ -277,7 +277,7 @@ provide:
   step's exit status, saying on stderr that it did and why. Every step of
   `make build` runs through it; `make build-debug` runs none of them through it.
   A step that succeeds removes nothing, and a removal that cannot be completed
-  is reported as loudly as one that was
+  is reported as loudly as one that was.
 - `script/test-verify-build` — exercise every failure mode of
   `script/verify-build` against a fixture tree in a temp dir, asserting the exit
   status and the message of each, assert the state of `dist/` on disk after a
@@ -1283,11 +1283,14 @@ behind a "···" menu.
 
 Navigation uses a stack model (like iOS): each forward action pushes the current
 screen onto `state.viewStack`, and "Back" pops it (`pushCurrentView()` and
-`goBack()` in `src/popup/views/helpers.js`). The root screen is either Welcome
-(no wallets) or Home (has wallets). Each screen below gives its view id in
-parentheses; the registry of view ids is the `VIEWS` array in
-`src/popup/views/helpers.js`, and the markup for a screen is the element with id
-`view-` plus that view id in `src/popup/index.html`.
+`goBack()` in `src/popup/views/helpers.js`). "Back" skips an entry for the
+screen already showing: ShowRecoveryPhrase and the two delete screens take
+themselves off the stack when left, so the Settings gear on one of them leaves
+Settings under Settings, and "Back" from there goes to the screen before
+Settings. The root screen is either Welcome (no wallets) or Home (has wallets).
+Each screen below gives its view id in parentheses; the registry of view ids is
+the `VIEWS` array in `src/popup/views/helpers.js`, and the markup for a screen
+is the element with id `view-` plus that view id in `src/popup/index.html`.
 
 Three elements sit outside the screens and are present on all of them: the title
 bar ("AutistMask by @sneak" plus the Settings gear), the flash message line

TODO.md

@@ -45,6 +45,29 @@ but the review is broader than any of them.
 
 # Completed Steps
 
+- 2026-10-07: Back from Settings no longer shows Settings again after the
+  settings gear was pressed on the recovery phrase or a delete wallet screen
+  opened from Settings, with or without a reopen in between
+  ([#481](https://git.eeqj.de/sneak/AutistMask/issues/481)). Those screens take
+  themselves off the Back stack when left, which leaves Settings under Settings;
+  `goBack()` now skips an entry for the screen already showing.
+  `tests/showPhrase.test.js`, `tests/deleteWalletLostPassword.test.js` and
+  `tests/backNavigation.test.js` drive each path.
+
+- 2026-10-07: `script/discard-dist-on-failure` returns the failed step's own
+  exit status even when it cannot write its message, to a closed stderr or to a
+  pipe nobody reads any more
+  ([#342](https://git.eeqj.de/sneak/AutistMask/issues/342)); it used to return 2
+  or 141 instead. An interrupt while a step runs now removes nothing and says
+  nothing whichever shell `/bin/sh` is, even when the step catches it and exits
+  with a status of its own, as `script/check-censored` does: the wrapper exits
+  with 130 once the step has ended. Under bash such a step used to have `dist/`
+  removed. A failed `check-censored --require-dist` still removes `dist/` like
+  any other step, because a `dist/` not cleared of the name `RULES.md` bars must
+  not ship. The header states both. `script/test-verify-build` runs the wrapper
+  with stderr closed and with stderr a pipe nobody reads, and interrupts it
+  under dash and under bash.
+
 - 2026-10-06: Back from Settings no longer lands on the delete wallet or
   lost-password screen after either was left by the settings gear
   ([#480](https://git.eeqj.de/sneak/AutistMask/issues/480)), the defect

script/discard-dist-on-failure

@@ -3,22 +3,21 @@
 # step fails, remove dist/ before returning its exit status. Our own extension
 # to scripts-to-rule-them-all, wrapped around every step of make build.
 #
-# Why: with AUTISTMASK_DEBUG=1 exported in the calling shell, make build
-# compiles a debug bundle and then fails on it in script/verify-build — but the
-# bundle is already written. It is loadable, and every wallet it creates gets
-# the publicly committed test recovery phrase from src/shared/constants.js. A
-# failed release build that leaves that behind is a smaller version of the trap
-# the verifier exists to close, and "the failure was loud" only works on an
-# operator who does not load dist/chrome/ anyway. Removing the artifact does not
-# depend on that.
+# Why: with AUTISTMASK_DEBUG=1 exported, make build compiles a debug bundle and
+# then fails on it in script/verify-build, after the bundle is written. It is
+# loadable, and every wallet it creates gets the publicly committed test
+# recovery phrase from src/shared/constants.js, so a failed build must not leave
+# it behind. The removal is never silent: it says on stderr that dist/ is gone
+# and why.
 #
-# Two things this deliberately does not do. It does not wrap make build-debug: a
-# debug build that failed is not a mistakable artifact, and its output is the
-# evidence of what went wrong. And it never removes anything on a step that
-# SUCCEEDS, including the final check-censored --require-dist pass.
-#
-# The removal is never silent: it says dist/ is gone and why, on stderr, above
-# the build's own failure.
+# A failed check-censored --require-dist removes dist/ like any other step: a
+# dist/ not cleared of the name RULES.md bars must not ship either. A step that
+# succeeds removes nothing. An interrupt (Ctrl-C) while a step runs removes
+# nothing and says nothing, even when the step catches it and exits with a
+# status of its own: the wrapper exits with 130 once the step has ended. An
+# interrupt is not a build failure, and whoever interrupted the build knows it
+# did not finish. make build-debug is not wrapped: a debug build that failed is
+# not a mistakable artifact, and its output is the evidence of what went wrong.
 set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
@@ -66,12 +65,20 @@ main() {
         exit 1
     }
 
+    # An interrupt is not a build failure: once the step has ended, exit
+    # without removing anything, even if the step caught the interrupt and
+    # exited with a status of its own. bash as /bin/sh would otherwise carry on.
+    trap 'exit 130' INT
+
     _status=0
     "$@" || _status=$?
 
     [ "$_status" -ne 0 ] || return 0
 
-    discard_dist
+    # A message that cannot be written, to a closed stderr or to a pipe nobody
+    # reads any more, must not replace the step's status.
+    trap '' PIPE
+    discard_dist || true
     exit "$_status"
 }
 

script/test-verify-build

@@ -915,6 +915,76 @@ run_cases() {
     discard_case "the wrapper given no command removes nothing" \
         c_control 1 kept "no command given" ""
 
+    # With stderr closed the wrapper cannot write its message, and must still
+    # remove dist/ and return the step's own status.
+    build_fixture
+    _status=0
+    (cd "$FIXTURE" &&
+        "$FIXTURE/script/discard-dist-on-failure" sh -c 'exit 6' 2>&-) ||
+        _status=$?
+    if [ "$_status" -eq 6 ] && [ ! -e "$FIXTURE/dist" ]; then
+        PASSED=$((PASSED + 1))
+        echo "  ok: a failed step's status survives a closed stderr"
+    else
+        FAILED=$((FAILED + 1))
+        echo "  FAIL: a failed step's status survives a closed stderr"
+        echo "    exit status $_status, wanted 6, and dist/ must be gone"
+    fi
+
+    # The same with stderr a pipe nobody reads any more, where the write would
+    # kill the wrapper with SIGPIPE. The FIFO's only reader opens it, exits and
+    # is waited for before the wrapper runs, so the pipe never has a reader.
+    build_fixture
+    mkfifo "$WORK/stderr-fifo"
+    _status=0
+    (
+        : <"$WORK/stderr-fifo" &
+        exec 3>"$WORK/stderr-fifo"
+        wait "$!"
+        cd "$FIXTURE" &&
+            "$FIXTURE/script/discard-dist-on-failure" sh -c 'exit 6' 2>&3
+    ) || _status=$?
+    if [ "$_status" -eq 6 ] && [ ! -e "$FIXTURE/dist" ]; then
+        PASSED=$((PASSED + 1))
+        echo "  ok: a failed step's status survives a pipe nobody reads"
+    else
+        FAILED=$((FAILED + 1))
+        echo "  FAIL: a failed step's status survives a pipe nobody reads"
+        echo "    exit status $_status, wanted 6, and dist/ must be gone"
+    fi
+
+    # An interrupt while a step runs removes nothing and says nothing, even
+    # when the step catches it and exits with a status of its own, as
+    # script/check-censored does. The step interrupts the wrapper and then
+    # itself, as Ctrl-C interrupts every process of the build at once. Run
+    # under dash and under bash, which /bin/sh may each be: bash carries on
+    # after such a step unless the wrapper stops it.
+    for _shell in dash bash; do
+        _name="an interrupt under $_shell removes nothing"
+        if ! command -v "$_shell" >/dev/null 2>&1; then
+            SKIPPED=$((SKIPPED + 1))
+            SKIPPED_NAMES="$SKIPPED_NAMES##   - $_name ($_shell not found)$NEWLINE"
+            echo "  SKIP ($_shell not found): $_name"
+            continue
+        fi
+        build_fixture
+        _status=0
+        _out="$(cd "$FIXTURE" && "$_shell" \
+            "$FIXTURE/script/discard-dist-on-failure" \
+            sh -c 'trap "exit 4" INT; kill -INT "$PPID" $$; exit 5' 2>&1)" ||
+            _status=$?
+        if [ "$_status" -eq 130 ] && [ -z "$_out" ] &&
+            [ -f "$FIXTURE/dist/chrome/src/popup/index.js" ]; then
+            PASSED=$((PASSED + 1))
+            echo "  ok: $_name"
+        else
+            FAILED=$((FAILED + 1))
+            echo "  FAIL: $_name"
+            echo "    exit status $_status, wanted 130; dist/ must be intact" \
+                "and nothing said. Output: $_out"
+        fi
+    done
+
     check_makefile_wiring
 }
 

src/popup/views/helpers.js

@@ -216,10 +216,21 @@ function pushCurrentView() {
 
 // Pop the navigation stack and show the previous view.  If the stack
 // is empty, fall back to the main (home) view.
+//
+// An entry for the view already showing is skipped: landing on it would
+// make Back seem to do nothing. Settings, the recovery phrase or delete
+// wallet screen, then the gear leaves Settings under Settings, because that
+// screen takes itself off the stack when left, and a reopened popup cuts it
+// off the restored stack the same way
+// (https://git.eeqj.de/sneak/AutistMask/issues/481).
 function goBack() {
+    const stack = state.viewStack;
+    while (stack.length > 0 && stack[stack.length - 1] === state.currentView) {
+        stack.pop();
+    }
     let target;
-    if (state.viewStack.length > 0) {
-        target = state.viewStack.pop();
+    if (stack.length > 0) {
+        target = stack.pop();
     } else {
         target = "main";
     }

tests/backNavigation.test.js

@@ -52,6 +52,7 @@ const {
     resetRenderedViews,
 } = require("../src/popup/viewRouter");
 const { state } = require("../src/shared/state");
+const { restorableStack } = require("../src/shared/persistedState");
 
 const ADDRESS = "0x1111111111111111111111111111111111111111";
 const TOKEN = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";
@@ -209,6 +210,23 @@ describe("Back onto a view the reopened popup never rendered", () => {
     });
 });
 
+// https://git.eeqj.de/sneak/AutistMask/issues/481. Settings, the recovery
+// phrase or delete wallet screen, the gear, then a reopen: the restored stack
+// is cut at the screen the gear left, which leaves Settings under the Settings
+// the popup reopens onto.
+describe("Back from Settings reopened over its own entry", () => {
+    test.each(["show-phrase", "delete-wallet-confirm"])(
+        "goes to the screen under it after leaving %s",
+        (left) => {
+            const stored = ["main", "settings", left];
+            reopenedOn("settings", restorableStack(stored, "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", () => {

tests/deleteWalletLostPassword.test.js

@@ -618,9 +618,11 @@ describe("the password route's confirm button", () => {
 
 // https://git.eeqj.de/sneak/AutistMask/issues/480: leaving either delete
 // screen drops its wallet selection, so Back onto one showed a screen whose
-// button could only answer "No wallet selected for deletion."
+// button could only answer "No wallet selected for deletion." Taking the
+// screen off the stack leaves Settings under Settings, and Back must not land
+// there either (https://git.eeqj.de/sneak/AutistMask/issues/481).
 describe("Back from Settings after leaving by the settings gear", () => {
-    test("does not land on the delete screen", () => {
+    test("goes past the delete screen to the screen under Settings", () => {
         const { helpers, deleteWallet, state } = load();
         deleteWallet.show(1);
         // The settings gear: push the current view, then show Settings.
@@ -629,10 +631,10 @@ describe("Back from Settings after leaving by the settings gear", () => {
 
         expect(state.viewStack).toEqual(["main", "settings"]);
         helpers.goBack();
-        expect(state.currentView).not.toBe("delete-wallet-confirm");
+        expect(state.currentView).toBe("main");
     });
 
-    test("does not land on the lost-password screen", async () => {
+    test("goes past the lost-password screen to the screen under Settings", async () => {
         const { helpers, deleteWallet, state } = load();
         await openLostPassword(deleteWallet, 1);
         // The settings gear: push the current view, then show Settings.
@@ -641,7 +643,7 @@ describe("Back from Settings after leaving by the settings gear", () => {
 
         expect(state.viewStack).toEqual(["main", "settings"]);
         helpers.goBack();
-        expect(state.currentView).not.toBe(VIEW);
+        expect(state.currentView).toBe("main");
     });
 
     // The lost-password screen's own Back is "Back returns to the delete

tests/showPhrase.test.js

@@ -131,8 +131,10 @@ describe("Back from Settings after leaving by the settings gear", () => {
 
     // https://git.eeqj.de/sneak/AutistMask/issues/461: leaving drops the
     // wallet selection, so Back onto this screen showed a password prompt
-    // that could only answer "No wallet is selected."
-    test("does not land on the recovery phrase screen", () => {
+    // that could only answer "No wallet is selected." Taking the screen off
+    // the stack leaves Settings under Settings, and Back must not land there
+    // either (https://git.eeqj.de/sneak/AutistMask/issues/481).
+    test("goes to the screen under Settings", () => {
         const { helpers, state, showPhrase } = load();
 
         // Opened from the wallet list in Settings, then left by the gear:
@@ -143,7 +145,21 @@ describe("Back from Settings after leaving by the settings gear", () => {
 
         expect(state.viewStack).toEqual(["main", "settings"]);
         helpers.goBack();
-        expect(state.currentView).not.toBe(SHOW_PHRASE_VIEW);
+        expect(state.currentView).toBe("main");
+    });
+
+    // Each round trip leaves one more Settings under Settings.
+    test("goes to the screen under Settings after two round trips", () => {
+        const { helpers, state, showPhrase } = load();
+        for (let i = 0; i < 2; i++) {
+            showPhrase.show(0);
+            helpers.pushCurrentView();
+            helpers.showView("settings");
+        }
+
+        expect(state.viewStack).toEqual(["main", "settings", "settings"]);
+        helpers.goBack();
+        expect(state.currentView).toBe("main");
     });
 
     // This Back takes Settings off the stack before the screen is left, so