fix(sidebar): refresh popup allowList after grantAccess succeeds#2850
fix(sidebar): refresh popup allowList after grantAccess succeeds#2850piyalbasu wants to merge 4 commits into
Conversation
In sidebar mode, `openSigningWindow` reuses the existing sidebar window across consecutive signing flows (e.g. `signMessage` → `requestAccess` → `signMessage` continuation) via `SIDEBAR_NAVIGATE` over the long-lived port. The popup React tree and its redux store are preserved, so `useIsDomainListedAllowed` keeps reading the pre-grant `state.settings.allowList` even though the backend just wrote the new domain to `localStore`. Result: after granting access for the first time from a sidebar, SignMessage / SignAuthEntry views render "domain not in allowlist" even though the dApp is now connected. Have the `grantAccess` thunk refresh settings after the backend write so popup-side redux picks up the new allowList entry. Popup windows opened via `browser.windows.create` already mount fresh redux and refetch — this just makes the sidebar surface behave the same way. Best-effort: a failed `loadSettings` doesn't reject the grant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-7649f3df12d039d32d1a (SDF collaborators only — install instructions in the release description) |
There was a problem hiding this comment.
Pull request overview
Fixes a sidebar-only UX bug where consecutive signing flows reuse the same popup React tree (and Redux store), leaving state.settings.allowList stale after a first-time grantAccess—which incorrectly shows “domain not in allowlist” on the follow-up signMessage / signAuthEntry continuation.
Changes:
- Update the
grantAccessthunk to refresh popup-side settings (vialoadSettings→saveSettingsAction) after a successful grant, with best-effort error swallowing. - Add Jest coverage to verify (1) refresh occurs after grant and updates
allowList, and (2) refresh failures don’t cause the thunk to reject.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
extension/src/popup/ducks/access.ts |
Refreshes popup-side Redux settings after grantAccess to prevent sidebar allowList staleness. |
extension/src/popup/ducks/__tests__/access.test.ts |
Adds regression tests for the post-grant refresh and best-effort error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cce2f34c13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The best-effort settings refresh in grantAccess logged failures with console.error, which never reaches Sentry. Use captureException so these transient failures are observable, passing the error object to preserve the stack trace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Awaiting the settings reload before the grantAccess thunk resolved delayed GrantAccess's window.close()/route-close. In sidebar mode the reused popup tree then sat on /grant-access long enough for the dApp's follow-up signing request to be treated as interrupting an active signing route (interstitial) and clobbered by the deferred close. Run the refresh fire-and-forget instead: the thunk resolves right after the grant, and redux still updates when the reload lands, re-rendering the next view with the fresh allowList. Sentry reporting of reload failures is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TL;DR
In sidebar mode, granting access to a dApp for the first time leaves the SignMessage / SignAuthEntry follow-up showing "domain not in allowlist" even though the backend just wrote the new entry. The bug is unrelated to auto-lock (uncovered while manually testing #2802 but reproduces independently) — the sidebar reuses the same React tree across consecutive signing flows, so the popup-side
allowListstays stale.This PR makes the
grantAccessthunk refresh popup-side settings after the backend write succeeds. Sidebar SignMessage / SignAuthEntry continuations now see "connected" as intended. Popup-window flows already worked correctly because eachbrowser.windows.createcall mounts a fresh redux store; this just makes the sidebar surface behave the same way.Details for future readers / agents
The bug
@stellar/freighter-api'ssignMessageandsignAuthEntryboth followrequestAllowedStatus → requestAccess → submit*. Each of those external calls invokesopenSigningWindowinextension/src/background/messageListener/freighterApiMessageListener.ts:90-126, which has two code paths:browser.windows.create({...})opens a new popup window. The new window mounts a fresh redux store, runsuseGetAppData → loadSettings, picks up the just-granted allowList entry, anduseIsDomainListedAllowedreturns true. ✅SIDEBAR_NAVIGATEmessage over the existing port and routes the same sidebar window to the next hash route. Same window, same redux store. NoloadSettingsrefetch happens betweenrequestAccessandsubmitMessage/submitAuthEntry, sostate.settings.allowListstill holds the pre-grant snapshot.useIsDomainListedAllowed(extension/src/popup/helpers/useIsDomainListedAllowed.ts) returns false → "domain not in allowlist" banner + disabled Sign button. ❌Why it surfaced now
The auto-lock work in #2802 made it easy to hit by introducing a long pause between a
requestAccessconfirm and the next view. But the same bug reproduces on a fast, unlocked sidebar flow — it's just less likely because the user is typically already moving on. Confirmed by manual testing.signTransactionis unaffected: itsfreighter-apiflow goes straight tosubmitTransactionwithout arequestAccessprecheck, and the SignTransaction view's "not in allowlist" banner is the intentional review-before-connect UX. Out of scope here.The fix
extension/src/popup/ducks/access.tsgrantAccessthunk now:internalGrantAccess(unchanged).loadSettings()and dispatchessaveSettingsAction(fresh)to refresh popup-side redux.Other thunks that mutate backend state without refreshing popup-redux (
saveAllowList,rejectAccess,setAllowed) have similar shapes but don't manifest the same UX bug today:saveAllowList.fulfilledalready updatesstate.allowListfrom its return payload (seepopup/ducks/settings.ts:429-443).rejectAccessdoesn't modify the allowList — only clears the pending request.setAllowed(Manage Connected Apps) re-renders on mount.Out of scope for this PR. Worth a follow-up audit if more sidebar staleness bugs surface.
Tests
Two new tests in
extension/src/popup/ducks/__tests__/access.test.ts:state.settings.allowListreflects the just-granted domain.All related suites pass:
extension/src/popup/ducks/__tests__/(4 suites, all green)extension/src/popup/views/__tests__/GrantAccess.test.tsxextension/src/popup/components/__tests__/SessionLockListener.test.tsxManual verification
Sidebar mode, fresh install on testnet:
signMessagefrom a never-connected dApp (e.g.account-viewer-v2)Same flow for
signAuthEntryworks equivalently.Test plan
signMessageagainst unconnected dApp → confirm SignMessage view shows the dApp as connectedsignAuthEntryagainst unconnected dApp → samesignMessagecontinues to work (no regression on the path that already worked)signTransactionwarning banner behaviour unchanged for unconnected dApps🤖 Generated with Claude Code