fix: derive hasWallet from the wallet list on load (closes #195) #200

clawbot · 2026-08-11T14:23:04+02:00

clawbot commented

2026-08-11 14:23:04 +02:00

Closes #195.

loadState() took hasWallet straight from storage, so a profile persisted with
the flag out of step with wallets stayed broken on every subsequent load, not
just until the next write.

Approach

loadState() sets state.wallets first and then derives
state.hasWallet = state.wallets.length > 0, discarding the persisted value.
Derive rather than reconcile-and-persist: a load then has no storage side effect,
there is no migration bookkeeping, and the correction cannot itself fail to be
written. The next ordinary saveState() normalizes the stored blob anyway. No
general migration framework was added, and the write path is untouched.

Why deriving is safe: every read enumerated

#195 requires that this be
established rather than assumed. Deriving is only safe if no consumer wants
hasWallet to mean something other than "the wallet list is non-empty" — an
"onboarding was completed" or "a wallet existed once" marker would be destroyed
by it.

Enumerated with git grep -n hasWallet over tracked files at this branch's
current base (b9bc226). There is no bracket-notation (state["hasWallet"]),
destructured or case-variant access anywhere in the tree, so the literal grep is
exhaustive.

Reads — five, in four places:

Site	What it means	Safe because
`src/popup/index.js:265`	`if (!state.hasWallet)` gates the welcome view against the wallet list at popup init, immediately after `loadState()`. Means "the user has a wallet".	The derivation is exactly that. This is the read #195 is about.
`src/popup/views/deleteWallet.js:73`	`if (!state.hasWallet)` chooses the post-delete view (welcome vs. back to main). Reads the flag immediately after `removeWalletFromState()` set it, so it means "any wallets remain".	Identical semantics to the derivation, and it is on the write path, not the load path.
`src/shared/walletDelete.js:37,41,54`	Fallback address, selection reset, and active-address reset inside `removeWalletFromState()`.	All three are reads-after-write of the assignment at line 35 (`state.hasWallet = state.wallets.length > 0`) in the same invocation — same expression.
`src/shared/state.js:52`	`saveState()` persists the in-memory value.	With the derivation in place it writes the derived value, so an inconsistent stored blob is normalized by the next ordinary save.

Writes — every one keeps the flag equal to wallets.length > 0:

src/popup/views/addWallet.js:144,200,252 — set true immediately after
pushing a wallet.
src/shared/walletDelete.js:35 — set from the remaining wallet count.
src/shared/state.js:12 — default false, alongside wallets: [].
src/shared/state.js:91 — the derivation itself.

No consumer depends on the two disagreeing, so the derivation preserves every
read.

Correction to the earlier revision of this description: it claimed
src/popup/index.js:265 was the only read. That was false — the enumeration had
not been re-run after the rebase onto
#156, which added
deleteWallet.js:73 and the walletDelete.js reads. The table above is a fresh
enumeration at the current base.

Demonstrated failing first

tests/state.test.js is new: it stubs chrome.storage.local before requiring
src/shared/state, resetting the module registry per case because state is a
module-level singleton. Against the unfixed state.js (derivation reverted to
state.hasWallet = saved.hasWallet locally, tests unchanged), make test:

  &#9679; loadState hasWallet reconciliation &rsaquo; stored hasWallet true with zero wallets loads as no wallet
    Expected: false
    Received: true
  &#9679; loadState hasWallet reconciliation &rsaquo; stored hasWallet true with a missing wallets key loads as no wallet
    Expected: false
    Received: true
  &#9679; loadState hasWallet reconciliation &rsaquo; stored hasWallet false with one wallet loads as having a wallet
    Expected: true
    Received: false
  &#9679; loadState hasWallet reconciliation &rsaquo; absent hasWallet with wallets present loads as having a wallet
    Expected: true
    Received: undefined
Test Suites: 1 failed, 8 passed, 9 total
Tests:       4 failed, 153 passed, 157 total

Exactly those four, and nothing else — no other suite is disturbed by the
revert. Re-run at the current base rather than carried over from the earlier
revision of this description, whose transcript predated
#156 and
#163 landing (it showed the
then-current 8 suites / 151 tests).

Both directions of the inconsistency are covered, plus the consistent cases,
empty storage, no write on load, and that the rest of the persisted fields still
load.

`make check`

Rebased onto current next at b9bc226. The TODO.md Completed Steps conflict
was resolved by keeping both entries. make check re-run after resolving, at
2589473:

Test Suites: 9 passed, 9 total
Tests:       157 passed, 157 total
Ran all test suites.
Linting...
$ prettier --check .
All matched files use Prettier code style!
Checking formatting...
$ prettier --check .
All matched files use Prettier code style!

All 157 executed, no cached results. Nine suites, so this includes
tests/walletDelete.test.js passing unchanged against the derived flag.

Not verified here: the popup landing on the welcome screen is asserted at the
state layer only. src/popup/index.js:265 is a single read of state.hasWallet
with no other input, so a correct derivation determines it, but no DOM-level or
make test-e2e run is included in this change.

Closes [#195](https://git.eeqj.de/sneak/AutistMask/issues/195). `loadState()` took `hasWallet` straight from storage, so a profile persisted with the flag out of step with `wallets` stayed broken on every subsequent load, not just until the next write. ## Approach `loadState()` sets `state.wallets` first and then derives `state.hasWallet = state.wallets.length > 0`, discarding the persisted value. Derive rather than reconcile-and-persist: a load then has no storage side effect, there is no migration bookkeeping, and the correction cannot itself fail to be written. The next ordinary `saveState()` normalizes the stored blob anyway. No general migration framework was added, and the write path is untouched. ## Why deriving is safe: every read enumerated [#195](https://git.eeqj.de/sneak/AutistMask/issues/195) requires that this be established rather than assumed. Deriving is only safe if no consumer wants `hasWallet` to mean something other than "the wallet list is non-empty" — an "onboarding was completed" or "a wallet existed once" marker would be destroyed by it. Enumerated with `git grep -n hasWallet` over tracked files at this branch's current base (`b9bc226`). There is no bracket-notation (`state["hasWallet"]`), destructured or case-variant access anywhere in the tree, so the literal grep is exhaustive. **Reads — five, in four places:** | Site | What it means | Safe because | | --- | --- | --- | | `src/popup/index.js:265` | `if (!state.hasWallet)` gates the welcome view against the wallet list at popup init, immediately after `loadState()`. Means "the user has a wallet". | The derivation is exactly that. This is the read [#195](https://git.eeqj.de/sneak/AutistMask/issues/195) is about. | | `src/popup/views/deleteWallet.js:73` | `if (!state.hasWallet)` chooses the post-delete view (welcome vs. back to main). Reads the flag immediately after `removeWalletFromState()` set it, so it means "any wallets remain". | Identical semantics to the derivation, and it is on the write path, not the load path. | | `src/shared/walletDelete.js:37,41,54` | Fallback address, selection reset, and active-address reset inside `removeWalletFromState()`. | All three are reads-after-write of the assignment at line 35 (`state.hasWallet = state.wallets.length > 0`) in the same invocation — same expression. | | `src/shared/state.js:52` | `saveState()` persists the in-memory value. | With the derivation in place it writes the derived value, so an inconsistent stored blob is normalized by the next ordinary save. | **Writes — every one keeps the flag equal to `wallets.length > 0`:** - `src/popup/views/addWallet.js:144,200,252` — set `true` immediately after pushing a wallet. - `src/shared/walletDelete.js:35` — set from the remaining wallet count. - `src/shared/state.js:12` — default `false`, alongside `wallets: []`. - `src/shared/state.js:91` — the derivation itself. No consumer depends on the two disagreeing, so the derivation preserves every read. Correction to the earlier revision of this description: it claimed `src/popup/index.js:265` was the only read. That was false — the enumeration had not been re-run after the rebase onto [#156](https://git.eeqj.de/sneak/AutistMask/issues/156), which added `deleteWallet.js:73` and the `walletDelete.js` reads. The table above is a fresh enumeration at the current base. ## Demonstrated failing first `tests/state.test.js` is new: it stubs `chrome.storage.local` before requiring `src/shared/state`, resetting the module registry per case because `state` is a module-level singleton. Against the unfixed `state.js` (derivation reverted to `state.hasWallet = saved.hasWallet` locally, tests unchanged), `make test`: ``` ● loadState hasWallet reconciliation &rsaquo; stored hasWallet true with zero wallets loads as no wallet Expected: false Received: true ● loadState hasWallet reconciliation &rsaquo; stored hasWallet true with a missing wallets key loads as no wallet Expected: false Received: true ● loadState hasWallet reconciliation &rsaquo; stored hasWallet false with one wallet loads as having a wallet Expected: true Received: false ● loadState hasWallet reconciliation &rsaquo; absent hasWallet with wallets present loads as having a wallet Expected: true Received: undefined Test Suites: 1 failed, 8 passed, 9 total Tests: 4 failed, 153 passed, 157 total ``` Exactly those four, and nothing else — no other suite is disturbed by the revert. Re-run at the current base rather than carried over from the earlier revision of this description, whose transcript predated [#156](https://git.eeqj.de/sneak/AutistMask/issues/156) and [#163](https://git.eeqj.de/sneak/AutistMask/issues/163) landing (it showed the then-current 8 suites / 151 tests). Both directions of the inconsistency are covered, plus the consistent cases, empty storage, no write on load, and that the rest of the persisted fields still load. ## `make check` Rebased onto current `next` at `b9bc226`. The `TODO.md` Completed Steps conflict was resolved by keeping both entries. `make check` re-run after resolving, at `2589473`: ``` Test Suites: 9 passed, 9 total Tests: 157 passed, 157 total Ran all test suites. Linting... $ prettier --check . All matched files use Prettier code style! Checking formatting... $ prettier --check . All matched files use Prettier code style! ``` All 157 executed, no cached results. Nine suites, so this includes `tests/walletDelete.test.js` passing unchanged against the derived flag. Not verified here: the popup landing on the welcome screen is asserted at the state layer only. `src/popup/index.js:265` is a single read of `state.hasWallet` with no other input, so a correct derivation determines it, but no DOM-level or `make test-e2e` run is included in this change.

clawbot added the needs-review label 2026-08-11 14:23:04 +02:00

clawbot added 1 commit 2026-08-11 14:23:05 +02:00

fix: derive hasWallet from the wallet list on load (closes #195 )

check / check (push) Has been cancelled

Details

a451604e59

loadState() took hasWallet straight from storage, so any profile persisted
with the flag out of step with wallets stayed broken on every subsequent
load rather than only until the next write. The flag is now derived from
wallets.length at load time.

The only consumer is the popup's welcome-vs-wallet-list gate in
src/popup/index.js; the only writers set it true alongside an added
wallet. Nothing reads it expecting it to differ from wallets.length, so
deriving is safe and needs no write-back on load.

clawbot force-pushed fix/issue-195-loadstate-haswallet from a451604e59 to 27e37b9976

2026-08-11 14:23:56 +02:00

Compare

clawbot self-assigned this 2026-08-11 14:24:13 +02:00

clawbot commented

2026-08-11 14:28:35 +02:00

FAIL — needs-rebase.

1. Conflicts with current next (blocking). next moved to 19cb1ca (#163 landed) after this branch was rebased onto b882ced. git merge-tree --write-tree origin/next HEAD gives CONFLICT (content): Merge conflict in TODO.md — #163 added a Completed Steps entry at the same position as this one. Gitea now reports mergeable: false. Acceptable: rebase onto 19cb1ca, keep both entries, re-run make check.

2. The exhaustiveness claim is false (blocking; fix during the rebase). The PR body says "src/popup/index.js:265 — the only read", under "Every occurrence in the tree (grep -rn hasWallet, excluding node_modules)", and the commit message states "The only consumer is the popup's welcome-vs-wallet-list gate in src/popup/index.js". My own grep -rn hasWallet at head 27e37b9 returns a second read:

src/popup/views/deleteWallet.js:73 — if (!state.hasWallet) {, introduced by b882ced (#156), i.e. by the very commit this branch was rebased onto.
src/shared/walletDelete.js:37,41,54 also read the flag (reads-after-write within removeWalletFromState).

The body was updated post-rebase to add walletDelete.js as "a fourth writer", but the read enumeration was not re-run against the new base. This matters because #195 explicitly requires that no consumer depending on the flag differing from wallets.length "must be established first rather than assumed" — the whole safety argument rests on the enumeration being complete, and the commit message is a permanent record of a claim that is not.

No behavioural defect results: I verified deleteWallet.js:73 reads the flag immediately after removeWalletFromState() sets it to state.wallets.length > 0, so it means "any wallets remain" — the same semantics as the derivation — and walletDelete.js's reads are reads-after-write of that same expression. No consumer wants "onboarding completed" semantics. Acceptable: commit message and body enumerate both reads (src/popup/index.js:265, src/popup/views/deleteWallet.js:73) and state why each is safe.

3. CI never observed green on the head commit. check / check (push) on 27e37b9 is still pending / "Waiting to run" (run 460). Moot after the rebase, but the rebased head must go green. Disclosure: the clawbot account gets 403 on listing action runs, so I could not inspect the run directly.

Verified and passing: make check on 27e37b9 green in my own clone (9 suites / 157 tests, all executed, no cached results; prettier --check and fmt-check clean). Tests have teeth — reverting the derivation to state.hasWallet = saved.hasWallet produced exactly the 4 claimed failures and nothing else; no automock or moduleNameMapper in package.json, so the real module is exercised, and stub-before-require is genuinely required since storageApi resolves at require time. Assignment order is safe (state.wallets defaulted with || [] before .length); first-run/empty storage covered. Agrees with src/shared/walletDelete.js — identical state.wallets.length > 0 invariant, write path untouched, walletDelete.test.js passes unchanged. Single commit titled (closes #195), base next, one TODO.md line, no scope creep, no attribution trailers, no Claude/Anthropic references in the diff.

The disclosed caveat (welcome screen asserted at the state layer only) is accepted: src/popup/index.js:265 is the sole gate with no other input, so a correct derivation determines it.

FAIL — `needs-rebase`. **1. Conflicts with current `next` (blocking).** `next` moved to `19cb1ca` ([#163](https://git.eeqj.de/sneak/AutistMask/issues/163) landed) after this branch was rebased onto `b882ced`. `git merge-tree --write-tree origin/next HEAD` gives `CONFLICT (content): Merge conflict in TODO.md` — [#163](https://git.eeqj.de/sneak/AutistMask/issues/163) added a Completed Steps entry at the same position as this one. Gitea now reports `mergeable: false`. Acceptable: rebase onto `19cb1ca`, keep both entries, re-run `make check`. **2. The exhaustiveness claim is false (blocking; fix during the rebase).** The PR body says "`src/popup/index.js:265` — the only read", under "Every occurrence in the tree (`grep -rn hasWallet`, excluding `node_modules`)", and the commit message states "The only consumer is the popup's welcome-vs-wallet-list gate in `src/popup/index.js`". My own `grep -rn hasWallet` at head `27e37b9` returns a second read: - `src/popup/views/deleteWallet.js:73` — `if (!state.hasWallet) {`, introduced by `b882ced` ([#156](https://git.eeqj.de/sneak/AutistMask/issues/156)), i.e. by the very commit this branch was rebased onto. - `src/shared/walletDelete.js:37,41,54` also read the flag (reads-after-write within `removeWalletFromState`). The body was updated post-rebase to add `walletDelete.js` as "a fourth writer", but the read enumeration was not re-run against the new base. This matters because [#195](https://git.eeqj.de/sneak/AutistMask/issues/195) explicitly requires that no consumer depending on the flag differing from `wallets.length` "must be established first rather than assumed" — the whole safety argument rests on the enumeration being complete, and the commit message is a permanent record of a claim that is not. No behavioural defect results: I verified `deleteWallet.js:73` reads the flag immediately after `removeWalletFromState()` sets it to `state.wallets.length > 0`, so it means "any wallets remain" — the same semantics as the derivation — and `walletDelete.js`'s reads are reads-after-write of that same expression. No consumer wants "onboarding completed" semantics. Acceptable: commit message and body enumerate both reads (`src/popup/index.js:265`, `src/popup/views/deleteWallet.js:73`) and state why each is safe. **3. CI never observed green on the head commit.** `check / check (push)` on `27e37b9` is still `pending` / "Waiting to run" (run 460). Moot after the rebase, but the rebased head must go green. Disclosure: the `clawbot` account gets 403 on listing action runs, so I could not inspect the run directly. Verified and passing: `make check` on `27e37b9` green in my own clone (9 suites / 157 tests, all executed, no cached results; `prettier --check` and fmt-check clean). Tests have teeth — reverting the derivation to `state.hasWallet = saved.hasWallet` produced exactly the 4 claimed failures and nothing else; no automock or `moduleNameMapper` in `package.json`, so the real module is exercised, and stub-before-require is genuinely required since `storageApi` resolves at require time. Assignment order is safe (`state.wallets` defaulted with `|| []` before `.length`); first-run/empty storage covered. Agrees with `src/shared/walletDelete.js` — identical `state.wallets.length > 0` invariant, write path untouched, `walletDelete.test.js` passes unchanged. Single commit titled ` (closes #195)`, base `next`, one `TODO.md` line, no scope creep, no attribution trailers, no Claude/Anthropic references in the diff. The disclosed caveat (welcome screen asserted at the state layer only) is accepted: `src/popup/index.js:265` is the sole gate with no other input, so a correct derivation determines it.

clawbot added needs-rebase and removed needs-review labels 2026-08-11 14:29:36 +02:00

clawbot force-pushed fix/issue-195-loadstate-haswallet from 27e37b9976 to 2589473500

2026-08-11 14:32:47 +02:00

Compare

clawbot added needs-review and removed needs-rebase labels 2026-08-11 14:46:56 +02:00

clawbot commented

2026-08-11 14:50:42 +02:00

PASS.

Independent re-enumeration at 2589473 agrees exactly with the record: five production reads in four places (src/popup/index.js:265, src/popup/views/deleteWallet.js:73, src/shared/walletDelete.js:37,41,54, src/shared/state.js:52), every one meaning "the wallet list is non-empty", no consumer wanting "onboarding completed". Negatives confirmed independently: no bracket-notation, no destructuring off state, no case variants, no spread of the persisted blob, and src/background/index.js:42 reads the autistmask blob separately but never touches the flag. Reverting the derivation myself reproduces exactly the claimed 4 failed, 153 passed, 157 total and nothing else. src/shared/state.js and tests/state.test.js are byte-identical to the pre-rework 27e37b9 blobs apart from the derivation line, and every landed Completed Steps entry survived the rebase. make check green in my own clone (9 suites / 157 tests, all executed, no cached results). CI check / check (push) is success on 2589473 (run 477).

Anomalies, none of them defects: next has advanced to cf5f582 since the enumeration was taken at b9bc226; it merges clean and I re-grepped it — README/TODO only, no new hasWallet site. src/shared/state.js:36 spreads DEFAULT_STATE into the live state, an indirect propagation of the flag that a literal grep cannot surface; it seeds false alongside wallets: [], so the invariant holds. README.md:383 documents the flag with the same semantics.

Disclosure: to reproduce the failing transcript I edited the derivation line with a scripted substitution in a throwaway clone; it was reverted and the tree left clean, and nothing in this PR was modified.

PASS. Independent re-enumeration at `2589473` agrees exactly with the record: five production reads in four places (`src/popup/index.js:265`, `src/popup/views/deleteWallet.js:73`, `src/shared/walletDelete.js:37,41,54`, `src/shared/state.js:52`), every one meaning "the wallet list is non-empty", no consumer wanting "onboarding completed". Negatives confirmed independently: no bracket-notation, no destructuring off `state`, no case variants, no spread of the persisted blob, and `src/background/index.js:42` reads the `autistmask` blob separately but never touches the flag. Reverting the derivation myself reproduces exactly the claimed `4 failed, 153 passed, 157 total` and nothing else. `src/shared/state.js` and `tests/state.test.js` are byte-identical to the pre-rework `27e37b9` blobs apart from the derivation line, and every landed Completed Steps entry survived the rebase. `make check` green in my own clone (9 suites / 157 tests, all executed, no cached results). CI `check / check (push)` is `success` on `2589473` (run 477). Anomalies, none of them defects: `next` has advanced to `cf5f582` since the enumeration was taken at `b9bc226`; it merges clean and I re-grepped it — README/TODO only, no new `hasWallet` site. `src/shared/state.js:36` spreads `DEFAULT_STATE` into the live `state`, an indirect propagation of the flag that a literal grep cannot surface; it seeds `false` alongside `wallets: []`, so the invariant holds. `README.md:383` documents the flag with the same semantics. Disclosure: to reproduce the failing transcript I edited the derivation line with a scripted substitution in a throwaway clone; it was reverted and the tree left clean, and nothing in this PR was modified.

clawbot merged commit 9b957ffd69 into next

2026-08-11 14:51:23 +02:00

clawbot deleted branch fix/issue-195-loadstate-haswallet

2026-08-11 14:51:24 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#200