Skip to content

✏️ Update network dropdown labels - #2784

Merged
piyalbasu merged 6 commits into
stellar:masterfrom
JFWooten4:fix/network-dropdown-labels
Aug 7, 2026
Merged

piyalbasu merged 6 commits into
stellar:masterfrom
JFWooten4:fix/network-dropdown-labels

Conversation

@JFWooten4

Copy link
Copy Markdown
Contributor

 Right now, the network selection dropdown shows locale selection as two words, which diverges from my documentation standards and the Lab.

This only changes the display text in the account header dropdown. The underlying stored networkName values remain unchanged for compatibility with existing settings and network switching.

Copilot AI review requested due to automatic review settings May 16, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the network selector dropdown in the account header to display single-word network labels (e.g., “Mainnet”, “Testnet”) while keeping the underlying stored networkName values unchanged for compatibility.

Changes:

  • Added a getNetworkDisplayName(networkName) helper to map stored network names to desired display labels.
  • Updated the network dropdown row rendering to show the mapped display name instead of the raw networkName.
Comments suppressed due to low confidence (2)

extension/src/popup/components/account/AccountHeader/index.tsx:57

  • This mapping updates only Main/Test. If the user enables experimental mode, Futurenet becomes the default network (NETWORK_NAMES.FUTURENET == "Future Net") and will still display as two words. Consider also mapping "Future Net" -> "Futurenet" to keep dropdown labels consistent with docs/UI elsewhere.
  switch (networkName) {
    case "Main Net":
      return "Mainnet";
    case "Test Net":
      return "Testnet";
    default:
      return networkName;

extension/src/popup/components/account/AccountHeader/index.tsx:277

  • Changing the dropdown labels will break existing UI tests that locate these options by text (e.g. many Playwright e2e tests use getByText("Main Net") / getByText("Test Net") in extension/e2e-tests/**). Please update those assertions/selectors to the new labels ("Mainnet"/"Testnet") or switch tests to a more stable selector than visible text.
                            <NetworkIcon index={i} />
                            <div className="AccountHeader__network-copy">
                              {getNetworkDisplayName(n.networkName)}
                            </div>

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

Comment thread extension/src/popup/components/account/AccountHeader/index.tsx Outdated
@JFWooten4

Copy link
Copy Markdown
Contributor Author

Anything else you'd like to see here, @JakeUrban?

@JakeUrban

Copy link
Copy Markdown
Contributor

We're supportive of this change, but as of now this PR isn't mergable since the tests are passing. If you can update the tests to match the changes we should be able to get this merged, thanks!

@JFWooten4

Copy link
Copy Markdown
Contributor Author

I think I fixed it. It's been running the test for over a day now.

@JFWooten4 JFWooten4 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed all tests passing on local deploy, cc @piyalbasu as the integration tree was modified.

  • Jest: 197 suites, 1,524 tests passed.
  • E2E: all 223 enabled scenarios passed across complete segmented runs.
  • Builds and git diff --check passed: API, docs, standard extension, experimental, production, and translations.

6 Jest suites/51 tests and 10 E2E cases are source-skipped. Four E2E skips require the secret-backed integration environment. Visual tests ran using temporary Windows baselines because the repository only commits macOS baselines; those temporary files were not retained.

Comment thread extension/e2e-tests/integration-tests/freighterApiIntegration.test.ts Outdated
Comment thread extension/e2e-tests/onboarding.test.ts
@JFWooten4
JFWooten4 force-pushed the fix/network-dropdown-labels branch from 9cc56c7 to 0d7c572 Compare August 6, 2026 17:26
JFWooten4 and others added 4 commits August 6, 2026 13:27
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@JFWooten4

Copy link
Copy Markdown
Contributor Author

Re failed run: fork PRs cannot access INDEXER_URL secrets. The extension builds with an empty URL, so account-history URL construction fails before Playwright can intercept it. Four history tests then render “No transactions to show.” The failure is unrelated to the network-label code.

@piyalbasu

Copy link
Copy Markdown
Contributor

toHaveCount(0) encodes the no-backend rendering and will fail on master

Merge blocker — will happen on every run, not an edge case. Everything else here is fine; this is a one-line revert.

TL;DR: One assertion in this PR was changed to expect that the Confirm button is missing from the "domain not allowed" screen. That's only true when the extension is built without a working backend, which is the situation on a fork PR. Maintainer runs do have a backend, the button renders there, and the test fails. So this would go green on your branch and red on master immediately after merge.

Thanks for adding the stubs in this file, by the way — those are doing real work. I verified they're what makes four of these tests survive without a backend at all, which is exactly what's needed for #2943 to work. This one assertion is the only thing in the group that goes the wrong way.

Steps to reproduce:

  1. Build the extension with a working INDEXER_URL (any maintainer checkout, or CI on master).
  2. Run npx playwright test integration-tests/freighterApiIntegration.test.ts:243.
  3. Fails: expected 0 elements, found 1.

Detailed explanation (for agents)

Root cause

await expect(txPopup.getByTestId("sign-transaction-sign")).toHaveCount(0);

sign-transaction-sign renders in the non-Blockaid branch of the actions row:

</span>
</Button>
<Button
data-testid="sign-transaction-sign"
disabled={isSubmitDisabled}
variant="secondary"
isFullWidth
isRounded
size="lg"
isLoading={isConfirming}
onClick={() => handleApprove()}
>
{t("Confirm")}
</Button>
</>

Which branch renders depends on scan data. With a reachable backend the button is present and disabled; with no reachable backend the component takes the other path and the button is absent. The assertion therefore encodes an environment, not a behavior.

Measured, varying only the build's INDEXER_URL (four runs of this file, 26 tests, --max-failures=0):

Build Test file Result
Real URL master 25 passed, 0 failed
Placeholder master 21 passed, 4 failed
Placeholder this PR 24 passed, 1 failed (screenshot only)
Real URL this PR :243 fails

Row 4:

> 274 |   await expect(txPopup.getByTestId("sign-transaction-sign")).toHaveCount(0);
Error: expect(locator).toHaveCount(expected) failed
Expected: 0
Received: 1
  14 × locator resolved to 1 element

Rows 2 and 3 are the useful part: the four tests failing without a backend are precisely the four this PR touches, and the stubs added here fix three of them outright.

Suggested fix

Revert this one line to await expect(txPopup.getByTestId("sign-transaction-sign")).toBeDisabled(); and let the stubs added elsewhere in this PR put the popup in the state where that holds. The assertion is the stronger one anyway — "present but not clickable" is the actual product guarantee; "absent" would also be satisfied by the actions row failing to render at all.

Context worth knowing

The likely origin of the wider test churn in this PR: fork PRs can't read INDEXER_URL, so your builds had no backend, and several tests genuinely fail in that environment. #2943 fixes that at the source. Once it lands, the environment you were developing against matches CI and the workarounds become unnecessary.

One knock-on: domain-not-allowed-sign-transaction.png differs by ~5% of pixels without a backend, since baselines were captured with one. Not caused by this PR and not yours to fix — noting it because it's the same root cause and it affects #2943.

@piyalbasu piyalbasu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing but I have confirmed that these would pass when the test runner is able to properly run e2e tests. Merging PR #2943 will make sure future forked PR's can run e2e tests

@piyalbasu
piyalbasu merged commit ef59dfd into stellar:master Aug 7, 2026
6 of 7 checks passed
@JFWooten4
JFWooten4 deleted the fix/network-dropdown-labels branch August 7, 2026 16:48
piyalbasu added a commit that referenced this pull request Aug 7, 2026
#2784 raised the test timeout 15s -> 60s and added a 20s expect timeout.
Those compensated for tests hanging on unreachable requests in a
no-backend environment -- the root cause fixed by the scan stubs in the
previous commit -- rather than for genuinely slow tests.

A/B on this tree, no-backend config, one shared build so the timeout is
the only variable (236 scenarios, --max-failures=0):

            passed  flaky  failed
  60s         223      2       0
  15s         221      4       0

Both reach 225 eventually-passing with no failures. The flaky sets are
disjoint between arms -- none of the 60s flakes recur at 15s -- which is
the signature of ambient flakiness rather than a timeout squeeze. The
same config produced 2, 6 and 11 flaky across runs today, so a 2 vs 4
difference sits inside normal variance. Nothing failed at 15s that
passed at 60s, even with 5 retries.

Restoring the shorter ceiling means a genuinely stuck test fails in
seconds rather than minutes, which matters most on fork PRs where
maxFailures truncates the run at the first failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
piyalbasu added a commit that referenced this pull request Aug 7, 2026
* test(e2e): stub the last two backend-dependent scan calls

Two e2e tests reach the real backend instead of their stubs. This is
invisible with credentials but breaks fork PRs, and is the last thing
blocking #2943.

Both are the same defect: an unstubbed Blockaid scan. When the scan
can't complete the component takes its unable-to-scan branch and the
element under test isn't rendered at all.

- freighterApiIntegration "should not sign transaction when not
  allowed": stubAllExternalApis registers scan-tx on the opener page but
  scan-dapp on the context, so the approval popup -- a separate page --
  inherits only the latter. Without the scan, the actions row renders
  its Blockaid branch, which has no sign-transaction-sign button at all.
  Applies the same per-popup stub #2784 added to the multiple-requests
  test.

- swap "recovers to the amount screen when the swap quote expires at
  submit": the suite stubs scan-tx but never scan-asset, and the test
  runs on mainnet where Blockaid is enabled. The review screen renders
  its warning variant, "Swap XLM to AQUA" never appears, and the test
  burns its full 180s timeout rather than failing fast.

Full suite (236 scenarios, --max-failures=0), varying only the build's
INDEXER_URL:

  real URL     225 passed, 0 failed  (unchanged)
  no backend   221 passed, 2 failed -> 225 passed, 0 failed

The two configurations now agree, which is what #2943 requires.
domain-not-allowed-sign-transaction.png matches the committed baseline
once scan-tx is stubbed; no regeneration needed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(e2e): restore the pre-#2784 Playwright timeouts

#2784 raised the test timeout 15s -> 60s and added a 20s expect timeout.
Those compensated for tests hanging on unreachable requests in a
no-backend environment -- the root cause fixed by the scan stubs in the
previous commit -- rather than for genuinely slow tests.

A/B on this tree, no-backend config, one shared build so the timeout is
the only variable (236 scenarios, --max-failures=0):

            passed  flaky  failed
  60s         223      2       0
  15s         221      4       0

Both reach 225 eventually-passing with no failures. The flaky sets are
disjoint between arms -- none of the 60s flakes recur at 15s -- which is
the signature of ambient flakiness rather than a timeout squeeze. The
same config produced 2, 6 and 11 flaky across runs today, so a 2 vs 4
difference sits inside normal variance. Nothing failed at 15s that
passed at 60s, even with 5 retries.

Restoring the shorter ceiling means a genuinely stuck test fails in
seconds rather than minutes, which matters most on fork PRs where
maxFailures truncates the run at the first failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants