Add ProtoOS E2E coverage for no-pools callout suppression by edgars-avotins · Pull Request #483 · block/proto-fleet

edgars-avotins · 2026-06-17T08:24:23Z

Summary

add ProtoOS E2E coverage for the route-sensitive no-pools global callout behavior
verify the global no-pools banner appears on Home when pools are cleared
verify the Pools page shows its own empty state while suppressing the global banner

Testing

./node_modules/.bin/eslint e2eTests/protoOS/pages/home.ts e2eTests/protoOS/pages/pools.ts e2eTests/protoOS/spec/pools.spec.ts
npx playwright test spec/00-onboarding.spec.ts --project=desktop
npx playwright test spec/pools.spec.ts --project=desktop --grep "Suppress no-pools global callout"
npx playwright test spec/pools.spec.ts --project=desktop
npx playwright test spec/pools.spec.ts --project=mobile --grep "Suppress no-pools global callout"

github-actions · 2026-06-17T08:26:09Z

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

Reviewed pull request diff only (d34b3cd7fc43747cf82d7170d3bd4e0b5756b8c9...70fa0a4c85e268c420b80786ea43609d7f1e956a, exact PR three-dot diff)

Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.

Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] E2E Test Clears All Mining Pools Without Restoring Them

Category: Reliability
Location: client/e2eTests/protoOS/spec/pools.spec.ts:104
Description: The new test calls clearAllPoolsViaApi(), which posts [] to POST /api/v1/pools. That endpoint replaces the current pool configuration, so this permanently leaves the target miner/test fixture with zero configured pools. There is no finally, afterEach, or fixture-level restore of the previous/default pool configuration.
Impact: A successful test run contaminates shared state for later specs, later Playwright projects, or the next suite run. If this suite is pointed at real miner hardware, it also leaves the miner without mining pools configured, interrupting mining and revenue until manually repaired.
Recommendation: Snapshot the existing pool configuration before clearing it and restore it in a finally/afterEach, or create this empty-state scenario in an isolated disposable fixture. At minimum, recreate the default test pool before the test exits.

Notes

The reviewed diff only changes ProtoOS Playwright page objects/specs. I did not find auth, SQL injection, gRPC, command injection, infrastructure, protobuf, Rust, or cryptostealing/pool-hijack issues in the changed hunks.

_{Generated by Codex Security Review |

Triggered by: @edgars-avotins |

Review workflow run}

Copilot

Pull request overview

Adds ProtoOS Playwright E2E coverage for the route-sensitive “no pools” global callout behavior, ensuring the global banner appears on Home when pools are cleared, but is suppressed on the Pools empty-state page in favor of the page-specific empty state.

Changes:

Added a new Pools spec asserting global “no pools” callout behavior on Home and suppression on Pools settings.
Added Pools page-object helpers to clear pools via authenticated API and validate the empty state / hidden global callout.
Added Home page-object helpers to validate and click the “no pools” callout CTA.

Reviewed changes

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

File	Description
client/e2eTests/protoOS/spec/pools.spec.ts	Adds a new scenario validating global callout vs Pools empty-state suppression.
client/e2eTests/protoOS/pages/pools.ts	Adds API-based pool clearing and new empty-state/callout assertions.
client/e2eTests/protoOS/pages/home.ts	Adds helpers to assert/click the “no pools configured” callout on Home.

+  async validateNoMiningPoolsCallout() {
+    const callout = this.page.getByTestId("callout");
+    await expect(callout).toBeVisible();
+    await expect(callout.getByText("No mining pools configured.")).toBeVisible();
+    await expect(callout.getByRole("button", { name: "Add mining pools" })).toBeVisible();
+  }


+  async validateNoPoolsEmptyState() {
+    await expect(this.page.getByText("Pools", { exact: true })).toBeVisible();
+    await expect(this.page.getByText("Add up to 3 pools for your miner.")).toBeVisible();
+    await expect(this.page.getByTestId("add-pool-button")).toBeVisible();
+  }


+  test("Suppress no-pools global callout on the pools empty state page", async ({ homePage, poolsPage }) => {
+    await test.step("Clear configured pools through the authenticated API", async () => {
+      await poolsPage.clearAllPoolsViaApi();
+      await homePage.reloadPage();
+    });


chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70fa0a4c85

ℹ️ 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".

chatgpt-codex-connector · 2026-06-17T08:30:16Z

+  }
+
+  async clickAddMiningPoolsInCallout() {
+    await this.page.getByTestId("callout").getByRole("button", { name: "Add mining pools" }).click();


Make the CTA locator unique

When Home has no pools, the route renders two NoPoolsCallouts: the App-level banner in client/src/protoOS/components/App/App.tsx:400-402 and the KPI layout banner in client/src/protoOS/features/kpis/components/KpiLayout/KpiLayout.tsx:58. Both use the default data-testid="callout" and button text Add mining pools, so this click() locator matches two buttons; Playwright strict locator actions throw when an action resolves to more than one element, so the new E2E fails before it can assert the Pools page. Scope the click to the intended banner or otherwise make the locator unique.

Useful? React with 👍 / 👎.

Add ProtoOS no-pools callout E2E coverage

70fa0a4

edgars-avotins requested a review from a team as a code owner June 17, 2026 08:24

Copilot AI review requested due to automatic review settings June 17, 2026 08:24

github-actions Bot assigned edgars-avotins Jun 17, 2026

github-actions Bot added javascript Pull requests that update javascript code client labels Jun 17, 2026

Copilot started reviewing on behalf of edgars-avotins June 17, 2026 08:24 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Jun 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add ProtoOS E2E coverage for no-pools callout suppression#483

Add ProtoOS E2E coverage for no-pools callout suppression#483
edgars-avotins wants to merge 1 commit into
mainfrom
codex/protoos-no-pools-callout-e2e

edgars-avotins commented Jun 17, 2026

Uh oh!

github-actions Bot commented Jun 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

edgars-avotins commented Jun 17, 2026

Summary

Testing

Uh oh!

github-actions Bot commented Jun 17, 2026

🔐 Codex Security Review

Review Summary

Findings

[MEDIUM] E2E Test Clears All Mining Pools Without Restoring Them

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants