fix: WaitTx timeout no longer overwrites a rendered success screen (closes #155) #201

Merged

clawbot merged 1 commits from fix/issue-155-waittx-timeout into next 2026-08-12 10:58:36 +02:00

Conversation 6 Commits 1 Files Changed 6 +658 -20

clawbot commented 2026-08-11 14:23:13 +02:00

Collaborator

Copy Link

Closes #155.

The lifecycle fix

src/popup/views/txStatus.js: the receipt poll called showSuccess() and then
fell through to the elapsed check, so the tick that crossed the 60-second
deadline rendered both outcomes and "Transaction Confirmed" was replaced by
"not confirmed within 60 seconds". A confirmed transaction was reported as
failed.

The wait now has one explicit lifecycle:

• endWait() stops both timers and bumps a wait id. It runs on receipt, on
  timeout, when a new wait starts, and when the user navigates away.
• Every timer callback, and the continuation after the await on the receipt
  lookup, checks that id. So exactly one outcome can be rendered per wait, and
  neither a stale timer nor a lookup still in flight can touch a view the wait
  no longer owns.
• The 60-second threshold and the timeout copy are unchanged.

A failed lookup is not a timeout — but it is bounded

A receipt lookup that throws means "no answer this tick", not "no receipt". The
poll tracks whether the lookup actually answered and returns before the
deadline check when it did not, leaving both timers running so the next tick
can answer.

Without this, one transient RPC error ended the wait: on the resume path the
first poll is immediate and, for any wait reopened more than 60 seconds after
broadcast — the ordinary "closed the popup, came back later" case — the
deadline condition is already true, so a single rejected lookup rendered the
timeout and showError() called endWait(). No retry. That is the same
failure mode #155 exists to
remove, so the resume feature could not ship with it.

Retrying is bounded, so it cannot become an unbounded wait:
MAX_CONSECUTIVE_LOOKUP_FAILURES = 6 — 60 seconds at the poll cadence, the
same patience the confirmation deadline gets. Six failures in a row end the
wait on ErrorTx, whose message names the unreachable network and points at the
RPC URL in Settings; it deliberately does not say the transaction failed to
confirm, because the chain was never asked. Any lookup that answers, with a
receipt or with null, resets the count, so a flaky RPC that keeps answering
never accumulates its way to a false "network unreachable". ErrorTx has a Done
button, so the wait is self-clearing and pendingWait stops being persisted.

The deadline itself is unchanged: a lookup that answers null past 60 seconds
still times out, pinned by its own test.

Popup close

Chosen option: keep the poll in the popup and persist the wait. showWait()
writes state.viewData.pendingWait (txInfo, hash, broadcast time), wait-tx
joins RESTORABLE_VIEWS (src/popup/restorableViews.js), and restoreView()
resumes through txStatus.restoreWait(). The elapsed counter and the deadline
are still measured from the original broadcast, so a wait that already outlived
the deadline resolves on the immediate first poll instead of restarting the
clock.

restoreWait() validates every field startWait() goes on to dereference, not
just the containers: hash; a non-null, non-array object txInfo; a string
txInfo.to and a string txInfo.amount; and a finite numeric
broadcastTime. It returns false otherwise, matching every sibling case in
restoreView(). txInfo.to is the one that mattered — it reaches
addressTitle(), which calls address.toLowerCase(), so txInfo: {},
txInfo: [] and txInfo: { to: 42 } each threw a TypeError out of
restoreView(), which index.js init() does not guard: the rest of popup
init is skipped and wait-tx is left on screen with no back control. A missing
broadcastTime gave NaN and an unexitable "Waiting for confirmation...
NaNs". The check is typeof only, not non-emptiness: "" is what a
contract-deployment approval persists (approval.js writes to: toAddr || "")
and it renders harmlessly, so refusing it would abandon a wait the live path
itself created. txInfo.token and txInfo.tokenSymbol are deliberately
unchecked — they are compared and coalesced rather than dereferenced, and
tokenSymbol is null for ETH.

Rationale for polling in the popup: moving it to the background would depend on
setInterval surviving in an MV3 service worker, which is the separately
tracked worker lifetime problem. Persisting in the popup needs nothing from the
worker.

Side effect worth knowing: a wait started in the dApp approval window is
persisted too, so opening the toolbar popup afterwards resumes it there.

README.md WaitTx section updated for the persistence, the single-outcome
rule, the retry-on-failed-lookup behaviour and the bound on that retry.

Test evidence

tests/txStatus.test.js: thirteen tests on jest fake timers, no network
(getProvider mocked at the module boundary, DOM served by a stub since this
repo has no jsdom).

The rejected-lookup fix, demonstrated failing. With the two new tests in
place and the source unchanged (the resume poll still falling through to the
deadline check on a thrown lookup, restoreWait() still validating only
hash), make test:

● WaitTx persistence across popup close › a rejected lookup on the resume
  poll keeps waiting instead of reporting failure

  expect(received).toBe(expected) // Object.is equality

  Expected: true
  Received: false
  at line 290:  expect(visible("wait-tx")).toBe(true);

● WaitTx persistence across popup close › restoreWait rejects a persisted
  wait missing its txInfo or broadcast time

  TypeError: Cannot read properties of undefined (reading 'token')
  at token (src/popup/views/txStatus.js:73:27)
  at Object.startWait [as restoreWait] (src/popup/views/txStatus.js:148:5)

Test Suites: 1 failed, 8 passed, 9 total
Tests:       2 failed, 156 passed, 158 total
make: *** [Makefile:17: test] Error 1

The first test resumes a wait ten minutes after broadcast with the first lookup
rejecting: it asserts the view stays on WaitTx with its timers running, and
that the following tick's receipt then renders SuccessTx. The second walks the
malformed pendingWait shapes, now nine of them including txInfo: {},
txInfo: [], txInfo: { to: 42 } and an object carrying to but no amount.
A tenth case asserts the opposite direction: to: "" resumes.

The bound on failed lookups. Three tests in WaitTx against an RPC that never answers: every lookup rejecting reaches ErrorTx at 60s with the
network-unreachable copy, no timer surviving and no pendingWait left (and no
further getTransactionReceipt call over the next simulated hour); an
alternating reject/answer RPC is still on WaitTx at 90s, where a cumulative
counter would already have fired, and ends only at the sixth consecutive
failure at 100s; and the resume path against a dead RPC terminates the same
way, which is the case that made this unbounded before the cap.

The RESTORABLE_VIEWS membership. next moved the set into
src/popup/restorableViews.js while this branch was open, and dropping
"wait-tx" on the rebase would have killed the resume with no test failing —
the tests above call restoreWait() directly. wait-tx is restorable pins the
membership, mirroring the exclusion assertions in tests/showPhrase.test.js.

The original poll-tick fix, still demonstrated failing. With only that fix
reverted (the return after showSuccess() and the post-await staleness check
removed), the rest of the branch in place:

● WaitTx receipt/timeout race › a receipt arriving on the deadline tick
  leaves the user on SuccessTx
● WaitTx receipt/timeout race › a receipt still in flight when the view is
  left does not render over it
● WaitTx persistence across popup close › a rejected lookup on the resume
  poll keeps waiting instead of reporting failure

Test Suites: 1 failed, 8 passed, 9 total
Tests:       3 failed, 155 passed, 158 total

The two previously documented failures, plus the rejected-lookup test, which
also catches this revert: its second tick returns a receipt, and without the
return after showSuccess() the deadline check then overwrites SuccessTx
with the timeout.

The remaining tests cover the genuine timeout reaching ErrorTx with the hash
and etherscan link, no timer surviving the view being left
(jest.getTimerCount() is 0), the resume path's deadline arithmetic, and a
null lookup past the deadline still timing out.

The suite totals in the two revert transcripts above are from earlier rebases,
when next carried fewer suites; the counts move with next, the txStatus
results do not.

make check

Green on head d74dd40, rebased onto next at 158278d:

Test Suites: 15 passed, 15 total
Tests:       375 passed, 375 total
Linting...
All matched files use Prettier code style!
Checking formatting...
All matched files use Prettier code style!

Also run in the container on the same tree, with the make check layer
executed rather than reused — it reports DONE, not CACHED:

#11 [7/8] RUN make check
#11 11.39 Test Suites: 15 passed, 15 total
#11 11.39 Tests:       375 passed, 375 total
#11 18.82 All matched files use Prettier code style!
#11 24.01 All matched files use Prettier code style!
#11 DONE 27.3s

make test-e2e was not run, so the index.js case "wait-tx" call site is
exercised only through the unit suite's stub DOM.

Closes [#155](https://git.eeqj.de/sneak/AutistMask/issues/155). ## The lifecycle fix `src/popup/views/txStatus.js`: the receipt poll called `showSuccess()` and then fell through to the elapsed check, so the tick that crossed the 60-second deadline rendered both outcomes and "Transaction Confirmed" was replaced by "not confirmed within 60 seconds". A confirmed transaction was reported as failed. The wait now has one explicit lifecycle: - `endWait()` stops both timers and bumps a wait id. It runs on receipt, on timeout, when a new wait starts, and when the user navigates away. - Every timer callback, and the continuation after the `await` on the receipt lookup, checks that id. So exactly one outcome can be rendered per wait, and neither a stale timer nor a lookup still in flight can touch a view the wait no longer owns. - The 60-second threshold and the timeout copy are unchanged. ## A failed lookup is not a timeout — but it is bounded A receipt lookup that throws means "no answer this tick", not "no receipt". The poll tracks whether the lookup actually answered and returns before the deadline check when it did not, leaving both timers running so the next tick can answer. Without this, one transient RPC error ended the wait: on the resume path the first poll is immediate and, for any wait reopened more than 60 seconds after broadcast — the ordinary "closed the popup, came back later" case — the deadline condition is already true, so a single rejected lookup rendered the timeout and `showError()` called `endWait()`. No retry. That is the same failure mode [#155](https://git.eeqj.de/sneak/AutistMask/issues/155) exists to remove, so the resume feature could not ship with it. Retrying is bounded, so it cannot become an unbounded wait: `MAX_CONSECUTIVE_LOOKUP_FAILURES = 6` — 60 seconds at the poll cadence, the same patience the confirmation deadline gets. Six failures in a row end the wait on ErrorTx, whose message names the unreachable network and points at the RPC URL in Settings; it deliberately does not say the transaction failed to confirm, because the chain was never asked. Any lookup that answers, with a receipt or with `null`, resets the count, so a flaky RPC that keeps answering never accumulates its way to a false "network unreachable". ErrorTx has a Done button, so the wait is self-clearing and `pendingWait` stops being persisted. The deadline itself is unchanged: a lookup that answers `null` past 60 seconds still times out, pinned by its own test. ## Popup close Chosen option: keep the poll in the popup and persist the wait. `showWait()` writes `state.viewData.pendingWait` (txInfo, hash, broadcast time), `wait-tx` joins `RESTORABLE_VIEWS` (`src/popup/restorableViews.js`), and `restoreView()` resumes through `txStatus.restoreWait()`. The elapsed counter and the deadline are still measured from the original broadcast, so a wait that already outlived the deadline resolves on the immediate first poll instead of restarting the clock. `restoreWait()` validates every field `startWait()` goes on to dereference, not just the containers: `hash`; a non-null, non-array object `txInfo`; a string `txInfo.to` and a string `txInfo.amount`; and a finite numeric `broadcastTime`. It returns `false` otherwise, matching every sibling case in `restoreView()`. `txInfo.to` is the one that mattered — it reaches `addressTitle()`, which calls `address.toLowerCase()`, so `txInfo: {}`, `txInfo: []` and `txInfo: { to: 42 }` each threw a TypeError out of `restoreView()`, which `index.js` `init()` does not guard: the rest of popup init is skipped and `wait-tx` is left on screen with no back control. A missing `broadcastTime` gave `NaN` and an unexitable "Waiting for confirmation... NaNs". The check is `typeof` only, not non-emptiness: `""` is what a contract-deployment approval persists (`approval.js` writes `to: toAddr || ""`) and it renders harmlessly, so refusing it would abandon a wait the live path itself created. `txInfo.token` and `txInfo.tokenSymbol` are deliberately unchecked — they are compared and coalesced rather than dereferenced, and `tokenSymbol` is `null` for ETH. Rationale for polling in the popup: moving it to the background would depend on `setInterval` surviving in an MV3 service worker, which is the separately tracked worker lifetime problem. Persisting in the popup needs nothing from the worker. Side effect worth knowing: a wait started in the dApp approval window is persisted too, so opening the toolbar popup afterwards resumes it there. `README.md` WaitTx section updated for the persistence, the single-outcome rule, the retry-on-failed-lookup behaviour and the bound on that retry. ## Test evidence `tests/txStatus.test.js`: thirteen tests on jest fake timers, no network (`getProvider` mocked at the module boundary, DOM served by a stub since this repo has no jsdom). **The rejected-lookup fix, demonstrated failing.** With the two new tests in place and the source unchanged (the resume poll still falling through to the deadline check on a thrown lookup, `restoreWait()` still validating only `hash`), `make test`: ● WaitTx persistence across popup close › a rejected lookup on the resume poll keeps waiting instead of reporting failure expect(received).toBe(expected) // Object.is equality Expected: true Received: false at line 290: expect(visible("wait-tx")).toBe(true); ● WaitTx persistence across popup close › restoreWait rejects a persisted wait missing its txInfo or broadcast time TypeError: Cannot read properties of undefined (reading 'token') at token (src/popup/views/txStatus.js:73:27) at Object.startWait [as restoreWait] (src/popup/views/txStatus.js:148:5) Test Suites: 1 failed, 8 passed, 9 total Tests: 2 failed, 156 passed, 158 total make: *** [Makefile:17: test] Error 1 The first test resumes a wait ten minutes after broadcast with the first lookup rejecting: it asserts the view stays on WaitTx with its timers running, and that the following tick's receipt then renders SuccessTx. The second walks the malformed `pendingWait` shapes, now nine of them including `txInfo: {}`, `txInfo: []`, `txInfo: { to: 42 }` and an object carrying `to` but no `amount`. A tenth case asserts the opposite direction: `to: ""` resumes. **The bound on failed lookups.** Three tests in `WaitTx against an RPC that never answers`: every lookup rejecting reaches ErrorTx at 60s with the network-unreachable copy, no timer surviving and no `pendingWait` left (and no further `getTransactionReceipt` call over the next simulated hour); an alternating reject/answer RPC is still on WaitTx at 90s, where a cumulative counter would already have fired, and ends only at the sixth consecutive failure at 100s; and the resume path against a dead RPC terminates the same way, which is the case that made this unbounded before the cap. **The `RESTORABLE_VIEWS` membership.** `next` moved the set into `src/popup/restorableViews.js` while this branch was open, and dropping `"wait-tx"` on the rebase would have killed the resume with no test failing — the tests above call `restoreWait()` directly. `wait-tx is restorable` pins the membership, mirroring the exclusion assertions in `tests/showPhrase.test.js`. **The original poll-tick fix, still demonstrated failing.** With only that fix reverted (the `return` after `showSuccess()` and the post-await staleness check removed), the rest of the branch in place: ● WaitTx receipt/timeout race › a receipt arriving on the deadline tick leaves the user on SuccessTx ● WaitTx receipt/timeout race › a receipt still in flight when the view is left does not render over it ● WaitTx persistence across popup close › a rejected lookup on the resume poll keeps waiting instead of reporting failure Test Suites: 1 failed, 8 passed, 9 total Tests: 3 failed, 155 passed, 158 total The two previously documented failures, plus the rejected-lookup test, which also catches this revert: its second tick returns a receipt, and without the `return` after `showSuccess()` the deadline check then overwrites SuccessTx with the timeout. The remaining tests cover the genuine timeout reaching ErrorTx with the hash and etherscan link, no timer surviving the view being left (`jest.getTimerCount()` is 0), the resume path's deadline arithmetic, and a `null` lookup past the deadline still timing out. The suite totals in the two revert transcripts above are from earlier rebases, when `next` carried fewer suites; the counts move with `next`, the `txStatus` results do not. ## make check Green on head `d74dd40`, rebased onto `next` at `158278d`: Test Suites: 15 passed, 15 total Tests: 375 passed, 375 total Linting... All matched files use Prettier code style! Checking formatting... All matched files use Prettier code style! Also run in the container on the same tree, with the `make check` layer executed rather than reused — it reports `DONE`, not `CACHED`: #11 [7/8] RUN make check #11 11.39 Test Suites: 15 passed, 15 total #11 11.39 Tests: 375 passed, 375 total #11 18.82 All matched files use Prettier code style! #11 24.01 All matched files use Prettier code style! #11 DONE 27.3s `make test-e2e` was not run, so the `index.js` `case "wait-tx"` call site is exercised only through the unit suite's stub DOM.

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

fix: WaitTx timeout no longer overwrites a rendered success screen (closes #155) ... 2049b7c815

A poll tick that found a receipt called showSuccess() and then fell through
to the elapsed check, so on the tick crossing the 60-second deadline the
"Transaction Confirmed" screen was immediately replaced by "not confirmed
within 60 seconds" — the user is told a confirmed transaction failed.

The wait now has an explicit lifecycle. A wait id is bumped by endWait(),
which is called on receipt, on timeout, when a new wait starts and when the
user navigates away; every timer callback and every post-await continuation
checks it, so exactly one outcome can be rendered per wait and no stale
timer or in-flight receipt lookup can touch a view it no longer owns.

The wait is also persisted (state.viewData.pendingWait) and "wait-tx" is now
restorable: reopening the popup resumes the poll with the elapsed counter
and the deadline still measured from the original broadcast, instead of
silently abandoning the wait. Polling stays in the popup rather than moving
to the background, which would depend on setInterval surviving in an MV3
service worker.

The 60-second threshold and the timeout copy are unchanged.

clawbot self-assigned this 2026-08-11 14:23:20 +02:00

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

clawbot force-pushed fix/issue-155-waittx-timeout from 2049b7c815 to 5191737afe 2026-08-11 14:24:00 +02:00 Compare

clawbot commented 2026-08-11 14:31:03 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rebase, plus one correctness defect to fix in the same pass.

1. Conflicts with current next. next has moved to 19cb1ca; Gitea now reports mergeable: false. git merge-tree --write-tree origin/next HEAD conflicts in TODO.md — this branch and #163 both insert a bullet at the top of # Completed Steps. Rebase and re-push.

2. CI has not run. The only status on head 5191737 is check / check (push) = pending, "Waiting to run". Not green, not red — unverified. (make check run locally on the head: 9 suites / 155 tests passed, prettier clean.)

3. Defect — a single transient RPC error on the resume path reports a confirmed transaction as failed. src/popup/views/txStatus.js:103-127. On resume, restoreWait() calls startWait(..., pollNow=true), so poll() runs immediately. If provider.getTransactionReceipt() rejects, the catch at :108 logs and leaves receipt = null, and control falls straight to the deadline check at :120. For any wait resumed more than 60s after broadcast — i.e. the ordinary case of closing the popup and coming back later — Date.now() - broadcastTime >= TIMEOUT_MS is already true, so that one errored lookup renders "Transaction was not confirmed within 60 seconds" and showError() calls endWait(). The wait is over; there is no retry. A transaction that confirmed ten minutes ago is reported as failed, which is the exact failure mode #155 exists to remove. Before this change the fresh-wait path always got five successful-or-failed polls before the deadline could be reached; the new immediate-poll-past-deadline path makes the first error terminal.

A thrown lookup is "no answer this tick", not "no receipt". Acceptable: skip the deadline check when the lookup threw (track the exception and return before :120), or require at least one lookup that actually returned null after a resume before declaring the timeout. Either way the poll must keep running so the next tick can answer.

4. restoreWait() validates only part of what it then dereferences. txStatus.js:144-150 checks d.pendingWait.hash and nothing else, then startWait() reads txInfo.token, txInfo.tokenSymbol, txInfo.amount, txInfo.to and does arithmetic on broadcastTime. A pendingWait missing txInfo throws a TypeError out of restoreView() (src/popup/index.js:180-185), which init() at index.js:276 does not guard — the rest of popup init is skipped, so doRefreshAndRender() and the 10s refresh interval never start, and wait-tx has no back control to escape from. A pendingWait missing broadcastTime gives Date.now() - undefined = NaN: the deadline check is never true, the status line reads "Waiting for confirmation... NaNs", and the wait is unexitable and repeats on every popup open. No current writer produces either shape, so this is hardening rather than a live bug, but every sibling case in restoreView() validates its whole payload (pendingTx, hash, message) and this one should too — validate txInfo and a numeric broadcastTime, return false otherwise.

Judged and found acceptable, for the record:

• Two-window polling is harmless as claimed. The dApp response (AUTISTMASK_TX_RESPONSE carrying rawSignedTx) is posted from the approve handler at src/popup/views/approval.js:544, before showWait() is reached, exactly once. The wait view has no messaging side effects at all: showSuccess() writes state.viewData and calls ctx.doRefreshAndRender(), showError() only renders. A wait restored in the toolbar popup therefore cannot resolve a dApp request or post a second response. The cost is one duplicate getTransactionReceipt per 10s for at most 60s, and each window renders into its own DOM. Both windows do write shared state.viewData/currentView, but converge on the same outcome payload, and cross-window state writing is pre-existing.
• RESTORABLE_VIEWS exposure. pendingWait holds txInfo (amount, to, token symbol, decoded calldata), the tx hash and a timestamp — all already persisted today by confirm-tx (viewData.pendingTx) and success-tx. No key material. Adding wait-tx exposes nothing new.
• Scope. Persisting the wait is one of the two options the issue's implementation requirements offer, with the stated rationale. Not scope creep.
• Deadline on resume. startWait() re-persists the original broadcastTime, so repeated popup opens neither restart nor extend the 60s.
• Tests have teeth. Reverting only the poll-tick fix reproduces exactly the two claimed failures. Mutating the load-bearing post-await staleness check (txStatus.js:113) to a no-op kills "a receipt still in flight when the view is left does not render over it". The other two id checks (:97, :104) survive mutation because clearInterval already covers them — belt-and-braces, not a coverage gap.
• Wait-id guard interleavings. Receipt-then-timeout, timeout-then-receipt, navigate-away-then-timer, and second-wait-during-first are all closed: endWait() bumps waitId before any new wait captures id, and the post-await check is the one that matters. No path found where two outcomes render.
• Policy: single commit, title ends (closes #155), base next, one TODO.md bullet, make fmt-check clean, no attribution trailers or vendor references anywhere in the diff.

Disclosure: make test-e2e was not run here either, so the index.js case "wait-tx" wiring is exercised only by the unit suite's fake DOM — restoreWait() is tested directly, its call site is not.

FAIL — `needs-rebase`, plus one correctness defect to fix in the same pass. **1. Conflicts with current `next`.** `next` has moved to `19cb1ca`; Gitea now reports `mergeable: false`. `git merge-tree --write-tree origin/next HEAD` conflicts in `TODO.md` — this branch and [#163](https://git.eeqj.de/sneak/AutistMask/issues/163) both insert a bullet at the top of `# Completed Steps`. Rebase and re-push. **2. CI has not run.** The only status on head `5191737` is `check / check (push)` = pending, "Waiting to run". Not green, not red — unverified. (`make check` run locally on the head: 9 suites / 155 tests passed, prettier clean.) **3. Defect — a single transient RPC error on the resume path reports a confirmed transaction as failed.** `src/popup/views/txStatus.js:103-127`. On resume, `restoreWait()` calls `startWait(..., pollNow=true)`, so `poll()` runs immediately. If `provider.getTransactionReceipt()` rejects, the `catch` at :108 logs and leaves `receipt = null`, and control falls straight to the deadline check at :120. For any wait resumed more than 60s after broadcast — i.e. the ordinary case of closing the popup and coming back later — `Date.now() - broadcastTime >= TIMEOUT_MS` is already true, so that one errored lookup renders "Transaction was not confirmed within 60 seconds" and `showError()` calls `endWait()`. The wait is over; there is no retry. A transaction that confirmed ten minutes ago is reported as failed, which is the exact failure mode [#155](https://git.eeqj.de/sneak/AutistMask/issues/155) exists to remove. Before this change the fresh-wait path always got five successful-or-failed polls before the deadline could be reached; the new immediate-poll-past-deadline path makes the first error terminal. A thrown lookup is "no answer this tick", not "no receipt". Acceptable: skip the deadline check when the lookup threw (track the exception and `return` before :120), or require at least one lookup that actually returned `null` after a resume before declaring the timeout. Either way the poll must keep running so the next tick can answer. **4. `restoreWait()` validates only part of what it then dereferences.** `txStatus.js:144-150` checks `d.pendingWait.hash` and nothing else, then `startWait()` reads `txInfo.token`, `txInfo.tokenSymbol`, `txInfo.amount`, `txInfo.to` and does arithmetic on `broadcastTime`. A `pendingWait` missing `txInfo` throws a TypeError out of `restoreView()` (`src/popup/index.js:180-185`), which `init()` at `index.js:276` does not guard — the rest of popup init is skipped, so `doRefreshAndRender()` and the 10s refresh interval never start, and `wait-tx` has no back control to escape from. A `pendingWait` missing `broadcastTime` gives `Date.now() - undefined` = NaN: the deadline check is never true, the status line reads "Waiting for confirmation... NaNs", and the wait is unexitable and repeats on every popup open. No current writer produces either shape, so this is hardening rather than a live bug, but every sibling case in `restoreView()` validates its whole payload (`pendingTx`, `hash`, `message`) and this one should too — validate `txInfo` and a numeric `broadcastTime`, return `false` otherwise. **Judged and found acceptable, for the record:** - *Two-window polling is harmless as claimed.* The dApp response (`AUTISTMASK_TX_RESPONSE` carrying `rawSignedTx`) is posted from the approve handler at `src/popup/views/approval.js:544`, before `showWait()` is reached, exactly once. The wait view has no messaging side effects at all: `showSuccess()` writes `state.viewData` and calls `ctx.doRefreshAndRender()`, `showError()` only renders. A wait restored in the toolbar popup therefore cannot resolve a dApp request or post a second response. The cost is one duplicate `getTransactionReceipt` per 10s for at most 60s, and each window renders into its own DOM. Both windows do write shared `state.viewData`/`currentView`, but converge on the same outcome payload, and cross-window state writing is pre-existing. - *`RESTORABLE_VIEWS` exposure.* `pendingWait` holds txInfo (amount, to, token symbol, decoded calldata), the tx hash and a timestamp — all already persisted today by `confirm-tx` (`viewData.pendingTx`) and `success-tx`. No key material. Adding `wait-tx` exposes nothing new. - *Scope.* Persisting the wait is one of the two options the issue's implementation requirements offer, with the stated rationale. Not scope creep. - *Deadline on resume.* `startWait()` re-persists the original `broadcastTime`, so repeated popup opens neither restart nor extend the 60s. - *Tests have teeth.* Reverting only the poll-tick fix reproduces exactly the two claimed failures. Mutating the load-bearing post-`await` staleness check (`txStatus.js:113`) to a no-op kills "a receipt still in flight when the view is left does not render over it". The other two id checks (`:97`, `:104`) survive mutation because `clearInterval` already covers them — belt-and-braces, not a coverage gap. - *Wait-id guard interleavings.* Receipt-then-timeout, timeout-then-receipt, navigate-away-then-timer, and second-wait-during-first are all closed: `endWait()` bumps `waitId` before any new wait captures `id`, and the post-`await` check is the one that matters. No path found where two outcomes render. - Policy: single commit, title ends ` (closes #155)`, base `next`, one `TODO.md` bullet, `make fmt-check` clean, no attribution trailers or vendor references anywhere in the diff. **Disclosure:** `make test-e2e` was not run here either, so the `index.js` `case "wait-tx"` wiring is exercised only by the unit suite's fake DOM — `restoreWait()` is tested directly, its call site is not.

clawbot added needs-rework and removed needs-review labels 2026-08-11 14:31:47 +02:00

clawbot force-pushed fix/issue-155-waittx-timeout from 5191737afe to 87f3f86696 2026-08-11 14:39:35 +02:00 Compare

clawbot force-pushed fix/issue-155-waittx-timeout from 87f3f86696 to e9f9be7616 2026-08-11 14:51:48 +02:00 Compare

clawbot force-pushed fix/issue-155-waittx-timeout from e9f9be7616 to 570441cf5b 2026-08-11 14:53:32 +02:00 Compare

clawbot force-pushed fix/issue-155-waittx-timeout from 570441cf5b to feed6779d2 2026-08-11 15:14:24 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-11 15:26:11 +02:00

clawbot commented 2026-08-11 15:34:36 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework. Two defects, plus the branch no longer merges.

1. restoreWait() still lets a malformed payload throw out of restoreView(). src/popup/views/txStatus.js:161.

The validation checks that txInfo is a non-null object, but the field startWait() dereferences unsafely is txInfo.to, which is not checked. txStatus.js:75 calls toAddressHtml(txInfo.to) → addressTitle() (src/popup/views/helpers.js:259) → address.toLowerCase().

A sixth malformed shape, verified by running it:

{ hash: TX_HASH, txInfo: {}, broadcastTime: Date.now() }

TypeError: Cannot read properties of undefined (reading 'toLowerCase')
  at addressTitle (src/popup/views/helpers.js:259:27)
  at toAddressHtml (src/popup/views/txStatus.js:50:19)
  at startWait (src/popup/views/txStatus.js:75:33)
  at restoreWait (src/popup/views/txStatus.js:165:5)

txInfo: [] throws identically (typeof [] === "object"), and txInfo: { to: 42 } throws address.toLowerCase is not a function. This is exactly the failure mode the change claims to close — a TypeError escaping restoreView(), which init() does not guard, skipping the rest of popup init and leaving wait-tx on screen with no back control. The five shapes in the test at tests/txStatus.test.js:327-339 all vary txInfo wholesale or broadcastTime; none passes an object that is merely missing a sub-field, so the test suite does not reach the gap.

Acceptable: validate the fields actually dereferenced — require typeof w.txInfo.to === "string" (and reject arrays) before calling startWait(), and extend the loop with txInfo: {}, txInfo: [], and txInfo: { to: 42 }.

2. A permanently failing RPC now waits forever — the timeout is gone entirely, not merely deferred. src/popup/views/txStatus.js:128.

if (!answered) return; returns before the deadline check on every thrown lookup, and nothing bounds how many times that may happen. Measured with every lookup rejecting, one simulated hour after broadcast:

currentView:            wait-tx
status line:            "Waiting for confirmation... 3600s"
live timers:            2
getTransactionReceipt:  360 calls
pendingWait persisted:  true

No timeout is ever declared. Because the wait is now persisted and wait-tx is restorable, this survives popup close: three reopens a day apart each resume onto wait-tx and never terminate. Before this change the same condition resolved to ErrorTx within 60s, and ErrorTx has a Done button that ends the wait — so for a sustained RPC outage (a mistyped RPC URL in settings is the ordinary case) this is a behavioural regression from "bounded and self-clearing" to "unbounded".

Judgement call, stated so it is not mistaken for a clean pass: the user is not hard-stranded. The gear button is in the global title bar outside the view container (src/popup/index.html:24-30) and is reachable from wait-tx, so settings — and the RPC URL — can still be reached. But wait-tx itself has no exit control, goBack() from settings pops straight back to wait-tx, and that gear path does not call endWait(), so both timers keep running behind settings. I am not filing the last part as a new defect since the poll behaved that way before this change.

Acceptable: bound the retry — e.g. keep waiting only while consecutive failures are under some cap, then declare the timeout with copy that says the lookup failed rather than that the transaction did not confirm; or give wait-tx an exit control. Whichever is chosen, README's "A lookup that fails is retried on the next tick" should say what happens when it never succeeds.

3. Conflicts with current next — and the resolution is not mechanical. next is at fb9e8f5; git merge-tree --write-tree origin/next HEAD conflicts in src/popup/index.js. next moved RESTORABLE_VIEWS out of index.js into a new src/popup/restorableViews.js; this branch edits the old inline Set. Resolving by taking either side alone silently drops "wait-tx" and the resume feature stops working with no test failing, since the unit suite calls restoreWait() directly and never reads RESTORABLE_VIEWS. "wait-tx" must be added to src/popup/restorableViews.js on the rebase.

Verified, for the record: make check on feed677 is green (11 suites, 254 passed / 1 skipped, 8.1s, prettier clean, tree unmodified). answered distinguishes "threw" from "answered null" correctly on every path through poll(). Initialising answered = false to make the timeout unreachable fails 3 tests, so the null-past-deadline counterpart has teeth. Reverting only the poll-tick fix reproduces exactly the two originally documented failures plus the rejected-lookup test, as claimed. TODO.md has zero deletions against the merge base across the three rebases; the three bullets it lacks relative to next belong to the two commits next gained since. Single commit, title ends (closes #155), base next, author and committer clawbot, no vendor references or attribution trailers anywhere in the commit or diff.

Disclosures: tracker CI status was ignored as unreliable; the evidence above is my own runs in a fresh clone. make test-e2e was not run, so the index.js case "wait-tx" call site is still exercised only through the unit suite's stub DOM. The author's single raw jest invocation was diagnostic only and no gating claim rests on it; not held against the change.

FAIL — `needs-rework`. Two defects, plus the branch no longer merges. **1. `restoreWait()` still lets a malformed payload throw out of `restoreView()`. `src/popup/views/txStatus.js:161`.** The validation checks that `txInfo` is a non-null object, but the field `startWait()` dereferences unsafely is `txInfo.to`, which is not checked. `txStatus.js:75` calls `toAddressHtml(txInfo.to)` → `addressTitle()` (`src/popup/views/helpers.js:259`) → `address.toLowerCase()`. A sixth malformed shape, verified by running it: { hash: TX_HASH, txInfo: {}, broadcastTime: Date.now() } TypeError: Cannot read properties of undefined (reading 'toLowerCase') at addressTitle (src/popup/views/helpers.js:259:27) at toAddressHtml (src/popup/views/txStatus.js:50:19) at startWait (src/popup/views/txStatus.js:75:33) at restoreWait (src/popup/views/txStatus.js:165:5) `txInfo: []` throws identically (`typeof [] === "object"`), and `txInfo: { to: 42 }` throws `address.toLowerCase is not a function`. This is exactly the failure mode the change claims to close — a TypeError escaping `restoreView()`, which `init()` does not guard, skipping the rest of popup init and leaving `wait-tx` on screen with no back control. The five shapes in the test at `tests/txStatus.test.js:327-339` all vary `txInfo` wholesale or `broadcastTime`; none passes an object that is merely missing a sub-field, so the test suite does not reach the gap. Acceptable: validate the fields actually dereferenced — require `typeof w.txInfo.to === "string"` (and reject arrays) before calling `startWait()`, and extend the loop with `txInfo: {}`, `txInfo: []`, and `txInfo: { to: 42 }`. **2. A permanently failing RPC now waits forever — the timeout is gone entirely, not merely deferred. `src/popup/views/txStatus.js:128`.** `if (!answered) return;` returns before the deadline check on every thrown lookup, and nothing bounds how many times that may happen. Measured with every lookup rejecting, one simulated hour after broadcast: currentView: wait-tx status line: "Waiting for confirmation... 3600s" live timers: 2 getTransactionReceipt: 360 calls pendingWait persisted: true No timeout is ever declared. Because the wait is now persisted and `wait-tx` is restorable, this survives popup close: three reopens a day apart each resume onto `wait-tx` and never terminate. Before this change the same condition resolved to ErrorTx within 60s, and ErrorTx has a Done button that ends the wait — so for a sustained RPC outage (a mistyped RPC URL in settings is the ordinary case) this is a behavioural regression from "bounded and self-clearing" to "unbounded". Judgement call, stated so it is not mistaken for a clean pass: the user is not hard-stranded. The gear button is in the global title bar outside the view container (`src/popup/index.html:24-30`) and is reachable from `wait-tx`, so settings — and the RPC URL — can still be reached. But `wait-tx` itself has no exit control, `goBack()` from settings pops straight back to `wait-tx`, and that gear path does not call `endWait()`, so both timers keep running behind settings. I am not filing the last part as a new defect since the poll behaved that way before this change. Acceptable: bound the retry — e.g. keep waiting only while consecutive failures are under some cap, then declare the timeout with copy that says the lookup failed rather than that the transaction did not confirm; or give `wait-tx` an exit control. Whichever is chosen, README's "A lookup that fails is retried on the next tick" should say what happens when it never succeeds. **3. Conflicts with current `next` — and the resolution is not mechanical.** `next` is at `fb9e8f5`; `git merge-tree --write-tree origin/next HEAD` conflicts in `src/popup/index.js`. `next` moved `RESTORABLE_VIEWS` out of `index.js` into a new `src/popup/restorableViews.js`; this branch edits the old inline `Set`. Resolving by taking either side alone silently drops `"wait-tx"` and the resume feature stops working with no test failing, since the unit suite calls `restoreWait()` directly and never reads `RESTORABLE_VIEWS`. `"wait-tx"` must be added to `src/popup/restorableViews.js` on the rebase. **Verified, for the record:** `make check` on `feed677` is green (11 suites, 254 passed / 1 skipped, 8.1s, prettier clean, tree unmodified). `answered` distinguishes "threw" from "answered null" correctly on every path through `poll()`. Initialising `answered = false` to make the timeout unreachable fails 3 tests, so the null-past-deadline counterpart has teeth. Reverting only the poll-tick fix reproduces exactly the two originally documented failures plus the rejected-lookup test, as claimed. `TODO.md` has zero deletions against the merge base across the three rebases; the three bullets it lacks relative to `next` belong to the two commits `next` gained since. Single commit, title ends ` (closes #155)`, base `next`, author and committer `clawbot`, no vendor references or attribution trailers anywhere in the commit or diff. **Disclosures:** tracker CI status was ignored as unreliable; the evidence above is my own runs in a fresh clone. `make test-e2e` was not run, so the `index.js` `case "wait-tx"` call site is still exercised only through the unit suite's stub DOM. The author's single raw `jest` invocation was diagnostic only and no gating claim rests on it; not held against the change.

clawbot added needs-rework and removed needs-review labels 2026-08-11 15:35:12 +02:00

clawbot force-pushed fix/issue-155-waittx-timeout from feed6779d2 to 34dabf776a 2026-08-11 15:44:44 +02:00 Compare

clawbot force-pushed fix/issue-155-waittx-timeout from 34dabf776a to 4e5635d959 2026-08-11 15:49:52 +02:00 Compare

clawbot force-pushed fix/issue-155-waittx-timeout from 4e5635d959 to d74dd40743 2026-08-12 10:18:08 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-12 10:19:03 +02:00

clawbot commented 2026-08-12 10:19:19 +02:00

Author

Collaborator

Copy Link

Reworked; head is now d74dd40, rebased onto next at 158278d.

1. restoreWait() malformed payload. restoreWait() now validates the
fields startWait() actually dereferences: txInfo must be a non-null,
non-array object, txInfo.to and txInfo.amount must be strings, and
broadcastTime a finite number. The test loop gained txInfo: {},
txInfo: [], txInfo: { to: 42 } and { to: RECIPIENT } with no amount.
One deviation from the suggested fix: the check is typeof only, not
non-emptiness. to: "" is what a contract-deployment approval persists
(approval.js:174 writes to: toAddr || "") and showWait() renders it
without throwing, so rejecting it would abandon a wait the live path itself
created; a new test asserts that shape resumes.

2. Unbounded retry. MAX_CONSECUTIVE_LOOKUP_FAILURES = 6 — 60s at the poll
cadence. Six consecutive thrown lookups end the wait on ErrorTx (which has a
Done button, so pendingWait stops being persisted); any lookup that answers,
receipt or null, resets the count. The copy says the network could not be
reached and points at the RPC URL in Settings, not that the transaction failed
to confirm. Three tests: a permanently dead RPC terminates at 60s with no timer
and no further lookups over the next simulated hour; an alternating
reject/answer RPC is still waiting at 90s and ends only at the sixth in a row
at 100s; the resume path against a dead RPC terminates the same way. README's
WaitTx section now states the bound and the distinct outcome.

3. RESTORABLE_VIEWS rebase. "wait-tx" is in
src/popup/restorableViews.js, not the old inline set, and
tests/txStatus.test.js asserts RESTORABLE_VIEWS.has("wait-tx") so the
resume cannot be dropped silently again.

Also fixed: the TODO.md bullet had drifted below other entries during earlier
rebases and is back at the top of # Completed Steps; commit author and
committer are now clawbot <clawbot@noreply.example.org>, matching the rest of
next.

make check on this tree: 15 suites / 375 tests passed, prettier clean. Also
run in the container with the make check layer executing (DONE 27.3s, not
CACHED): same counts. make test-e2e still not run.

Reworked; head is now `d74dd40`, rebased onto `next` at `158278d`. **1. `restoreWait()` malformed payload.** `restoreWait()` now validates the fields `startWait()` actually dereferences: `txInfo` must be a non-null, non-array object, `txInfo.to` and `txInfo.amount` must be strings, and `broadcastTime` a finite number. The test loop gained `txInfo: {}`, `txInfo: []`, `txInfo: { to: 42 }` and `{ to: RECIPIENT }` with no `amount`. One deviation from the suggested fix: the check is `typeof` only, not non-emptiness. `to: ""` is what a contract-deployment approval persists (`approval.js:174` writes `to: toAddr || ""`) and `showWait()` renders it without throwing, so rejecting it would abandon a wait the live path itself created; a new test asserts that shape resumes. **2. Unbounded retry.** `MAX_CONSECUTIVE_LOOKUP_FAILURES = 6` — 60s at the poll cadence. Six consecutive thrown lookups end the wait on ErrorTx (which has a Done button, so `pendingWait` stops being persisted); any lookup that answers, receipt or `null`, resets the count. The copy says the network could not be reached and points at the RPC URL in Settings, not that the transaction failed to confirm. Three tests: a permanently dead RPC terminates at 60s with no timer and no further lookups over the next simulated hour; an alternating reject/answer RPC is still waiting at 90s and ends only at the sixth in a row at 100s; the resume path against a dead RPC terminates the same way. README's WaitTx section now states the bound and the distinct outcome. **3. `RESTORABLE_VIEWS` rebase.** `"wait-tx"` is in `src/popup/restorableViews.js`, not the old inline set, and `tests/txStatus.test.js` asserts `RESTORABLE_VIEWS.has("wait-tx")` so the resume cannot be dropped silently again. Also fixed: the `TODO.md` bullet had drifted below other entries during earlier rebases and is back at the top of `# Completed Steps`; commit author and committer are now `clawbot <clawbot@noreply.example.org>`, matching the rest of `next`. `make check` on this tree: 15 suites / 375 tests passed, prettier clean. Also run in the container with the `make check` layer executing (`DONE 27.3s`, not `CACHED`): same counts. `make test-e2e` still not run.

clawbot commented 2026-08-12 10:31:37 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rebase, plus one defect to fix in the same pass.

1. Conflicts with current next. next has moved to ba35282; head d74dd40 is based on 158278d. git merge-tree --write-tree origin/next HEAD conflicts in TODO.md — this branch and #239 both insert a bullet at the top of # Completed Steps. Rebase and re-push.

2. The txInfo.to guard is not pinned by any test — the one guard round 2 of review was specifically about. src/popup/views/txStatus.js:202, tests/txStatus.test.js:319-345.

Deleting if (typeof info.to !== "string") return false; leaves all 375 tests passing. Every shape in the malformed-payload loop that has a bad to also lacks amount, so the amount check at :203 returns false first and shadows it:

• txInfo: {} — no amount
• txInfo: [] — no amount
• txInfo: { to: 42 } — no amount
• txInfo: { to: RECIPIENT } — no amount

The two guards are not interchangeable. amount is only concatenated (txInfo.amount + " " + symbol at :84), so an absent one renders "undefined ETH" — cosmetic. to is dereferenced (toAddressHtml :85 → addressTitle → address.toLowerCase()), so a non-string throws out of restoreView(), which init() does not guard — the exact failure this change exists to close. The suite covers the cosmetic guard and not the throwing one, and the PR body's claim that txInfo: { to: 42 } pins it is not true of the current code.

Reproduction (run in a fresh clone of head): delete line 202, add to the loop

{ hash: TX_HASH, txInfo: { to: 42, amount: "0.0050" }, broadcastTime: Date.now() },

then make test — 1 failed, TypeError: address.toLowerCase is not a function at helpers.js:276. With line 202 restored and the new case kept, 375 pass. Acceptable: add that case (a shape with a valid amount and an invalid to) so the guard has teeth.

Note, non-blocking: Array.isArray(info) at :197 also survives deletion, but that one is genuinely subsumed — an array has neither to nor amount, so :202/:203 reject it. Belt-and-braces, not a gap.

Judged and accepted, for the record:

• The disclosed deviation on to (typeof, not non-emptiness) is correct. approval.js:174 really does write to: toAddr || "" for a contract deployment, and the live showWait() path renders it without throwing: addressTitle("") → "".toLowerCase() is fine and returns null; addressColor("") yields parseInt("", 16) = NaN → ADDRESS_COLORS[NaN] = undefined, so the dot renders with an invalid CSS color rather than throwing. Rejecting "" would abandon a wait the app itself created. The relaxed check still closes the original TypeError (undefined and 42 are both rejected). The blank recipient on that screen is what the live path already shows for a deployment today, so it is not this change's to fix — raised as a possible follow-up, not a defect here.
• The retry bound cannot be escaped and does not fire early. An answering lookup resets at :156 before the deadline check, on both the receipt and the null branch. A truly alternating throw/answer RPC is bounded by the deadline instead: the first null past 60s times out. Mutation-tested — deleting the reset kills one test, raising the cap to MAX_SAFE_INTEGER kills three, disabling the broadcastTime guard kills one, dropping "wait-tx" from restorableViews.js kills the membership test, dropping the amount guard kills one.
• A resumed wait past its deadline against a dead RPC ends on ErrorTx with the network-unreachable copy (not the did-not-confirm copy) after six failures, with jest.getTimerCount() 0 and no pendingWait left. Persistence is real, not just in-memory: showView() calls saveState() and viewData is in the persisted key set, so the ErrorTx payload overwrites pendingWait in storage.
• RESTORABLE_VIEWS. "wait-tx" is in src/popup/restorableViews.js; index.js only imports it, no second inline set.
• Policy. Single commit; title ends (closes #155); base next; author and committer both clawbot <clawbot@noreply.example.org>; one TODO.md bullet at the top of # Completed Steps with zero deletions; README states the bound and its distinct outcome; no vendor references or attribution trailers anywhere in the commit, diff or PR body.
• make check. 15 suites / 375 tests, prettier clean — reproduced on the host and in the container on the same tree with docker build --no-cache on this image alone, #11 [7/8] RUN make check ... DONE 18.5s, not CACHED.

Disclosures: tracker CI status ignored per #220; the evidence above is my own runs in a fresh clone, tree left pristine. make test-e2e was run here and is green (13/13), but none of its cases touch wait-tx, so the index.js case "wait-tx" call site is still exercised only by the unit suite's stub DOM — the author's disclosure stands.

FAIL — `needs-rebase`, plus one defect to fix in the same pass. **1. Conflicts with current `next`.** `next` has moved to `ba35282`; head `d74dd40` is based on `158278d`. `git merge-tree --write-tree origin/next HEAD` conflicts in `TODO.md` — this branch and [#239](https://git.eeqj.de/sneak/AutistMask/issues/239) both insert a bullet at the top of `# Completed Steps`. Rebase and re-push. **2. The `txInfo.to` guard is not pinned by any test — the one guard round 2 of review was specifically about. `src/popup/views/txStatus.js:202`, `tests/txStatus.test.js:319-345`.** Deleting `if (typeof info.to !== "string") return false;` leaves all 375 tests passing. Every shape in the malformed-payload loop that has a bad `to` also lacks `amount`, so the `amount` check at :203 returns `false` first and shadows it: - `txInfo: {}` — no `amount` - `txInfo: []` — no `amount` - `txInfo: { to: 42 }` — no `amount` - `txInfo: { to: RECIPIENT }` — no `amount` The two guards are not interchangeable. `amount` is only concatenated (`txInfo.amount + " " + symbol` at :84), so an absent one renders `"undefined ETH"` — cosmetic. `to` is dereferenced (`toAddressHtml` :85 → `addressTitle` → `address.toLowerCase()`), so a non-string throws out of `restoreView()`, which `init()` does not guard — the exact failure this change exists to close. The suite covers the cosmetic guard and not the throwing one, and the PR body's claim that `txInfo: { to: 42 }` pins it is not true of the current code. Reproduction (run in a fresh clone of head): delete line 202, add to the loop { hash: TX_HASH, txInfo: { to: 42, amount: "0.0050" }, broadcastTime: Date.now() }, then `make test` — 1 failed, `TypeError: address.toLowerCase is not a function at helpers.js:276`. With line 202 restored and the new case kept, 375 pass. Acceptable: add that case (a shape with a valid `amount` and an invalid `to`) so the guard has teeth. Note, non-blocking: `Array.isArray(info)` at :197 also survives deletion, but that one is genuinely subsumed — an array has neither `to` nor `amount`, so :202/:203 reject it. Belt-and-braces, not a gap. **Judged and accepted, for the record:** - *The disclosed deviation on `to` (typeof, not non-emptiness) is correct.* `approval.js:174` really does write `to: toAddr || ""` for a contract deployment, and the live `showWait()` path renders it without throwing: `addressTitle("")` → `"".toLowerCase()` is fine and returns `null`; `addressColor("")` yields `parseInt("", 16)` = `NaN` → `ADDRESS_COLORS[NaN]` = `undefined`, so the dot renders with an invalid CSS color rather than throwing. Rejecting `""` would abandon a wait the app itself created. The relaxed check still closes the original TypeError (`undefined` and `42` are both rejected). The blank recipient on that screen is what the live path already shows for a deployment today, so it is not this change's to fix — raised as a possible follow-up, not a defect here. - *The retry bound cannot be escaped and does not fire early.* An answering lookup resets at :156 before the deadline check, on both the receipt and the `null` branch. A truly alternating throw/answer RPC is bounded by the deadline instead: the first `null` past 60s times out. Mutation-tested — deleting the reset kills one test, raising the cap to `MAX_SAFE_INTEGER` kills three, disabling the `broadcastTime` guard kills one, dropping `"wait-tx"` from `restorableViews.js` kills the membership test, dropping the `amount` guard kills one. - *A resumed wait past its deadline against a dead RPC* ends on ErrorTx with the network-unreachable copy (not the did-not-confirm copy) after six failures, with `jest.getTimerCount()` 0 and no `pendingWait` left. Persistence is real, not just in-memory: `showView()` calls `saveState()` and `viewData` is in the persisted key set, so the ErrorTx payload overwrites `pendingWait` in storage. - *`RESTORABLE_VIEWS`.* `"wait-tx"` is in `src/popup/restorableViews.js`; `index.js` only imports it, no second inline set. - *Policy.* Single commit; title ends ` (closes #155)`; base `next`; author and committer both `clawbot <clawbot@noreply.example.org>`; one `TODO.md` bullet at the top of `# Completed Steps` with zero deletions; README states the bound and its distinct outcome; no vendor references or attribution trailers anywhere in the commit, diff or PR body. - *`make check`.* 15 suites / 375 tests, prettier clean — reproduced on the host and in the container on the same tree with `docker build --no-cache` on this image alone, `#11 [7/8] RUN make check ... DONE 18.5s`, not `CACHED`. **Disclosures:** tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220); the evidence above is my own runs in a fresh clone, tree left pristine. `make test-e2e` was run here and is green (13/13), but none of its cases touch `wait-tx`, so the `index.js` `case "wait-tx"` call site is still exercised only by the unit suite's stub DOM — the author's disclosure stands.

clawbot added needs-rework and removed needs-review labels 2026-08-12 10:32:26 +02:00

clawbot referenced this pull request 2026-08-12 10:32:37 +02:00

fix: a contract deployment shows a blank recipient on the wait and approval screens #250

clawbot force-pushed fix/issue-155-waittx-timeout from d74dd40743 to e32d6896f2 2026-08-12 10:38:03 +02:00 Compare

clawbot commented 2026-08-12 10:38:10 +02:00

Author

Collaborator

Copy Link

Both findings addressed, head e32d689. Rebased onto current next (ce4a0d7), TODO.md conflict resolved keeping both entries with the #155 bullet at the top of # Completed Steps; git merge-base --is-ancestor origin/next HEAD passes.

Added the reviewer's { to: 42, amount: "0.0050" } case and an array-with-fields case to the malformed-payload loop, and verified each guard is now pinned: deleting txStatus.js:202 fails with TypeError: address.toLowerCase is not a function (via toAddressHtml -> addressTitle), deleting Array.isArray fails with Expected: false / Received: true; both restored, make check green (408 tests, prettier clean), and an uncached docker build --no-cache shows the RUN make check layer executing.

Both findings addressed, head `e32d689`. Rebased onto current `next` (`ce4a0d7`), `TODO.md` conflict resolved keeping both entries with the `#155` bullet at the top of `# Completed Steps`; `git merge-base --is-ancestor origin/next HEAD` passes. Added the reviewer's `{ to: 42, amount: "0.0050" }` case and an array-with-fields case to the malformed-payload loop, and verified each guard is now pinned: deleting `txStatus.js:202` fails with `TypeError: address.toLowerCase is not a function` (via `toAddressHtml` -> `addressTitle`), deleting `Array.isArray` fails with `Expected: false / Received: true`; both restored, `make check` green (408 tests, prettier clean), and an uncached `docker build --no-cache` shows the `RUN make check` layer executing.

clawbot added needs-review and removed needs-rework labels 2026-08-12 10:39:21 +02:00

clawbot commented 2026-08-12 10:43:50 +02:00

Author

Collaborator

Copy Link

Delta re-check of round 3 (#201 (comment)), head e32d689: FAIL — needs-rebase. Both round-3 findings are fixed and verified; the only defect is that next moved during the check and TODO.md now conflicts.

Finding — TODO.md conflicts with current next (bd4bdca).
At the start of this review origin/next was ce4a0d7 and e32d689 fast-forwarded cleanly. bd4bdca ("fix: explain a stored non-master xprv wallet..., closes #234", #234) then landed its own bullet at the top of # Completed Steps, colliding with this PR's bullet at TODO.md:47.

Reproduce:

git fetch origin && git rebase origin/next
# CONFLICT (content): Merge conflict in TODO.md

Acceptable: rebase onto current next, keep both bullets (this unit's at the top of # Completed Steps), re-run make check, force-push. No other file conflicts — README.md auto-merges and no source file is touched by bd4bdca.

Verified and passing (delta scope only):

• Guard pinning, by mutation on a private clone. Deleting if (typeof info.to !== "string") return false; (src/popup/views/txStatus.js:202) now fails restoreWait rejects a persisted wait missing its txInfo or broadcast time with TypeError: address.toLowerCase is not a function at src/popup/views/helpers.js:276. Removing Array.isArray(info) from txStatus.js:197 fails the same test with Expected: false / Received: true. Both restored, green again.
• No unreviewed code riding along: src/popup/views/txStatus.js, src/popup/index.js and src/popup/restorableViews.js are byte-identical to round-3 head d74dd4074362c589e0bf535351a38996beeb887a (blob 3a114da, ed4e955, 9d20495 respectively). Only tests/txStatus.test.js (the two added cases), TODO.md and rebase churn in README.md differ. The empty-string to deviation, MAX_CONSECUTIVE_LOOKUP_FAILURES = 6, the deadline path and "wait-tx" in restorableViews.js are therefore untouched, and the README still states the six-in-a-row bound.
• No TODO.md entry lost: the diff against next is a pure addition, zero deletions.
• make check on the unmodified head: 19 suites / 408 tests, all executed (6.65s, no cached results), prettier clean. Single commit; title ends (closes #155); author and committer both clawbot <clawbot@noreply.example.org>; no attribution trailers.

Disclosures: the new bullet is dated 2026-08-11 and sits above two 2026-08-12 entries, so # Completed Steps is no longer date-sorted. Not raised as a defect — the workflow rule in TODO.md is "move Next Step to the top", which this satisfies — but worth fixing while resolving the conflict above. script/cibuild was not re-run; the delta is two test cases and a doc bullet, and script/lint in this repo is prettier-only.

Delta re-check of round 3 (https://git.eeqj.de/sneak/AutistMask/pulls/201#issuecomment-57939), head `e32d689`: **FAIL — needs-rebase.** Both round-3 findings are fixed and verified; the only defect is that `next` moved during the check and `TODO.md` now conflicts. **Finding — `TODO.md` conflicts with current `next` (`bd4bdca`).** At the start of this review `origin/next` was `ce4a0d7` and `e32d689` fast-forwarded cleanly. `bd4bdca` ("fix: explain a stored non-master xprv wallet..., closes #234", https://git.eeqj.de/sneak/AutistMask/issues/234) then landed its own bullet at the top of `# Completed Steps`, colliding with this PR's bullet at `TODO.md:47`. Reproduce: ``` git fetch origin && git rebase origin/next # CONFLICT (content): Merge conflict in TODO.md ``` Acceptable: rebase onto current `next`, keep both bullets (this unit's at the top of `# Completed Steps`), re-run `make check`, force-push. No other file conflicts — `README.md` auto-merges and no source file is touched by `bd4bdca`. **Verified and passing** (delta scope only): - Guard pinning, by mutation on a private clone. Deleting `if (typeof info.to !== "string") return false;` (`src/popup/views/txStatus.js:202`) now fails `restoreWait rejects a persisted wait missing its txInfo or broadcast time` with `TypeError: address.toLowerCase is not a function` at `src/popup/views/helpers.js:276`. Removing `Array.isArray(info)` from `txStatus.js:197` fails the same test with `Expected: false / Received: true`. Both restored, green again. - No unreviewed code riding along: `src/popup/views/txStatus.js`, `src/popup/index.js` and `src/popup/restorableViews.js` are byte-identical to round-3 head `d74dd4074362c589e0bf535351a38996beeb887a` (blob `3a114da`, `ed4e955`, `9d20495` respectively). Only `tests/txStatus.test.js` (the two added cases), `TODO.md` and rebase churn in `README.md` differ. The empty-string `to` deviation, `MAX_CONSECUTIVE_LOOKUP_FAILURES = 6`, the deadline path and `"wait-tx"` in `restorableViews.js` are therefore untouched, and the README still states the six-in-a-row bound. - No `TODO.md` entry lost: the diff against `next` is a pure addition, zero deletions. - `make check` on the unmodified head: 19 suites / 408 tests, all executed (6.65s, no cached results), prettier clean. Single commit; title ends ` (closes #155)`; author and committer both `clawbot <clawbot@noreply.example.org>`; no attribution trailers. Disclosures: the new bullet is dated `2026-08-11` and sits above two `2026-08-12` entries, so `# Completed Steps` is no longer date-sorted. Not raised as a defect — the workflow rule in `TODO.md` is "move Next Step to the top", which this satisfies — but worth fixing while resolving the conflict above. `script/cibuild` was not re-run; the delta is two test cases and a doc bullet, and `script/lint` in this repo is prettier-only.

clawbot added needs-rebase and removed needs-review labels 2026-08-12 10:44:27 +02:00

clawbot referenced this pull request 2026-08-12 10:44:46 +02:00

decision: the per-unit TODO.md entry makes every merge invalidate every open PR #222

clawbot force-pushed fix/issue-155-waittx-timeout from e32d6896f2 to 2a364e60ba 2026-08-12 10:58:07 +02:00 Compare

clawbot merged commit afe6ddaea0 into next 2026-08-12 10:58:36 +02:00

clawbot deleted branch fix/issue-155-waittx-timeout 2026-08-12 10:58:36 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-rebase

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#201