fix: a dropped dispatch or a failed window open is no longer a silent no-op - #1464
Open
ost-ptk wants to merge 12 commits into
Open
fix: a dropped dispatch or a failed window open is no longer a silent no-op#1464ost-ptk wants to merge 12 commits into
ost-ptk wants to merge 12 commits into
Conversation
…sees A `sagaError` cannot report a transport failure: it lives in the background store, and the background store is precisely what is unreachable when `runtime.sendMessage` rejects. The replica cannot hold one either — `createMainStoreReplica(state)` runs in the render body of every app's `Tree`, so a local dispatch is discarded on the next broadcast. Hence a module store, read directly by the banner. It carries a `kind` rather than a message because its callers are plain functions with no access to `t()`; the component owns the copy. Reports dedupe by `kind:detail`, since a restarting service worker plus three impatient clicks is the normal shape of this failure and the popup is too narrow for three identical rows. `getUiErrorsSnapshot` returns a reference that changes only when the list does (`useSyncExternalStore` otherwise re-renders on every check), and `getUiErrorsServerSnapshot` exists because this repo renders components through `renderToStaticMarkup`, where the hook throws without it.
Logging every dropped dispatch is right; showing a banner for every one is not. `dispatchToMainStore` has ~105 call sites, and a banner on a dropped theme change or a dropped banner-dismissal would train the user to ignore the banner. So membership has one criterion, written down next to the list: the dispatch IS the sole source of the visible result — if it never lands, the user sees nothing happen and no other feedback path exists. Five actions qualify: the export-keys window (the case from the ticket), `lockVault` (the wallet stays unlocked, which is a security outcome), the Ledger window attach (the request hangs until the dapp times out), and `resetVault` / `accountsImported`, whose `.then` callbacks would otherwise present unperformed work as done. Built from the creators' `.type`, so a rename is a compile error, and pinned by a test so growing or shrinking the list is a deliberate edit rather than a side effect. Adds no import cycle: `redux/utils.ts` already imports `redux-action.ts`, which pulls every slice's actions.
`dispatchToMainStore` logged the failure and returned a promise that resolved either way, so a caller could not tell a dispatch that ran and failed from one that never arrived — and the user was told nothing in either case. It now resolves `true` / `false` and, for an allow-listed action, reports to the UI-local error channel. It still never rejects: ~105 call sites do not catch, so rejecting would produce an unhandled rejection at each of them. The log stays universal — type and cause, never the action, whose payload carries vault and signature material. The `Promise<boolean>` widening is source-compatible with every fire-and-forget caller; `tsc` over the whole tree confirms nothing else depended on the old type.
…dropped dispatch Three call sites chained `.then` onto `dispatchToMainStore` and ran their success path unconditionally, so a dispatch that never reached the background still navigated or reloaded as if the work had happened. The reset one was the worst of the three: `closeWindowByReloadExtension()` reloaded the extension, presenting an unperformed vault reset as complete and taking the error banner down with the page. Onboarding advanced to the create-password step on a vault that was never reset; the Ledger flow navigated Home with no accounts imported. All three now branch on the verdict. Each of the two actions involved is allow-listed, so the user gets the banner instead of a false success.
Both `openWindow` call sites ended in the same `.catch(e => console.error(e))`: the window failed to open, the record went somewhere no end user can read, and the UI showed nothing at all. The reporting moves into `useWindowManager` rather than being repeated at the two sites, so the returned `openWindow` never rejects and a third call site added later cannot reintroduce the swallow. No allow-list is involved here — a window that did not open is always visible to the user by its absence.
One surface, two inputs: background `sagaError`s keep arriving through the replica
store, and the UI-local channel is read through `useSyncExternalStore`. The copy
for the local rows lives here, where `t()` does, because the channel carries a
`kind` — unlike `SagaError.message`, which is produced in the background and
rendered verbatim and untranslated. The action type and the window name stay in
the console: they name no dapp and suggest no next step.
Two imports changed to reach leaves instead of barrels: the component now takes
its layout primitives from `@libs/layout/containers` and its `SvgIcon` /
`Typography` directly, and `typography.tsx` takes `Skeleton` from its sibling
rather than from `@libs/ui/components` — a barrel importing itself back, which is
what `containers.ts` warns about in its own header comment ("cause huge problems
with webpack bundle and lead to blank popups").
That was also what made the component untestable: the barrels transitively load
`webextension-polyfill`, which throws on import outside an extension, and
`mac-scrollbar`, which ships only an ESM entry that jest cannot resolve — the
reason no barrel-importing component in this repo has ever had a test. With the
chain broken, the suite renders the banner for each channel and for both at once.
Verified against a production `build:chrome`.
… success Fixing the three chained `.then` sites left the same failure live on the paths that dispatch and continue without chaining anything — including the two most used import flows, where only the Ledger one had been covered. - `accountImported` was missing from the surfaced list entirely (only its plural sibling was there), and both of its call sites showed success unconditionally: the secret-key-file flow navigated to its success screen and the Torus flow set its success step for an import that never reached the vault. - `resetVault` has five call sites, and two more needed guarding. The error page ran `dispatch; closeCurrentWindow(); openOnboardingUi();` — tearing the window down destroys the banner it just queued and drops the user into onboarding believing the vault was reset. Onboarding's secret-phrase back button navigated to create-password on a vault that was never wiped, so the generated phrase survived and the user would set a password over a phrase they never recorded. - `reset-wallet` needs no guard: it dispatches and navigates nowhere, so the banner is the whole fix there. Also drops the `try/catch` around the error-page teardown. It caught nothing — all three calls inside it were unawaited, so nothing could throw synchronously. Adds `dismissSagaError`: without it, pressing × on a background row while the transport is down does nothing and explains nothing. With it the reason appears as one deduped row, whose own × is local and always works.
…tracted One string serves five unrelated actions, and the old wording asserted two things the boolean cannot guarantee. "Nothing was changed" is provably false in at least one path: `handleReduxAction` dispatches `resetVault` into the store and only then awaits `enableOnboardingFlow`, so a rejection can arrive with the vault already wiped. "Please try again" is false for the Ledger attach: `use-ledger` sets `triggeredRef.current` immediately after its dispatch, so that effect cannot re-run, and the permission window is open either way. The copy now says only what is true for all of them — the wallet did not respond, so the last action may not have been applied. It also moves from a `t()` lookup keyed on `kind` to a literal `<Trans>` per kind: a dynamic key is invisible to i18next-parser, so those strings would never have reached any catalog, which contradicted the very comment distinguishing them from the untranslated saga messages. Drops two mocks and the comment justifying them from the suite: the same commit that added them switched the component to direct leaf imports, so the barrels are no longer pulled. Re-ran the suite without them — all five still pass.
`windows.create` is given a URL built from `OpenWindowProps.searchParams`, so logging the raw rejection can put those params in the console. A sign-message plaintext rides in that query string on other window paths, and the semgrep `cw-logging-secrets` pattern would not catch it. No current UI caller passes `searchParams`, so this is latent — but the hook's public type invites one. Logs `error.name` instead, matching what `use-ledger.ts` already does for the same reason.
The unit tests sat on either side of the seam — one asserts `reportUiError` is called, the other that such a call renders a row — but nothing drove a real dropped dispatch in a real popup. This does: it breaks the page's transport, clicks the menu item, and asserts the banner. Four things had to be learned the hard way and are recorded in the file: - The break must be a *rejected promise*, not a synchronous throw. Current Chrome exposes `browser` natively, so `webextension-polyfill` re-exports it rather than wrapping `chrome`, and the app calls the patched function directly. A `throw` escapes synchronously out of `dispatchToMainStore` and trips the ErrorBoundary — the whole popup renders "Something went wrong" and no banner is ever involved. - Stopping the service worker does not reproduce this at all: in MV3 an incoming message is the event that cold-starts a stopped worker, and the keep-alive alarm wakes it within 30s while the vault is unlocked. - `unlockVault` returns before the unlock has finished (it only waits for network idle, and the flow completes through further dispatches from the page), so the transport must not be broken until the unlocked UI is up. - Dedupe is deliberately left to the unit test: the banner is `position: fixed; top: 0` at a tooltip z-index, so a visible row covers the header — measured 1280x77 over a menu icon at y=24..48, with `elementFromPoint` returning the banner — and a second menu click cannot land until it is dismissed. Covers: banner on a dropped export-keys dispatch (and that no window opens), no banner for a non-surfaced action, dismiss-then-reappear, and the failed window open.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes WALLET-1390.
Completes WALLET-1364 P1 #2/#3. That work made the in-saga failure loud; two adjacent halves of the same journey stayed silent, so "the user clicks and nothing happens" was still reachable after it shipped.
Why this needed a new surface rather than a
sagaErrorA
sagaErrorcannot report a transport failure: it lives in the background store, and the background store is precisely what is unreachable whenruntime.sendMessagerejects — the dispatch that would report the error is the dispatch that failed.The replica cannot hold one either.
createMainStoreReplica(state)runs in the render body of every app'sTree(popup/index.tsx:50and four siblings), so a new store is built on every broadcast and any locally dispatched action is discarded on the next one.So the errors live in a small module store,
ui-error-channel.ts, which the already-mountedSagaErrorBannerreads throughuseSyncExternalStore. One surface, two inputs — not a second banner. It carries akindrather than a message, because its callers are plain functions with no access tot(); the component owns the copy.Transport half
dispatchToMainStorenow resolvestrue/falseand still never rejects — ~105 call sites do not catch, so rejecting would produce an unhandled rejection at each of them. The log stays universal (type + cause, never the action); the banner is filtered by an allow-list.SURFACED_DISPATCH_ACTIONShas one written-down criterion: the dispatch IS the sole source of the visible result. Seven actions qualify — the export-keys window (the case from the ticket),lockVault(the wallet stays unlocked, a security outcome rather than an inconvenience), the Ledger window attach,resetVault,accountsImported/accountImported, anddismissSagaError(without it, pressing × on a background row while the transport is down does nothing and explains nothing). A banner on all ~105 sites — theme changes and the like — would train the user to ignore the banner.The list is built from the creators'
.type(a rename is a compile error) and pinned by a test, so growing or shrinking it is a deliberate edit. It adds no import cycle —redux/utils.tsalready importsredux-action.ts, which pulls every slice's actions — verified withdependency:circular(140 before, 140 after) andimport/no-cycle.Nine call sites that reported failure as success
Chaining
.thenwas only half of it; the paths that dispatch and continue without chaining had the same bug.reset-vaultcloseWindowByReloadExtension()reloaded the extension, presenting an unperformed vault reset as complete and taking the banner down with the pageonboarding/welcomeconnected-ledgerimport-account-with-file-uploadimport-account-from-torusonboarding/create-secret-phraselayout/error/window-pageAll now branch on the verdict.
onboarding/reset-walletneeds no guard: it dispatches and navigates nowhere, so the banner is the whole fix there.The error page also loses its
try/catch: all three calls inside it were unawaited, so it never caught anything.UI half
The two identical
.catch(e => console.error(e))handlers aroundopenWindoware gone. Reporting moved intouseWindowManager, so the returnedopenWindownever rejects and a third call site cannot reintroduce the swallow. It logserror.name, not the rejection: the URLwindows.createis given embedsOpenWindowProps.searchParams, where a sign-message plaintext can ride on other window paths, and semgrep'scw-logging-secretspattern would not catch that.The copy says only what is true
One string serves five unrelated actions, so it claims neither that nothing changed nor that retrying helps. Both would be false somewhere:
handleReduxActiondispatchesresetVaultinto the store and only then awaitsenableOnboardingFlow, so a rejection can arrive with the vault already wiped; anduse-ledgersetstriggeredRef.currentimmediately after its dispatch, so that effect cannot re-run. It reads: The wallet didn't respond. Your last action may not have been applied.Each kind renders a literal
<Trans>rather than at()lookup keyed onkind— a dynamic key is invisible to i18next-parser, so the strings would never reach a catalog. No extraction run here (lang/is far behind); they ship with their English fallback.Two import changes, and why they are here
SagaErrorBannertakes its layout primitives from@libs/layout/containersand itsSvgIcon/Typographydirectly, andtypography.tsxtakesSkeletonfrom its sibling instead of from the@libs/ui/componentsbarrel — a barrel importing itself back, which is whatcontainers.tswarns about in its own header comment ("cause huge problems with webpack bundle and lead to blank popups").That was also what made the component untestable: the barrels transitively load
webextension-polyfill, which throws on import outside an extension, andmac-scrollbar, which ships only an ESM entry with nomainthat jest cannot resolve. It is why no barrel-importing component in this repo has had a test. Breaking the chain was cheaper and more useful than three mocks plus ajest.config.jschange — and since it touches a component used app-wide, it is verified against a productionbuild:chrome.Tests
ui-error-channel— dedupe by key, snapshot-reference stability (a fresh array each call would re-render forever), no allocation on a deduped repeat, subscribe/unsubscribe, re-report after dismiss, server snapshot.SURFACED_DISPATCH_ACTIONS— pin, checked by mutation: dropping an entry fails the test.dispatchToMainStore—trueon success,falseinstead of a rejection, log shape, allow-listed action surfaces, non-allow-listed action logs only.SagaErrorBanner— rendered withrenderToStaticMarkupfor each channel separately and both at once, asserting the action type never reaches the markup.ci-checkgreen: 87 suites, 763 tests, knip clean.The dispatch→banner seam is covered by e2e
e2e-tests/popup/error-surface/dropped-dispatch.spec.tsdrives the real extension: it breaks the page's transport to the background, clicks the menu item, and asserts the banner. Four cases — banner on a dropped export-keys dispatch (and that no window opens), no banner for a non-surfaced action, dismiss-then-reappear, and the failed window open. 4/4 green in ~13s.Three things that had to be established by running it, and are recorded in the file:
browsernatively, sowebextension-polyfillre-exports it instead of wrappingchrome, and the app calls the patched function directly. Athrowescapes synchronously out ofdispatchToMainStoreand trips the ErrorBoundary — the popup renders "Something went wrong" and no banner is involved at all.unlockVaultreturns before the unlock finishes (it waits only for network idle; the flow completes through further dispatches from the page), so the transport must not be broken until the unlocked UI is up.One finding worth a decision, not fixed here
While a banner row is visible it covers the header, including the menu button. Measured: the banner is
position: fixed; top: 0at a tooltip z-index, occupying 1280×77 over a menu icon at y=24..48, andelementFromPointat the icon's centre returns the banner. So the user cannot reopen the menu to retry until they dismiss the row — the × is right there, so it is one extra click rather than a dead end.This is pre-existing
SagaErrorBannerlayout, not something this PR introduces, but this PR is what makes the banner routine rather than rare. Fixing it means either pushing page content down by the banner height or moving the banner out of the header's strip — a visual change worth a deliberate call rather than a drive-by. It is also why the dedupe case lives in the unit test instead of e2e: a second menu click cannot land while the row is up.Deliberately left out
loginRetryLockoutTimeSet(use-lock-wallet-when-no-more-retries.ts:26) is the dispatch that actually enforces the brute-force lockout, and it is not surfaced. A banner would tell the truth and change nothing: the effect's deps cannot change, so there is no retry path, and the real fix is not letting the lockout depend on a droppable dispatch. Worth its own ticket.redux/utils.tsnow imports from@libs/ui/..., so the channel would ride into the service-worker bundle. Not reachable today — no background module callsdispatchToMainStore— but it inverts the background→UI direction. Fixing it properly means injecting the reporter, which is more machinery than the problem currently justifies.app-eventsslice orSagaErrorSource: local errors deliberately do not survive a reload.