Feature/sidebar mode audit

[piyalbasu]

This pull request introduces several improvements and bug fixes related to the extension's sidebar functionality, including more robust sidebar connection management, improved message routing, and better handling of signing request rejection. It also adds and updates tests to ensure correct behavior and compatibility across Chrome and Firefox.

Sidebar connection and lifecycle management:

• Added a new module (sidebarPort.ts) to manage the sidebar's long-lived port, allowing direct communication and safer handling of sidebar state transitions. [1] [2]
• Improved sidebar disconnect logic to only reject signing requests handled by the sidebar, preventing accidental cancellation of requests managed by popups.
• Enhanced logic to differentiate between sidebar reloads and true disconnects, deferring cleanup to avoid prematurely rejecting requests.
• Introduced sidebarQueueUuids to track which signing requests are handled by the sidebar. [1] [2]

Message types and API changes:

• Added new REJECT_SIGNING_REQUEST service type and message, and removed unused sidebar registration messages. [1] [2] [3]
• Added rejectSigningRequest API method for programmatically rejecting signing requests.

Sidebar user experience and manifest:

• Updated the manifest to add a sidebar_action entry for better Firefox support and improved sidebar icon handling.
• Clarified and improved logic for the "open sidebar by default" feature, including Chrome/Firefox compatibility notes.

Message routing and navigation:

• Changed sidebar navigation to use direct port messaging instead of broadcasting, improving reliability and security. [1] [2]

Testing improvements:

• Added and updated tests for sidebar behavior, settings, and port management to ensure robust handling of edge cases and cross-browser support. [1] [2]

These changes collectively make the sidebar more robust, reliable, and compatible across browsers, while ensuring that signing requests and user actions are handled safely and predictably.

Screen.Recording.2026-04-02.at.1.08.39.PM.mov

[Copilot]

Pull request overview

This PR improves the browser extension’s “sidebar mode” integration by moving sidebar communication to a long-lived port, strengthening cross-browser (Chrome/Firefox) sidebar behavior, and adding UI + tests to handle concurrent signing flows more safely.

Changes:

• Introduces a long-lived sidebar port connection and uses it for sidebar navigation + cleanup on disconnect.
• Adds Firefox sidebar support (sidebar_action) and updates popup/sidebar opening logic for both Chrome and Firefox.
• Adds a “new signing request” interstitial route/view plus new unit tests for sidebar and settings behavior.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
extension/src/popup/views/ConfirmSidebarRequest/index.tsx	Adds interstitial screen to gate concurrent signing requests in sidebar mode
extension/src/popup/views/ConfirmSidebarRequest/styles.scss	Styles for the new interstitial view
extension/src/popup/Router.tsx	Registers the new interstitial route
extension/src/popup/metrics/views.ts	Maps new route to a metrics event name
extension/src/popup/constants/routes.ts	Adds confirmSidebarRequest route constant
extension/src/popup/constants/metricsNames.ts	Adds a metric name for the new interstitial view
extension/src/popup/components/SidebarSigningListener/index.tsx	Switches from broadcast messages to port-based sidebar navigation; adds route gating
extension/src/popup/components/account/AccountHeader/index.tsx	Shows sidebar option for Chrome sidePanel and Firefox sidebarAction
extension/src/popup/helpers/navigate.ts	Ensures sidebar open supports Firefox and closes popup after opening
extension/src/popup/helpers/isSidebarMode.ts	Hardens sidebar-mode detection to extension origins only
extension/src/popup/locales/en/translation.json	Adds new strings for sidebar mode + interstitial
extension/src/popup/locales/pt/translation.json	Adds new strings for sidebar mode + interstitial (but see comment)
extension/src/background/index.ts	Adds sidebar port connection listener; rejects/cleans queues on sidebar disconnect; adds Firefox click handling
extension/src/background/messageListener/freighterApiMessageListener.ts	Uses port for sidebar navigation; uses browser.runtime.getURL; adds safeResolve guards
extension/src/background/messageListener/popupMessageListener.ts	Removes sidebar register/unregister messages; restricts OPEN_SIDEBAR to extension-page senders
extension/src/background/messageListener/handlers/loadSettings.ts	Changes parsing of isOpenSidebarByDefault (but see comment)
extension/src/background/messageListener/handlers/saveSettings.ts	Changes parsing/return of isOpenSidebarByDefault (but see comment)
extension/src/background/messageListener/tests/sidebar.test.ts	Adds tests for sidebar authorization + port/windowId management
extension/src/background/messageListener/tests/loadSaveSettings.test.ts	Adds tests for sidebar-by-default setting parsing/behavior (but see comment)
extension/public/static/manifest/v3.json	Adds Firefox sidebar_action manifest configuration
@shared/constants/services.ts	Removes sidebar register/unregister service types; adds SIDEBAR_NAVIGATE constant
@shared/api/types/message-request.ts	Removes SidebarRegister/Unregister message types

Comments suppressed due to low confidence (3)

extension/src/background/messageListener/freighterApiMessageListener.ts:446

• browser.windows.onRemoved.addListener here does not filter by the popup window id, meaning unrelated window closes can trigger a declined response. Additionally, the anonymous listener can’t be removed and will accumulate across requests. Filter on popup.id and remove the listener after resolve.

        } else {
          browser.windows.onRemoved.addListener(() =>
            safeResolve({
              // return 2 error formats: one for clients running older versions of freighter-api, and one to adhere to the standard wallet interface
              apiError: FreighterApiDeclinedError,
              error: FreighterApiDeclinedError.message,
            }),
          );

extension/src/background/messageListener/freighterApiMessageListener.ts:535

• This browser.windows.onRemoved listener is unscoped (no windowId check) and anonymous (can’t be removed), so it can decline requests when unrelated windows close and will leak listeners over time. Scope it to popup.id and unregister it after resolving.

        } else {
          browser.windows.onRemoved.addListener(() =>
            safeResolve({
              // return 2 error formats: one for clients running older versions of freighter-api, and one to adhere to the standard wallet interface
              apiError: FreighterApiDeclinedError,
              error: FreighterApiDeclinedError.message,
            }),
          );

extension/src/background/messageListener/freighterApiMessageListener.ts:638

• browser.windows.onRemoved.addListener is added without checking which window was removed, so any window close can cause a decline. Because the callback is anonymous it can’t be removed, causing listener buildup. Use a named handler with removedWindowId === popup.id and remove it after resolve.

        } else {
          browser.windows.onRemoved.addListener(() =>
            safeResolve({
              // return 2 error formats: one for clients running older versions of freighter-api, and one to adhere to the standard wallet interface
              apiError: FreighterApiDeclinedError,
              error: FreighterApiDeclinedError.message,
            }),
          );

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[piyalbasu]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Both issues from that review are addressed in 26200d0 — see replies on the individual comment threads above.

[piyalbasu]

Code review

Found 3 issues:

1. setAllowedStatus is missing sidebarQueueUuids tracking, unlike every other signing handler

Every other handler (requestAccess, submitToken, submitTransaction, submitBlob, submitAuthEntry) adds its UUID to sidebarQueueUuids when routed to the sidebar and deletes it in safeResolve. setAllowedStatus was updated with the safeResolve pattern in this PR but omits both sidebarQueueUuids.add(uuid) and sidebarQueueUuids.delete(uuid). If the sidebar disconnects while a setAllowedStatus request is pending, the disconnect handler in index.ts won't find the UUID in sidebarQueueUuids, so the dapp's promise hangs until the TTL timeout instead of being rejected immediately.

freighter/extension/src/background/messageListener/freighterApiMessageListener.ts

Lines 783 to 800 in 26200d0

	await openSigningWindow(`/grant-access?${encodeOrigin}`, 400);
	return new Promise((resolve) => {
	let resolved = false;
	const safeResolve = (value: any) => {
	if (resolved) return;
	resolved = true;
	resolve(value);
	};
	setTimeout(
	() =>
	safeResolve({
	apiError: FreighterApiDeclinedError,
	error: FreighterApiDeclinedError.message,
	}),
	QUEUE_ITEM_TTL_MS,

2. ConfirmSidebarRequest "Reject" button navigates home but does not reject the pending signing request

handleReject only calls navigate(ROUTES.account). The new signing request that triggered this interstitial is still sitting in responseQueue with no resolution. The dapp is left with a hanging promise until the 5-minute TTL fires or the sidebar disconnects. All other rejection paths call responseQueueItem.response(undefined) to signal rejection back to the dapp immediately.

freighter/extension/src/popup/views/ConfirmSidebarRequest/index.tsx

Lines 39 to 42 in 26200d0

	};
	const handleReject = () => {
	navigate(ROUTES.account);
	};

3. openSidebar() in navigate.ts moves window.close() inside the try block, so the popup is never closed on error

Previously window.close() ran unconditionally after the try/catch. Now if chrome.sidePanel.setOptions or .open throws (e.g., API unavailable), execution jumps to catch and window.close() is skipped, leaving the popup orphaned.

freighter/extension/src/popup/helpers/navigate.ts

Lines 30 to 35 in 26200d0

	await chrome.sidePanel.open({ windowId: win.id! });
	}
	window.close();
	} catch (e) {
	console.error("Failed to open sidebar:", e);
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Code review

Found 3 issues:...

Addressed in c6e0f88:

1. setAllowedStatus sidebarQueueUuids tracking — already present in the current code (both sidebarQueueUuids.add and .delete were in place from the previous commit), no change needed.
2. ConfirmSidebarRequest Reject not rejecting — fixed by introducing REJECT_SIGNING_REQUEST and rejectSigningRequest handler that cleans all five queues; handleReject now calls it.
3. openSidebar() window.close() — already fixed in the current code (window.close() is in both the try and catch blocks), no change needed.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[piyalbasu]

I added this spec because there ended up being some "gotchas" with this implementation.

First, there's the difference in browser implementation

Second, the inability to add e2e tests with Playwright

I asked Claude how we can help future agents have an easier time making changes and it guided me towards this solution: this type of spec that is linked in CLAUDE.MD should help make any architectural decisions and prevent agents from going down dead-ends we've already investigated in the future

[piyalbasu]

It's also nice to have a human readable description of what we're doing here for future human devs! 😄

[CassioMG]

@piyalbasu overall I think the PR is looking good! Posting here some findings and suggestions from superpowers for you to take a look - thanks!

Important

I1. REJECT_SIGNING_REQUEST handler lacks isFromExtensionPage guard

File: extension/src/background/messageListener/popupMessageListener.ts — REJECT_SIGNING_REQUEST case (~line 355)

Unlike OPEN_SIDEBAR which explicitly checks if (!isFromExtensionPage) return { error: "Unauthorized" }, REJECT_SIGNING_REQUEST has no sender validation. This handler resolves a dApp's pending signing promise with undefined — defense-in-depth warrants an explicit guard.

Fix: Add if (!isFromExtensionPage) return { error: "Unauthorized" } at the top of the case.

I2. Duplicated queue cleanup logic

Files: extension/src/background/index.ts (disconnect handler, ~lines 121-145) and extension/src/background/messageListener/handlers/rejectSigningRequest.ts

The disconnect handler duplicates the exact same iterate-find-splice-resolve pattern across all 5 queues. If either is updated (e.g., a new queue type), the other must be updated in lockstep.

Fix: Have the disconnect handler call rejectSigningRequest for each UUID, or extract a shared removeFromAllQueues(uuid, queues) helper.

I3. ConfirmSidebarRequest uses raw HTML instead of design system components

File: extension/src/popup/views/ConfirmSidebarRequest/index.tsx — lines 68-76

Uses <h1> with custom CSS font-size/font-weight and <div> for body text. Per project conventions: "Always use Text or Heading from @stellar/design-system — never raw HTML text elements with custom font styles."

Fix:

// Replace:
<h1 className="ConfirmSidebarRequest__title">{t("New Signing Request")}</h1>
<div className="ConfirmSidebarRequest__body">{t("A new signing request...")}</div>

// With:
<Heading as="h1" size="md" addlClassName="ConfirmSidebarRequest__title">{t("New Signing Request")}</Heading>
<Text as="p" size="sm" addlClassName="ConfirmSidebarRequest__body">{t("A new signing request...")}</Text>

Also in styles.scss: replace font-weight: 500 with var(--font-weight-medium), font-weight: 400 with var(--font-weight-regular), and remove the hardcoded font-family: "Inter", sans-serif.

I4. requestAccess and setAllowedStatus missing rejectOnWindowClose for popup windows (pre-existing)

File: extension/src/background/messageListener/freighterApiMessageListener.ts

requestAccess (~line 174) has if (popup === null) (sidebar TTL fallback) but no else if (popup !== null) branch calling rejectOnWindowClose. If the user closes the popup window, the dApp promise hangs until TTL (~5 min). Same for setAllowedStatus (~line 770).

Compare with submitTransaction, submitBlob, submitAuthEntry which all have the three-way branch: undefined (error), null (sidebar TTL), !== null (popup window close via rejectOnWindowClose).

Note: Pre-existing — not introduced by this PR. But since this audit added rejectOnWindowClose as a consolidated helper, it's a good opportunity to fix these two.

Fix: Add else if (popup !== null) { rejectOnWindowClose(popup.id!, safeResolve); } to both flows.

I5. submitToken missing sidebar TTL fallback (pre-existing)

File: extension/src/background/messageListener/freighterApiMessageListener.ts — submitToken (~line 283)

Has popup === undefined (error) and popup !== null (window close via rejectOnWindowClose), but no popup === null (sidebar) branch with TTL fallback. Inconsistent with the other signing flows. The sidebar disconnect cleanup via sidebarQueueUuids covers this, but the explicit TTL fallback is a safety net.

Note: Pre-existing.

---

Code Quality

C1. import statement placed after function definition

File: extension/src/background/messageListener/freighterApiMessageListener.ts — ~line 126

const rejectOnWindowClose = (...) => { ... };

import { DataStorageAccess } from "background/helpers/dataStorageAccess";

Import placed between the rejectOnWindowClose function and the main export. Works due to hoisting, but imports should be at the top of the file.

C2. safeResolve typed as (value: any) => void

File: extension/src/background/messageListener/freighterApiMessageListener.ts — rejectOnWindowClose and all safeResolve closures

Using any loses type safety on the resolve value. Could be typed generically or at minimum use unknown.

C3. SIDEBAR_NAVIGATE exported as plain string constant, not in an enum

File: @shared/constants/services.ts:77

All other service types use the SERVICE_TYPES or EXTERNAL_SERVICE_TYPES enums. A comment explaining why it's separate (only used on ports, not through standard message routing) would help.

C4. safeResolve duplicated 6 times inline

File: extension/src/background/messageListener/freighterApiMessageListener.ts

The identical closure appears in all 6 signing handlers. Consider extracting a makeSafeResolve helper:

const makeSafeResolve = <T>(resolve: (value: T) => void) => {
  let resolved = false;
  return (value: T) => {
    if (resolved) return;
    resolved = true;
    resolve(value);
  };
};

---

Suggestions

S1. Derive SIGNING_ROUTE_PREFIXES from ALLOWED_NAV_PREFIXES

File: extension/src/popup/components/SidebarSigningListener/index.tsx

Two arrays that must stay in sync. Derive one from the other:

const SIGNING_ROUTE_PREFIXES = [...ALLOWED_NAV_PREFIXES, ROUTES.confirmSidebarRequest];

S2. Extract 500ms debounce to a named constant

File: extension/src/background/index.ts — ~line 147

export const SIDEBAR_DISCONNECT_DEBOUNCE_MS = 500;

S3. Guard ConfirmSidebarRequest route against non-sidebar access

File: extension/src/popup/Router.tsx

The route is registered globally. Consider redirecting to ROUTES.account if !isSidebarMode().

S4. window.close() in navigate.ts — use finally

File: extension/src/popup/helpers/navigate.ts

window.close() now appears in both try and catch. A finally block would be clearer.

---

Missing Test Coverage

1. rejectSigningRequest handler — No dedicated tests. Should verify: UUID found in responseQueue -> response called with undefined; UUID not found -> Sentry exception captured; missing UUID -> early return; all queue types cleaned up.

2. ConfirmSidebarRequest component — isValidNextRoute contains security-sensitive open-redirect validation. Should test edge cases: //evil.com, javascript:alert(1), empty strings, encoded characters, valid routes.

3. SidebarSigningListener route allowlist — No tests verifying disallowed routes are blocked.

---

Nits

Location	Issue
ConfirmSidebarRequest/styles.scss:27	--sds-clr-amber-* tokens — verify these exist in the SDS token set (CLAUDE.md only documents grayscale and red)
ConfirmSidebarRequest/styles.scss:37	font-weight: 500 → use var(--font-weight-medium)
ConfirmSidebarRequest/styles.scss:45	font-weight: 400 → use var(--font-weight-regular)
queueCleanup.ts:18-22	Comment could link to the actual cleanup code location

---

Summary Table

Category	Count	Items
Important	5	I1-I5 (I4-I5 pre-existing)
Code Quality	4	C1-C4
Suggestions	4	S1-S4
Missing Tests	3	rejectSigningRequest, ConfirmSidebarRequest, route allowlist
Nits	4	Style conventions