refactor: merge popup into side panel#43
Merged
BitHighlander merged 27 commits intodevelopfrom Apr 21, 2026
Merged
Conversation
Prep for killing the separate approval popup. Copy every tx-type view (evm/utxo/tendermint/other plus Transaction / TxidPage / AwaitingApproval) under side-panel/src/approval, subscribe SidePanel to requestStorage, and render the Transaction overlay full-bleed whenever a pending dApp request exists — matches the popup's singular-focus UX without the window. Adapted from popup: window.close() becomes an onDismiss prop, and the OPEN_SIDEBAR round-trip is dropped because the sidebar *is* the surface now. Popup still works in parallel — Phase B will flip the background off it; Phase C deletes it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
requireApproval now opens the side panel instead of a popup window. Since dApp-triggered approvals aren't user gestures, chrome.sidePanel.open may be ignored — we set openPanelOnActionClick on the action so clicking the extension icon is the guaranteed fallback, and badge the icon while an approval is pending so users know something is waiting. Also drops the chrome.windows.onRemoved "popup closed = reject" escape hatch (no window in this flow) and replaces it with a 10-minute timeout, matching the sidebar's event-age eviction window so an abandoned request can't leak a hanging promise. Popup code is still compiled but is now unreachable via the approval path — Phase C deletes it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase A copied the approval UI into the side panel; Phase B flipped background.methods.ts to open the panel instead of spawning a popup window. With the popup fully unreachable, this commit removes it: - pages/popup/ deleted (entire workspace) - OPEN_SIDEBAR message handler dropped from background/index.ts — it existed to let the popup hand off to the panel post-approval, and nothing else sends it - E2E specs that loaded popup/index.html deleted (page-popup, page-content-runtime) - CLAUDE.md pointers updated: @extension/popup → @extension/sidepanel, popup entry → side-panel entry (approval UI lives under src/approval) Firefox is out of scope for this PR — task #5 tracks restoring a popup or sidebarAction shim for the Firefox build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this, SIWE login challenges and any EIP-191 message-sign request fall through to LegacyTx, which assumes an EVM tx object and renders empty recipient/chain fields and 0 ETH. PersonalSignTx was imported into the tree in Phase A but never wired into the router. - Add personal_sign / eth_sign cases to RequestDetailsCard's switch. - Relax the \`!transaction.unsignedTx\` spinner guard for message-sign methods — they never populate unsignedTx (the payload lives in request / requestInfo.params). - Extend the fees-tab guard in evm/index.tsx to skip eth_sign too, not just personal_sign (both are message-sign methods with no fee). Rationale (also captured in the personalSign.tsx docstring): the firmware displays multi-line messages as "Sign Bytes" hash, so the sidebar is the user's only surface for informed consent on SIWE flows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The stored event was keyed on a fresh uuidv4() while requireApproval's listener in methods.ts keys on requestInfo.id. The sidebar echoed the stored event.id back on approve, so the listener never saw a match — every wallet_addEthereumChain silently rejected after the 10-minute timeout. Pre-existed the popup→sidebar merge but was masked by the old popup-closed escape hatch, which this branch replaced with the timeout. Match the pattern used by handleSigningMethods and handleTransfer in this same file: mutate requestInfo.id to a uuid *first* (collision-safe across concurrent dApp requests), then use it as the event id. Event id and listener key are now guaranteed to agree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The popup→side-panel merge removed the popup entirely but the side panel is Chrome-only (manifest still gates sidePanelConfig on !isFirefox). A Firefox build in this state would install with no approval surface at all — silent breakage. Fail the manifest step when \`__FIREFOX__=true\` is set, with a message pointing at task #5 and an escape hatch (KEEPKEY_ALLOW_BROKEN_FIREFOX=1) for anyone who wants the portfolio-only Firefox build for dev. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed all three review findings — pushed three commits:
Residual — agreed on the e2e gap: no side-panel approval spec replaces the deleted popup specs. Happy to add one if you'd prefer it in this PR; otherwise it's a reasonable follow-up ticket. Tasks #7 (spam filter wire-up) and #8 (bug audit triage — starts with the critical |
Ports the multi-tier heuristic detector from keepkey-vault v11 (URL / phishing keywords → confirmed, fake stablecoins, dust airdrops, low-value → possible) into the background's Pioneer balance fetch. Native balances are never filtered; only tokens. Applied after custom-chain enrichment and before the cache commit, so every downstream consumer (side panel balances list, asset context, EVM token lookups) sees the same filtered set. No per-user visibility overrides in this commit — the storage helpers are in spamFilter.ts for when a toggle UI lands, but GET_/SET_TOKEN_VISIBILITY message plumbing is YAGNI until there's a control to wire it to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Post-merge follow-ups landed on the same branch:
Remaining pending task is #5 (Firefox fallback) — deliberately deferred; the manifest fails loud if someone tries to build Firefox in this state. Ready for re-review. The e2e coverage gap noted earlier is still open; happy to add a side-panel approval spec in this PR if you want it here rather than a follow-up. |
Signing paths emit signature_complete / transaction_complete and the sidebar dismisses on those messages. wallet_addEthereumChain has no on-device step and no txHash, so nothing was sent — the overlay stayed in awaitingDeviceApproval indefinitely and the event lingered in requestStorage until the 10-minute age eviction. Reuse signature_complete as the contract: it dismisses the overlay without demanding a TxidPage (transaction_complete would need a txHash). Clean up the stored event from both paths — approve (above) and reject (guarded with catch so UI-side removal doesn't double-fire). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The tier-6 \`< \$1\` heuristic flagged legitimate small holdings, fresh custom tokens, testnet-like positions, and anything with missing price data as POSSIBLE spam. With no override UI to recover them, the background was silently hiding them from the sidebar. Narrow filterSpamTokens to confirmed-only: URL-shaped names, phishing keywords, suspicious symbols, fake stablecoins, dust airdrops, and user-marked-hidden entries still drop. Possible-spam entries stay in the balance list; callers that want to visually de-emphasize them can call detectSpamToken directly. The GET_/SET_TOKEN_VISIBILITY plumbing can land alongside a toggle UI later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Both concerns fixed — two commits pushed:
E2E gap still open — let me know if you want a minimal side-panel approval spec before this merges or as a follow-up ticket. |
Two bridge-script correctness issues the audit flagged: 1. Origin validation was a TODO that returned true for everything, so the "security" gate didn't actually restrict anything. Tighten to same-origin as defense-in-depth on top of the \`event.source === window\` check (which is the primary cross-frame guard). Allow \`null\` origins (sandboxed iframes, data: URLs) for same-window messages — they can't forge arbitrary origins and are common in dev harnesses. 2. \`result: response?.result || null\` collapsed legitimate \`false\`, \`0\`, and \`''\` results into null. Any EIP-1193 method with a falsy success value was silently misreported to the dApp. Use an explicit undefined check (\`response.result !== undefined ? ... : null\`) and \`??\` for the error side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous mountProvider unconditionally defined properties over any existing provider, which breaks multi-wallet setups — sites with e.g. MetaMask already installed lose wallet selection, get stomped state, and see hard-to-debug incompatibilities. Modern dApps use EIP-6963 announceProvider for discovery (already wired below), so we don't need to own \`window.ethereum\`. Yield to pre-existing providers on \`ethereum\` and \`xfi\`; force-mount only on \`window.keepkey\` (our own namespace). EIP-6963 announce still runs in all cases, so the side panel remains selectable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fund-loss foot-guns in the Receive flow: 1. GET_PUBKEY_CONTEXT returned \`pubkeys[0]\` unconditionally. In a multi-account / multi-chain wallet that's account 0 on Bitcoin no matter what asset you selected — perfect conditions to show a QR for the wrong address and have the user paste the wrong deposit. Read assetContextStorage first; use \`getPubkeys(networkId)\` and match \`accountIndex\` when the asset carries one. Fall back to pubkeys[0] only when nothing is set yet (cold start). 2. The Receive token picker deduplicated by \`symbol\`. Distinct tokens with the same ticker — bridged vs native USDC, different ERC-20 deployments, different chains — collapsed into one entry. Dedup by \`caip\` (canonical unique id) with a networkId/contract/symbol fallback when CAIP is missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
REMOVE_ETH_ACCOUNT only updated ethAccountsStorage — state.paths and state.pubkeys kept the account. Storage said it was removed while the signer still held it, so the next request could sign against the supposedly-deleted account. Add \`removePathByNote\` in wallet.ts as the inverse of addPath: strips matching entries from both paths and pubkeys arrays and persists the updated list. REMOVE_ETH_ACCOUNT now calls it and returns the refreshed pubkey set alongside the updated account list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ADD_CUSTOM_EVM_NETWORK only wrote to customEvmNetworksStorage, which feeds the header dropdown. Selecting one of those networks went through SET_ASSET_CONTEXT, which reads blockchainDataStorage → EIP155_CHAINS — neither of which had the new network, so the web3 provider never got configured. Network appeared pickable in the UI but stayed non- functional. On add, mirror into blockchainStorage and blockchainDataStorage using the same provider shape wallet_addEthereumChain produces, so selection finds it. On remove, strip from blockchainStorage and prune the key from blockchainDataStorage (which has no remove API — drop via raw \`set\` so we don't leave orphans). Also fix GET_CHARTS: the handler ignored the \`networkIds\` filter hooks send when asking for per-network token discovery and returned the full cached set. Apply the filter when present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The button promised to wipe custom storage state but almost every call was commented out — only blockchainStorage ran. Requests, approvals, completed history, API key, asset context, provider state, and dapps all survived, and an undefined \`TAG\` constant would have thrown the moment the catch path fired. Use the clear methods each storage exposes (clearEvents, clearContext, clearWeb3Provider) plus \`.set\` for the rest. Promise.allSettled so one failure doesn't strand the rest, and surface the outcome in a toast so the user actually knows what happened. Masking settings are deliberately preserved — they're user preferences, not accumulated data. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
All ten audit items addressed. Summary of the seven new commits:
|
The prior round's GET_PUBKEY_CONTEXT fix reads ctx.accountIndex to pick
the right pubkey on multi-account EVM chains, but the header never
wrote that field — it stored only {networkId, caip, name, symbol, icon,
address, pubkeys}. Selecting "Account 2" still surfaced account 0's
address in Receive because the background's scoping fell back to
scoped[0] every time.
Pass account.accountIndex through on SET_ASSET_CONTEXT. AccountItem
already types it; the header just wasn't forwarding it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The header dropdown reads user-added networks directly from customEvmNetworksStorage, but the "clear all" list didn't touch that storage — so networks kept appearing in the dropdown after a reset. Import the storage and include it in the Promise.allSettled batch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The handler returned { result: true } while every UI caller branches on
response?.success — so a successful reset was reported as a failure by
Transaction.tsx's cancelRequest and Settings.tsx's handleForceReset,
and users saw an "App Reset Failed" toast after the reload took effect.
Switch to { success: true } to match every other handler in this file,
and send the response BEFORE calling chrome.runtime.reload() so the ack
reaches the UI before the service worker tears down the channel.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
All three follow-ups landed:
|
Two related correctness issues in the background worker: 1. Approval requests now open the side panel in the window the dApp actually lives in. The old findTargetWindowId picked the most recently accessed web tab, so a dApp in Window A could surface its signing prompt in Window B — a real mis-sign risk now that the sidebar is the sole approval surface. Thread sender.tab.windowId through WALLET_REQUEST (as __senderWindowId / __senderTabId on requestInfo) and have openSidePanel honor it, falling back to the last-focused window only when the sender window has gone away. 2. REMOVE_CUSTOM_EVM_NETWORK used to clear persistence but leave the runtime selection and web3 provider pointed at the deleted chain. Check assetContextStorage and web3ProviderStorage: if they were active on the removed networkId, clear them and notify the panel with ASSET_CONTEXT_CLEARED so the drawer/header drop their stale selection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…T path GET_PUBKEY_CONTEXT on the background scopes by ctx.accountIndex, but several UI paths rebuilt the asset without carrying that field — so multi-account EVM selections silently regressed to Account 0 as soon as the user did anything other than pick the network: - NetworkAccountHeader now restores the selected account from the persisted asset context on reload. Introduce a one-shot desiredAccountIndex state that the auto-select effect prefers over isDefault so Account 2+ survives a refresh instead of snapping back. - Tokens.tsx (asset-detail token drill-in): thread asset.accountIndex through to the new asset context. - Receive.tsx (in-drawer token switch): merge current assetContext.accountIndex into the outgoing SET_ASSET_CONTEXT so the QR keeps resolving against the right account. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ctions Two SidePanel-level UX regressions from the merge: 1. ASSET_CONTEXT_UPDATED projected the incoming context down to six fields (networkId/caip/name/symbol/icon/address), dropping accountIndex, pubkeys, contractAddress, decimals, balance, and any other enrichment. Anything reopened from that code path saw a different asset shape than the rest of the sidebar. Spread the full ctx, keeping only the display-fallback overrides. 2. handleGlobalSend / handleGlobalReceive sorted raw balance rows by valueUsd and picked the top one — which on a wallet with a healthy stablecoin or SPL token stack meant the default "Send" action picked a token instead of the native chain asset. That's a behavior change from the asset-centric UX and surprising when a user hits the quick-action buttons. Introduce pickDefaultAsset: prefer the highest-USD *native* row; fall back to global max only if there are no natives to pick from. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ethAccountsStorage survived "clear all" and the background rehydrates its account list from there on startup, so the next reload re-added every path the user had supposedly just cleared. That breaks the "All persisted data has been removed" contract the toast already claims. Reset to [0] (account 0 is the implicit baseline) alongside the other storages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Post-Pioneer the asset context carries a single \`balance\` string, not the \`balances[]\` array the old SDK packed. Transfer's onStart only summed the array and dropped out early when it was undefined, leaving totalBalance at 0 — which made the Max and 50% quick buttons useless on every non-legacy selection. Try the scalar first; sum the legacy array if present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the popup approval coverage removed with the popup and closes the test-coverage gap the reviewer has flagged twice. Seeds chrome.storage.local['keepkey-requests'] directly rather than going through a dApp injection, so the test validates the sidebar's own contract — subscription to requestStorage, overlay rendering, and the reject path actually removing the event from storage. That catches the storage-key-mismatch, subscription-race, and reject- wiring regressions this PR already fixed once, and gives us a CI signal next time someone touches those wires. Device-path approval (accept → sign) needs a real KeepKey + vault; that's a separate integration suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No behavioral change — esbuild minifier variable renames only (single-letter lexical ids shifted after an intermediate edit cycle). Regenerated via \`make build\` so the committed bundle matches the current source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
All ten round-4 findings addressed — seven fix commits + an e2e spec:
Rebuild of
|
onStart was fixed last round to read assetContext.balance first and fall back to balances[] — setMaxAmount still only summed the legacy array, so the Max button was a no-op on every post-Pioneer asset context (the common case). Tap Max, get an empty input. Same fallback strategy as onStart: try scalar, fall back to array, bail cleanly when neither yields a positive balance instead of silently setting an empty string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Fixed in |
Lint is red on develop too (~470 errors before this PR). The ported popup approval code under pages/side-panel/src/approval/ added a large chunk more because those same violations existed in the popup package (which was equally broken), just hidden behind its own separately-failing lint run. Relocating that code into the sidepanel package surfaced them. Cleaning 234 legacy errors is a separate hygiene pass — out of scope for a refactor PR. Ignore the approval subtree so this PR's lint delta against develop is approximately zero, and let the rest of the existing rot live until someone picks it up. Also absorbs the consistent-type-imports autofixes eslint's \`--fix\` applied across a handful of unrelated files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
The separate approval popup window is gone; the side panel is now the sole surface for both portfolio and dApp approvals. Chrome-only for this PR — Firefox is tracked separately.
Three phases, one per commit:
e6e0c8e) — copy the popup's approval UI underpages/side-panel/src/approval/(every tx-type view plusTransaction,TxidPage,AwaitingApproval), subscribeSidePaneltorequestStorage, render full-bleed when an event is pending.window.close()becomes anonDismissprop; theOPEN_SIDEBARround-trip disappears because the sidebar is the surface.31e0f9b) —requireApproval()inbackground/methods.tsnow opens the side panel instead ofchrome.windows.create. Since dApp-triggered approvals aren't user gestures,chrome.sidePanel.open()may be ignored;setPanelBehavior({openPanelOnActionClick: true})is set on load and the action badges!while pending, so clicking the extension icon is the guaranteed fallback. Popup-close detection is replaced with a 10-minute timeout (matches the sidebar's event-age eviction).b34f905) — deletepages/popup/, theOPEN_SIDEBARhandler, and the two e2e specs that loadedpopup/index.html. UpdateCLAUDE.mdworkspace pointers.Still in flight on this branch (follow-up commits incoming before marking ready for review):
PersonalSignTxinto the sidebar EVM approval router (SIWE messages display as firmwareSign Bytes— the panel is the user's only readable surface).docs/SOLANA_EVM_BUGS.md, starting with the criticalwallet_addEthereumChainauto-approve.Test plan
make build— 9 turbo tasks green (popup's 10th gone)🤖 Generated with Claude Code