restoreView() refuses to reopen the popup ONTO a non-restorable view, but the
stack behind it was restored verbatim, so a screen the popup will not render — export-privkey, show-phrase — could sit in it. Back then landed on a view
the popup had declined to restore, and show-phrase has no Back control to
leave by.
What this is not: no secret is exposed. Those screens are empty precisely
because nothing is restored into them. This is a navigation defect, not a
disclosure one.
Scope
This fixes exactly the narrowed DoD: Back after reopening never lands on a view
the popup DECLINED TO RESTORE.
It does not make Back safe in general. goBack() unhides the popped view
without re-rendering it, so a restorable view sitting in the stack — address, receive, confirm-tx — still comes up as the blank static
template on a freshly reopened popup. That is independent of this filter and is
tracked as #268. The test an ordinary restorable stack is restored unchanged asserts the stack contents
only, and says so.
Truncate, not splice
loadState() truncates the stored stack at the first entry outside RESTORABLE_VIEWS — the entry and everything above it go.
The result is therefore always a prefix of what was stored, which is the whole
argument: every surviving entry keeps exactly the Back target it had before.
Splicing the bad entry out preserves more of the stack but silently re-points
the entry that sat above the hole at a different screen, so a Back press means
something new without anything visible having changed. Truncation also matches
how the stack was built: the entries above a dropped view were reached through it, so the path they describe no longer exists once it is gone. What
truncation loses is depth, and the cost of that is bounded — Back reaches the
root sooner — whereas the cost of splicing is unbounded: it can send Back
anywhere.
Never an empty dead end: when truncation leaves nothing and the restored view
is a restorable non-root view, main is put beneath it. (An empty stack
already sends goBack() to main; making it explicit means the invariant does
not depend on that fallback, and it is behaviourally identical for stacks that
were legitimately empty.) The stack stays empty only when the popup restores
onto main, or onto a view that is not restorable at all — in which case restoreView() will not reopen onto it and there is nothing to sit under.
A stored stack that is missing or is not an array now runs through that same
rule instead of returning early, so a corrupt stack does not fall back on the goBack() behaviour the explicit ["main"] exists in order not to depend on.
Note on what the ["main"] injection does NOT promise: several views are in RESTORABLE_VIEWS but still fall back to main at restore time when their
backing data is gone — address-token without selectedToken, confirm-tx
without a pending tx, transaction without a tx, a wait-tx whose resume
fails, any needsAddress view without a valid address. Those get ["main"]
injected and then land on main anyway, leaving main beneath main.
Harmless (Back from main is not reachable), but the stack is not empty in
that case and an earlier revision of this description wrongly said it was.
On load, not on save
Two reasons:
Only a load-side filter is retroactive. Filtering on save fixes stacks
written after this ships and leaves every stack already in storage exactly
as broken as it is today — including stacks written by a build where a view
was still restorable and has since left the set. Filtering on load means
the guarantee holds on the first read of any profile, whatever wrote it.
The live in-session stack is legitimate and must stay whole: while the
popup is open the user really is one Back away from a screen that is
rendered right now. The set is about what may be re-rendered from
persisted state, so it belongs at the point where state comes back from
storage.
saveState() is unchanged and still persists the live stack verbatim; a test
pins that.
Tests, failing first
New cases in tests/state.test.js cover a non-restorable view at the top, in
the middle and at the bottom of the stack, a name that is not a restorable view
at all, an ordinary restorable stack restored unchanged, restoring onto main,
restoring onto a view the popup will not reopen, a non-array stack under both a
restorable and a non-restorable current view, and the save path. Against
unmodified next six of them fail:
✕ a non-restorable view at the top of the stack is dropped
✕ a non-restorable view in the middle truncates the stack there
✕ a non-restorable view at the bottom leaves main to go back to
✕ no restored stack retains a secret-bearing view
✕ a name that is not a restorable view at all is dropped
✕ restoring onto main keeps the stack empty
Tests: 6 failed, 561 passed, 567 total
src/popup/restorableViews.js is untouched, so the existing assertions that export-privkey and show-phrase are absent and wait-tx is present continue
to hold and now also govern the stack.
Mutation pinned
The review found the RESTORABLE_VIEWS.has(currentView) clause of the
never-empty guard unpinned: dropping it left every test green. restoring onto a view the popup will not reopen keeps the stack empty now kills it. With the
clause removed:
✕ restoring onto a view the popup will not reopen keeps the stack empty
- Expected - 1
+ Received + 3
- Array []
+ Array [
+ "main",
+ ]
Tests: 1 failed, 586 passed, 587 total
With it restored: 587 passed, 587 total.
Verification
make check on the rebased branch: 25 suites / 587 tests, test-verify-build
18 cases, prettier clean. make test-e2e: 27/27 in the pinned container.
Closes [#224](https://git.eeqj.de/sneak/AutistMask/issues/224).
`restoreView()` refuses to reopen the popup ONTO a non-restorable view, but the
stack behind it was restored verbatim, so a screen the popup will not render —
`export-privkey`, `show-phrase` — could sit in it. Back then landed on a view
the popup had declined to restore, and `show-phrase` has no Back control to
leave by.
What this is not: no secret is exposed. Those screens are empty precisely
because nothing is restored into them. This is a navigation defect, not a
disclosure one.
## Scope
This fixes exactly the narrowed DoD: Back after reopening never lands on a view
the popup DECLINED TO RESTORE.
It does not make Back safe in general. `goBack()` unhides the popped view
without re-rendering it, so a *restorable* view sitting in the stack —
`address`, `receive`, `confirm-tx` — still comes up as the blank static
template on a freshly reopened popup. That is independent of this filter and is
tracked as [#268](https://git.eeqj.de/sneak/AutistMask/issues/268). The test
`an ordinary restorable stack is restored unchanged` asserts the stack contents
only, and says so.
## Truncate, not splice
`loadState()` truncates the stored stack at the first entry outside
`RESTORABLE_VIEWS` — the entry and everything above it go.
The result is therefore always a prefix of what was stored, which is the whole
argument: every surviving entry keeps exactly the Back target it had before.
Splicing the bad entry out preserves more of the stack but silently re-points
the entry that sat above the hole at a different screen, so a Back press means
something new without anything visible having changed. Truncation also matches
how the stack was built: the entries above a dropped view were reached
*through* it, so the path they describe no longer exists once it is gone. What
truncation loses is depth, and the cost of that is bounded — Back reaches the
root sooner — whereas the cost of splicing is unbounded: it can send Back
anywhere.
Never an empty dead end: when truncation leaves nothing and the restored view
is a restorable non-root view, `main` is put beneath it. (An empty stack
already sends `goBack()` to `main`; making it explicit means the invariant does
not depend on that fallback, and it is behaviourally identical for stacks that
were legitimately empty.) The stack stays empty only when the popup restores
onto `main`, or onto a view that is not restorable at all — in which case
`restoreView()` will not reopen onto it and there is nothing to sit under.
A stored stack that is missing or is not an array now runs through that same
rule instead of returning early, so a corrupt stack does not fall back on the
`goBack()` behaviour the explicit `["main"]` exists in order not to depend on.
Note on what the `["main"]` injection does NOT promise: several views are in
`RESTORABLE_VIEWS` but still fall back to `main` at restore time when their
backing data is gone — `address-token` without `selectedToken`, `confirm-tx`
without a pending tx, `transaction` without a tx, a `wait-tx` whose resume
fails, any `needsAddress` view without a valid address. Those get `["main"]`
injected and then land on `main` anyway, leaving `main` beneath `main`.
Harmless (Back from `main` is not reachable), but the stack is not empty in
that case and an earlier revision of this description wrongly said it was.
## On load, not on save
Two reasons:
1. Only a load-side filter is retroactive. Filtering on save fixes stacks
written after this ships and leaves every stack already in storage exactly
as broken as it is today — including stacks written by a build where a view
was still restorable and has since left the set. Filtering on load means
the guarantee holds on the first read of any profile, whatever wrote it.
2. The live in-session stack is legitimate and must stay whole: while the
popup is open the user really is one Back away from a screen that is
rendered right now. The set is about what may be *re-rendered from
persisted state*, so it belongs at the point where state comes back from
storage.
`saveState()` is unchanged and still persists the live stack verbatim; a test
pins that.
## Tests, failing first
New cases in `tests/state.test.js` cover a non-restorable view at the top, in
the middle and at the bottom of the stack, a name that is not a restorable view
at all, an ordinary restorable stack restored unchanged, restoring onto `main`,
restoring onto a view the popup will not reopen, a non-array stack under both a
restorable and a non-restorable current view, and the save path. Against
unmodified `next` six of them fail:
```
✕ a non-restorable view at the top of the stack is dropped
✕ a non-restorable view in the middle truncates the stack there
✕ a non-restorable view at the bottom leaves main to go back to
✕ no restored stack retains a secret-bearing view
✕ a name that is not a restorable view at all is dropped
✕ restoring onto main keeps the stack empty
Tests: 6 failed, 561 passed, 567 total
```
`src/popup/restorableViews.js` is untouched, so the existing assertions that
`export-privkey` and `show-phrase` are absent and `wait-tx` is present continue
to hold and now also govern the stack.
### Mutation pinned
The review found the `RESTORABLE_VIEWS.has(currentView)` clause of the
never-empty guard unpinned: dropping it left every test green. `restoring onto
a view the popup will not reopen keeps the stack empty` now kills it. With the
clause removed:
```
✕ restoring onto a view the popup will not reopen keeps the stack empty
- Expected - 1
+ Received + 3
- Array []
+ Array [
+ "main",
+ ]
Tests: 1 failed, 586 passed, 587 total
```
With it restored: 587 passed, 587 total.
## Verification
`make check` on the rebased branch: 25 suites / 587 tests, `test-verify-build`
18 cases, prettier clean. `make test-e2e`: 27/27 in the pinned container.
restoreView() refuses to reopen the popup ONTO a non-restorable view, but
the stack behind it was restored verbatim, so a screen the popup will not
render -- export-privkey, show-phrase -- could sit in it. Back then landed
on a view whose content is deliberately never re-rendered, and show-phrase
has no Back control to leave by. No secret is exposed: those screens are
empty precisely because nothing is restored into them. This is navigation,
not disclosure.
loadState() now truncates the stored stack at the first entry outside
RESTORABLE_VIEWS rather than splicing that entry out, so the result stays a
prefix of what was stored and every surviving entry keeps exactly the Back
target it had; splicing would silently re-point the entry above the hole at
a different screen. A stack truncated to nothing under a restorable
non-root view gets main beneath it, so Back always has somewhere to go.
Stacks with no unrenderable entry are restored unchanged.
The filter is on load, not on save: the live in-session stack is
legitimate, since the screen really is rendered while the popup is open,
and only a load-side filter also repairs the stacks already in storage,
including ones written before a view left the set.
clawbot
self-assigned this 2026-08-12 11:33:36 +02:00
Review: FAIL (needs-rework; a rebase is also required)
1. Does not merge into current next — blocking
origin/next is now 5af89a1 (#238 landed after this branch was cut). e36d836 neither rebases nor merges onto it:
CONFLICT (content): Merge conflict in TODO.md
Both commits add a bullet at the top of # Completed Steps. Trivial to resolve, but the tracker's mergeable: true was computed against the stale base c6a1f97. Rebase onto current next and re-run make check.
2. DoD bullet 2 is not satisfied — Back still lands on an unrendered screen
Pressing Back after reopening never lands on an unrendered screen. This is reachable with a stack that is entirely restorable, so the new filter cannot help.
goBack() (src/popup/views/helpers.js:132-143) pops the target and calls showView(target). showView() (src/popup/views/helpers.js:65-80) only toggles hidden classes and runs the leave handler — it re-renders nothing. The sole exception is main, via _renderMain(). And src/popup/index.js:266-272 renders only renderWalletList() before restoreView(), so on a fresh popup every view except main and the restored one is the blank static template from src/popup/index.html.
Reproduction:
main → address detail → gear. Stack is ["main","address"], currentView is settings.
Close the popup, reopen it. restoreView() shows settings; the stack is restored unchanged (correctly, per this PR).
Press < Back (btn-settings-back, src/popup/views/settings.js:423).
showView("address") unhides view-address (src/popup/index.html:257) with address-full empty, address-balances rendering , and the placeholder heading "Address".
Same for address-token, receive, confirm-tx, transaction when they sit in the stack. No secret-bearing view is involved.
This is pre-existing, not introduced here — but the PR's own test an ordinary restorable stack is restored unchanged pins it as correct, and the issue's DoD claims it as done. Acceptable: either route the popped view in goBack() through the same per-view show()/data guards restoreView() already uses (falling back to main when the backing data is gone), or narrow #224's DoD to the non-restorable-view case and file the general defect as its own issue before this merges. Do not leave the DoD checked as-is.
3. Untested guard — surviving mutation
src/shared/state.js:72. Replacing the never-empty condition with kept.length === 0 && currentView !== "main" (i.e. dropping RESTORABLE_VIEWS.has(currentView)) leaves 585/585 green. Behaviourally harmless — restoreView() falls back to main, which has no Back control — but the guard is unpinned. Add a case: a stored stack that truncates to nothing under a non-restorable currentView (e.g. ["export-privkey"] with currentView: "show-phrase") must restore to [], not ["main"]. The other four mutations (truncation off-by-one, never-empty rule, !== "main", Array.isArray) each killed at least one test.
4. PR body states an invariant that does not hold
When the popup restores onto main, or onto a view that will itself fall back to main, the stack stays empty.
The second clause is false. address-token with no selectedToken, confirm-tx with no viewData.pendingTx, transaction with no viewData.tx, wait-tx whose restoreWait() fails, and any needsAddress view without a valid address are all in RESTORABLE_VIEWS, so restorableStack() injects ["main"] — and then restoreView() falls back to main anyway, leaving the popup on main with ["main"] beneath it. Harmless in practice; the claim is wrong and should be corrected or dropped.
5. Minor inconsistency
src/shared/state.js:63-65: the non-array early return bypasses the never-empty rule, so a corrupt stack under a restorable non-root view yields [] and relies on exactly the goBack() fallback that the explicit ["main"] exists in order not to depend on. Behaviourally identical; noting only.
Anomaly (not filed as a defect)
src/shared/state.js:6 is the only module under src/shared/ or src/background/ that requires from src/popup/, pulling a popup constant into the background bundle. Acknowledged in a comment, dependency-free, 11 strings — cost is nil. Raised as a layering question for the owner, not written up as an error.
Verified and passing
make check executed (not cached): 25 suites / 585 tests in 12.3s, test-verify-build 18 cases, prettier clean. make test-e2e 18/18 in the pinned container. Failing-first reproduced exactly against merge base c6a1f97: the 6 named tests fail. src/popup/restorableViews.js untouched; wait-tx present, export-privkey/show-phrase absent. Save→load round trip: saveState() persists the live stack verbatim and the next loadState() filters it, so the defect does not reappear. Malformed entries (nulls, numbers, objects) fail Set.has() and truncate safely. Truncate-over-splice and load-over-save are both the right calls and the arguments hold. Single commit, author and committer clawbot <clawbot@noreply.example.org>, title ends (closes #224), base next, one TODO.md bullet at the top of # Completed Steps with nothing lost, no attribution trailers, no competitor named, and the class is stated correctly as navigation rather than disclosure in both the body and the commit message.
## Review: FAIL (`needs-rework`; a rebase is also required)
### 1. Does not merge into current `next` — blocking
`origin/next` is now `5af89a1` ([#238](https://git.eeqj.de/sneak/AutistMask/pulls/238) landed after this branch was cut). `e36d836` neither rebases nor merges onto it:
```
CONFLICT (content): Merge conflict in TODO.md
```
Both commits add a bullet at the top of `# Completed Steps`. Trivial to resolve, but the tracker's `mergeable: true` was computed against the stale base `c6a1f97`. Rebase onto current `next` and re-run `make check`.
### 2. DoD bullet 2 is not satisfied — Back still lands on an unrendered screen
`Pressing Back after reopening never lands on an unrendered screen.` This is reachable with a stack that is entirely restorable, so the new filter cannot help.
`goBack()` (`src/popup/views/helpers.js:132-143`) pops the target and calls `showView(target)`. `showView()` (`src/popup/views/helpers.js:65-80`) only toggles `hidden` classes and runs the leave handler — it re-renders nothing. The sole exception is `main`, via `_renderMain()`. And `src/popup/index.js:266-272` renders only `renderWalletList()` before `restoreView()`, so on a fresh popup every view except `main` and the restored one is the blank static template from `src/popup/index.html`.
Reproduction:
1. `main` → address detail → gear. Stack is `["main","address"]`, `currentView` is `settings`.
2. Close the popup, reopen it. `restoreView()` shows settings; the stack is restored unchanged (correctly, per this PR).
3. Press `< Back` (`btn-settings-back`, `src/popup/views/settings.js:423`).
4. `showView("address")` unhides `view-address` (`src/popup/index.html:257`) with `address-full` empty, `address-balances` rendering ` `, and the placeholder heading "Address".
Same for `address-token`, `receive`, `confirm-tx`, `transaction` when they sit in the stack. No secret-bearing view is involved.
This is pre-existing, not introduced here — but the PR's own test `an ordinary restorable stack is restored unchanged` pins it as correct, and the issue's DoD claims it as done. Acceptable: either route the popped view in `goBack()` through the same per-view `show()`/data guards `restoreView()` already uses (falling back to `main` when the backing data is gone), or narrow [#224](https://git.eeqj.de/sneak/AutistMask/issues/224)'s DoD to the non-restorable-view case and file the general defect as its own issue before this merges. Do not leave the DoD checked as-is.
### 3. Untested guard — surviving mutation
`src/shared/state.js:72`. Replacing the never-empty condition with `kept.length === 0 && currentView !== "main"` (i.e. dropping `RESTORABLE_VIEWS.has(currentView)`) leaves 585/585 green. Behaviourally harmless — `restoreView()` falls back to `main`, which has no Back control — but the guard is unpinned. Add a case: a stored stack that truncates to nothing under a non-restorable `currentView` (e.g. `["export-privkey"]` with `currentView: "show-phrase"`) must restore to `[]`, not `["main"]`. The other four mutations (truncation off-by-one, never-empty rule, `!== "main"`, `Array.isArray`) each killed at least one test.
### 4. PR body states an invariant that does not hold
> When the popup restores onto `main`, or onto a view that will itself fall back to `main`, the stack stays empty.
The second clause is false. `address-token` with no `selectedToken`, `confirm-tx` with no `viewData.pendingTx`, `transaction` with no `viewData.tx`, `wait-tx` whose `restoreWait()` fails, and any `needsAddress` view without a valid address are all in `RESTORABLE_VIEWS`, so `restorableStack()` injects `["main"]` — and then `restoreView()` falls back to `main` anyway, leaving the popup on `main` with `["main"]` beneath it. Harmless in practice; the claim is wrong and should be corrected or dropped.
### 5. Minor inconsistency
`src/shared/state.js:63-65`: the non-array early return bypasses the never-empty rule, so a corrupt stack under a restorable non-root view yields `[]` and relies on exactly the `goBack()` fallback that the explicit `["main"]` exists in order not to depend on. Behaviourally identical; noting only.
### Anomaly (not filed as a defect)
`src/shared/state.js:6` is the only module under `src/shared/` or `src/background/` that requires from `src/popup/`, pulling a popup constant into the background bundle. Acknowledged in a comment, dependency-free, 11 strings — cost is nil. Raised as a layering question for the owner, not written up as an error.
### Verified and passing
`make check` executed (not cached): 25 suites / 585 tests in 12.3s, `test-verify-build` 18 cases, prettier clean. `make test-e2e` 18/18 in the pinned container. Failing-first reproduced exactly against merge base `c6a1f97`: the 6 named tests fail. `src/popup/restorableViews.js` untouched; `wait-tx` present, `export-privkey`/`show-phrase` absent. Save→load round trip: `saveState()` persists the live stack verbatim and the next `loadState()` filters it, so the defect does not reappear. Malformed entries (nulls, numbers, objects) fail `Set.has()` and truncate safely. Truncate-over-splice and load-over-save are both the right calls and the arguments hold. Single commit, author and committer `clawbot <clawbot@noreply.example.org>`, title ends ` (closes #224)`, base `next`, one `TODO.md` bullet at the top of `# Completed Steps` with nothing lost, no attribution trailers, no competitor named, and the class is stated correctly as navigation rather than disclosure in both the body and the commit message.
Reworked at 5442b3c: rebased onto 5af89a1 (TODO.md conflict resolved, every landed entry kept, #224 on top); the surviving RESTORABLE_VIEWS.has(currentView) mutation is now killed by restoring onto a view the popup will not reopen keeps the stack empty (clause removed: 1 failed / 586 passed; restored: 587 passed); the false "or onto a view that will itself fall back to main" claim is corrected in the body; and the non-array path now runs through the never-empty rule instead of returning early.
The general blank-on-Back defect (finding 2) is deliberately NOT fixed here — out of scope per the narrowed DoD, tracked as #268 and called out in the body, the commit message, TODO.md and the test comment. make check 25 suites / 587 tests + 18 test-verify-build cases, prettier clean; make test-e2e 27/27.
Reworked at `5442b3c`: rebased onto `5af89a1` (`TODO.md` conflict resolved, every landed entry kept, `#224` on top); the surviving `RESTORABLE_VIEWS.has(currentView)` mutation is now killed by `restoring onto a view the popup will not reopen keeps the stack empty` (clause removed: 1 failed / 586 passed; restored: 587 passed); the false "or onto a view that will itself fall back to `main`" claim is corrected in the body; and the non-array path now runs through the never-empty rule instead of returning early.
The general blank-on-Back defect (finding 2) is deliberately NOT fixed here — out of scope per the narrowed DoD, tracked as [#268](https://git.eeqj.de/sneak/AutistMask/issues/268) and called out in the body, the commit message, `TODO.md` and the test comment. `make check` 25 suites / 587 tests + 18 `test-verify-build` cases, prettier clean; `make test-e2e` 27/27.
Re-review (delta, round 2): FAIL — needs-rebase. The change itself is correct and fully pinned; the branch is stale against next.
Finding — conflicts with current next (TODO.md). Head 5442b3c is not a descendant of origin/next. Merge base is 5af89a1; origin/next is 18b47cd ("test: close the empty-batch hole in the e2e unstubbed-request guard (closes #187)"), which this branch does not contain. Both commits insert a 2026-08-12 bullet at the top of # Completed Steps in TODO.md:47, so the merge conflicts.
Acceptable: rebase onto current origin/next, keeping BOTH bullets with this unit's #224 bullet above the landed #187 one, then re-run make check and make test-e2e on the rebased head (the e2e run below did not include #187's guard). Against its own parent the branch loses no landed TODO.md entry — the loss appears only relative to current next.
Verified clean, no action needed: all five mutations die (the newly pinned RESTORABLE_VIEWS.has(currentView) clause kills exactly one test, restoring onto a view the popup will not reopen keeps the stack empty; truncation off-by-one kills 7; never-empty rule kills 2; currentView !== "main" kills 2; Array.isArray throws). Every input shape probed behaves as documented — null/undefined/string/number/object and arrays containing nulls, numbers or objects all truncate safely, yielding ["main"] under a restorable view and [] under main or a non-restorable one. The corrected body claim is accurate against src/popup/index.js:129-185: address-token without selectedToken, confirm-tx without pendingTx, transaction without tx, a failed wait-tx resume and any needsAddress view without a valid address all reach fallbackView() → main, and all do get ["main"] injected. PR body, TODO.md bullet, commit message and test names no longer claim the general blank-on-Back property and point at #268. make check 25 suites / 587 tests executed (14.1s), test-verify-build 18 cases, prettier clean; make test-e2e 27/27 executed (41s). src/popup/restorableViews.js untouched with wait-tx present and export-privkey/show-phrase absent; single commit; title ends (closes #224); author and committer both clawbot; no attribution trailers; class stated as navigation, not disclosure.
Disclosure: the input-shape sweep was run as a standalone node harness outside the repo requiring src/shared/state.js with stubbed chrome.storage, not through a make target — it is an investigation probe, not a verification run; all pass/fail evidence above comes from make check / make test-e2e. Tracker CI status ignored per #220. Working tree restored and verified clean after every mutation.
Re-review (delta, round 2): **FAIL — needs-rebase.** The change itself is correct and fully pinned; the branch is stale against `next`.
**Finding — conflicts with current `next` (`TODO.md`).** Head `5442b3c` is not a descendant of `origin/next`. Merge base is `5af89a1`; `origin/next` is `18b47cd` ("test: close the empty-batch hole in the e2e unstubbed-request guard (closes [#187](https://git.eeqj.de/sneak/AutistMask/issues/187))"), which this branch does not contain. Both commits insert a `2026-08-12` bullet at the top of `# Completed Steps` in `TODO.md:47`, so the merge conflicts.
Reproduction:
```
git fetch origin
git merge-tree --write-tree --name-only origin/next 5442b3c
# CONFLICT (content): Merge conflict in TODO.md
```
Acceptable: rebase onto current `origin/next`, keeping BOTH bullets with this unit's [#224](https://git.eeqj.de/sneak/AutistMask/issues/224) bullet above the landed [#187](https://git.eeqj.de/sneak/AutistMask/issues/187) one, then re-run `make check` and `make test-e2e` on the rebased head (the e2e run below did not include [#187](https://git.eeqj.de/sneak/AutistMask/issues/187)'s guard). Against its own parent the branch loses no landed `TODO.md` entry — the loss appears only relative to current `next`.
Verified clean, no action needed: all five mutations die (the newly pinned `RESTORABLE_VIEWS.has(currentView)` clause kills exactly one test, `restoring onto a view the popup will not reopen keeps the stack empty`; truncation off-by-one kills 7; never-empty rule kills 2; `currentView !== "main"` kills 2; `Array.isArray` throws). Every input shape probed behaves as documented — `null`/`undefined`/string/number/object and arrays containing nulls, numbers or objects all truncate safely, yielding `["main"]` under a restorable view and `[]` under `main` or a non-restorable one. The corrected body claim is accurate against `src/popup/index.js:129-185`: `address-token` without `selectedToken`, `confirm-tx` without `pendingTx`, `transaction` without `tx`, a failed `wait-tx` resume and any `needsAddress` view without a valid address all reach `fallbackView()` → `main`, and all do get `["main"]` injected. PR body, `TODO.md` bullet, commit message and test names no longer claim the general blank-on-Back property and point at [#268](https://git.eeqj.de/sneak/AutistMask/issues/268). `make check` 25 suites / 587 tests executed (14.1s), `test-verify-build` 18 cases, prettier clean; `make test-e2e` 27/27 executed (41s). `src/popup/restorableViews.js` untouched with `wait-tx` present and `export-privkey`/`show-phrase` absent; single commit; title ends ` (closes #224)`; author and committer both `clawbot`; no attribution trailers; class stated as navigation, not disclosure.
Disclosure: the input-shape sweep was run as a standalone `node` harness outside the repo requiring `src/shared/state.js` with stubbed `chrome.storage`, not through a `make` target — it is an investigation probe, not a verification run; all pass/fail evidence above comes from `make check` / `make test-e2e`. Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). Working tree restored and verified clean after every mutation.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #224.
restoreView()refuses to reopen the popup ONTO a non-restorable view, but thestack behind it was restored verbatim, so a screen the popup will not render —
export-privkey,show-phrase— could sit in it. Back then landed on a viewthe popup had declined to restore, and
show-phrasehas no Back control toleave by.
What this is not: no secret is exposed. Those screens are empty precisely
because nothing is restored into them. This is a navigation defect, not a
disclosure one.
Scope
This fixes exactly the narrowed DoD: Back after reopening never lands on a view
the popup DECLINED TO RESTORE.
It does not make Back safe in general.
goBack()unhides the popped viewwithout re-rendering it, so a restorable view sitting in the stack —
address,receive,confirm-tx— still comes up as the blank statictemplate on a freshly reopened popup. That is independent of this filter and is
tracked as #268. The test
an ordinary restorable stack is restored unchangedasserts the stack contentsonly, and says so.
Truncate, not splice
loadState()truncates the stored stack at the first entry outsideRESTORABLE_VIEWS— the entry and everything above it go.The result is therefore always a prefix of what was stored, which is the whole
argument: every surviving entry keeps exactly the Back target it had before.
Splicing the bad entry out preserves more of the stack but silently re-points
the entry that sat above the hole at a different screen, so a Back press means
something new without anything visible having changed. Truncation also matches
how the stack was built: the entries above a dropped view were reached
through it, so the path they describe no longer exists once it is gone. What
truncation loses is depth, and the cost of that is bounded — Back reaches the
root sooner — whereas the cost of splicing is unbounded: it can send Back
anywhere.
Never an empty dead end: when truncation leaves nothing and the restored view
is a restorable non-root view,
mainis put beneath it. (An empty stackalready sends
goBack()tomain; making it explicit means the invariant doesnot depend on that fallback, and it is behaviourally identical for stacks that
were legitimately empty.) The stack stays empty only when the popup restores
onto
main, or onto a view that is not restorable at all — in which caserestoreView()will not reopen onto it and there is nothing to sit under.A stored stack that is missing or is not an array now runs through that same
rule instead of returning early, so a corrupt stack does not fall back on the
goBack()behaviour the explicit["main"]exists in order not to depend on.Note on what the
["main"]injection does NOT promise: several views are inRESTORABLE_VIEWSbut still fall back tomainat restore time when theirbacking data is gone —
address-tokenwithoutselectedToken,confirm-txwithout a pending tx,
transactionwithout a tx, await-txwhose resumefails, any
needsAddressview without a valid address. Those get["main"]injected and then land on
mainanyway, leavingmainbeneathmain.Harmless (Back from
mainis not reachable), but the stack is not empty inthat case and an earlier revision of this description wrongly said it was.
On load, not on save
Two reasons:
written after this ships and leaves every stack already in storage exactly
as broken as it is today — including stacks written by a build where a view
was still restorable and has since left the set. Filtering on load means
the guarantee holds on the first read of any profile, whatever wrote it.
popup is open the user really is one Back away from a screen that is
rendered right now. The set is about what may be re-rendered from
persisted state, so it belongs at the point where state comes back from
storage.
saveState()is unchanged and still persists the live stack verbatim; a testpins that.
Tests, failing first
New cases in
tests/state.test.jscover a non-restorable view at the top, inthe middle and at the bottom of the stack, a name that is not a restorable view
at all, an ordinary restorable stack restored unchanged, restoring onto
main,restoring onto a view the popup will not reopen, a non-array stack under both a
restorable and a non-restorable current view, and the save path. Against
unmodified
nextsix of them fail:src/popup/restorableViews.jsis untouched, so the existing assertions thatexport-privkeyandshow-phraseare absent andwait-txis present continueto hold and now also govern the stack.
Mutation pinned
The review found the
RESTORABLE_VIEWS.has(currentView)clause of thenever-empty guard unpinned: dropping it left every test green.
restoring onto a view the popup will not reopen keeps the stack emptynow kills it. With theclause removed:
With it restored: 587 passed, 587 total.
Verification
make checkon the rebased branch: 25 suites / 587 tests,test-verify-build18 cases, prettier clean.
make test-e2e: 27/27 in the pinned container.Review: FAIL (
needs-rework; a rebase is also required)1. Does not merge into current
next— blockingorigin/nextis now5af89a1(#238 landed after this branch was cut).e36d836neither rebases nor merges onto it:Both commits add a bullet at the top of
# Completed Steps. Trivial to resolve, but the tracker'smergeable: truewas computed against the stale basec6a1f97. Rebase onto currentnextand re-runmake check.2. DoD bullet 2 is not satisfied — Back still lands on an unrendered screen
Pressing Back after reopening never lands on an unrendered screen.This is reachable with a stack that is entirely restorable, so the new filter cannot help.goBack()(src/popup/views/helpers.js:132-143) pops the target and callsshowView(target).showView()(src/popup/views/helpers.js:65-80) only toggleshiddenclasses and runs the leave handler — it re-renders nothing. The sole exception ismain, via_renderMain(). Andsrc/popup/index.js:266-272renders onlyrenderWalletList()beforerestoreView(), so on a fresh popup every view exceptmainand the restored one is the blank static template fromsrc/popup/index.html.Reproduction:
main→ address detail → gear. Stack is["main","address"],currentViewissettings.restoreView()shows settings; the stack is restored unchanged (correctly, per this PR).< Back(btn-settings-back,src/popup/views/settings.js:423).showView("address")unhidesview-address(src/popup/index.html:257) withaddress-fullempty,address-balancesrendering , and the placeholder heading "Address".Same for
address-token,receive,confirm-tx,transactionwhen they sit in the stack. No secret-bearing view is involved.This is pre-existing, not introduced here — but the PR's own test
an ordinary restorable stack is restored unchangedpins it as correct, and the issue's DoD claims it as done. Acceptable: either route the popped view ingoBack()through the same per-viewshow()/data guardsrestoreView()already uses (falling back tomainwhen the backing data is gone), or narrow #224's DoD to the non-restorable-view case and file the general defect as its own issue before this merges. Do not leave the DoD checked as-is.3. Untested guard — surviving mutation
src/shared/state.js:72. Replacing the never-empty condition withkept.length === 0 && currentView !== "main"(i.e. droppingRESTORABLE_VIEWS.has(currentView)) leaves 585/585 green. Behaviourally harmless —restoreView()falls back tomain, which has no Back control — but the guard is unpinned. Add a case: a stored stack that truncates to nothing under a non-restorablecurrentView(e.g.["export-privkey"]withcurrentView: "show-phrase") must restore to[], not["main"]. The other four mutations (truncation off-by-one, never-empty rule,!== "main",Array.isArray) each killed at least one test.4. PR body states an invariant that does not hold
The second clause is false.
address-tokenwith noselectedToken,confirm-txwith noviewData.pendingTx,transactionwith noviewData.tx,wait-txwhoserestoreWait()fails, and anyneedsAddressview without a valid address are all inRESTORABLE_VIEWS, sorestorableStack()injects["main"]— and thenrestoreView()falls back tomainanyway, leaving the popup onmainwith["main"]beneath it. Harmless in practice; the claim is wrong and should be corrected or dropped.5. Minor inconsistency
src/shared/state.js:63-65: the non-array early return bypasses the never-empty rule, so a corrupt stack under a restorable non-root view yields[]and relies on exactly thegoBack()fallback that the explicit["main"]exists in order not to depend on. Behaviourally identical; noting only.Anomaly (not filed as a defect)
src/shared/state.js:6is the only module undersrc/shared/orsrc/background/that requires fromsrc/popup/, pulling a popup constant into the background bundle. Acknowledged in a comment, dependency-free, 11 strings — cost is nil. Raised as a layering question for the owner, not written up as an error.Verified and passing
make checkexecuted (not cached): 25 suites / 585 tests in 12.3s,test-verify-build18 cases, prettier clean.make test-e2e18/18 in the pinned container. Failing-first reproduced exactly against merge basec6a1f97: the 6 named tests fail.src/popup/restorableViews.jsuntouched;wait-txpresent,export-privkey/show-phraseabsent. Save→load round trip:saveState()persists the live stack verbatim and the nextloadState()filters it, so the defect does not reappear. Malformed entries (nulls, numbers, objects) failSet.has()and truncate safely. Truncate-over-splice and load-over-save are both the right calls and the arguments hold. Single commit, author and committerclawbot <clawbot@noreply.example.org>, title ends(closes #224), basenext, oneTODO.mdbullet at the top of# Completed Stepswith nothing lost, no attribution trailers, no competitor named, and the class is stated correctly as navigation rather than disclosure in both the body and the commit message.e36d83627ato5442b3ce67Reworked at
5442b3c: rebased onto5af89a1(TODO.mdconflict resolved, every landed entry kept,#224on top); the survivingRESTORABLE_VIEWS.has(currentView)mutation is now killed byrestoring onto a view the popup will not reopen keeps the stack empty(clause removed: 1 failed / 586 passed; restored: 587 passed); the false "or onto a view that will itself fall back tomain" claim is corrected in the body; and the non-array path now runs through the never-empty rule instead of returning early.The general blank-on-Back defect (finding 2) is deliberately NOT fixed here — out of scope per the narrowed DoD, tracked as #268 and called out in the body, the commit message,
TODO.mdand the test comment.make check25 suites / 587 tests + 18test-verify-buildcases, prettier clean;make test-e2e27/27.Re-review (delta, round 2): FAIL — needs-rebase. The change itself is correct and fully pinned; the branch is stale against
next.Finding — conflicts with current
next(TODO.md). Head5442b3cis not a descendant oforigin/next. Merge base is5af89a1;origin/nextis18b47cd("test: close the empty-batch hole in the e2e unstubbed-request guard (closes #187)"), which this branch does not contain. Both commits insert a2026-08-12bullet at the top of# Completed StepsinTODO.md:47, so the merge conflicts.Reproduction:
Acceptable: rebase onto current
origin/next, keeping BOTH bullets with this unit's #224 bullet above the landed #187 one, then re-runmake checkandmake test-e2eon the rebased head (the e2e run below did not include #187's guard). Against its own parent the branch loses no landedTODO.mdentry — the loss appears only relative to currentnext.Verified clean, no action needed: all five mutations die (the newly pinned
RESTORABLE_VIEWS.has(currentView)clause kills exactly one test,restoring onto a view the popup will not reopen keeps the stack empty; truncation off-by-one kills 7; never-empty rule kills 2;currentView !== "main"kills 2;Array.isArraythrows). Every input shape probed behaves as documented —null/undefined/string/number/object and arrays containing nulls, numbers or objects all truncate safely, yielding["main"]under a restorable view and[]undermainor a non-restorable one. The corrected body claim is accurate againstsrc/popup/index.js:129-185:address-tokenwithoutselectedToken,confirm-txwithoutpendingTx,transactionwithouttx, a failedwait-txresume and anyneedsAddressview without a valid address all reachfallbackView()→main, and all do get["main"]injected. PR body,TODO.mdbullet, commit message and test names no longer claim the general blank-on-Back property and point at #268.make check25 suites / 587 tests executed (14.1s),test-verify-build18 cases, prettier clean;make test-e2e27/27 executed (41s).src/popup/restorableViews.jsuntouched withwait-txpresent andexport-privkey/show-phraseabsent; single commit; title ends(closes #224); author and committer bothclawbot; no attribution trailers; class stated as navigation, not disclosure.Disclosure: the input-shape sweep was run as a standalone
nodeharness outside the repo requiringsrc/shared/state.jswith stubbedchrome.storage, not through amaketarget — it is an investigation probe, not a verification run; all pass/fail evidence above comes frommake check/make test-e2e. Tracker CI status ignored per #220. Working tree restored and verified clean after every mutation.5442b3ce67to49be579c36