Skip to content

[codex] Add reparent E2E scenarios#530

Open
edgars-avotins wants to merge 13 commits into
mainfrom
codex/protofleet-reparent-e2e-scenarios
Open

[codex] Add reparent E2E scenarios#530
edgars-avotins wants to merge 13 commits into
mainfrom
codex/protofleet-reparent-e2e-scenarios

Conversation

@edgars-avotins

Copy link
Copy Markdown
Contributor

Reviewable diff: +0/-0 across 0 files (excludes generated, test, and story files).

Summary

This PR adds ProtoFleet E2E coverage for three reparent workflows that operators can trigger from the existing Fleet surfaces: moving a rack to a building, assigning a miner to a site, and assigning a miner to a rack. The spec verifies the network requests and resulting UI state for each flow, while also handling the local dev configuration where multi-site Fleet tabs may be disabled and the legacy sites/settings routes remain the stable management surface.

How it works

The new Playwright spec provisions temporary sites, buildings, and racks as needed, performs the reparent action from the Racks or Miners tab, then validates the request payload sent to the backend and the resulting placement state visible in the app. For local environments where VITE_MULTI_SITE_ENABLED is off, setup and cleanup use the legacy /settings/sites and related site-scoped building surfaces, but the user actions under test still happen from the Fleet Racks and Miners tabs.

Diagrams

flowchart LR
  A["Create temporary site/building/rack state"] --> B["Open Fleet Racks or Miners tab"]
  B --> C["Trigger reparent action from row/menu UI"]
  C --> D["Select target in ParentPickerModal"]
  D --> E["Capture assign request payload"]
  E --> F["Assert resulting placement in UI"]
  F --> G["Clean up temporary state"]
Loading

Areas of the code involved

Area / package / file What changed Why it matters for review
client/e2eTests/protoFleet/spec/minersReparent.spec.ts Added a new Playwright spec covering rack-to-building, miner-to-site, and miner-to-rack reparent flows, plus environment-aware setup/cleanup helpers. This is the full behavior change in the PR; reviewers can focus on scenario selection, assertions, and local-environment compatibility.

Key technical decisions & trade-offs

  • The spec validates request payloads and final visible state instead of depending on transient toast copy, because the local legacy surfaces do not render every success toast consistently.
  • Setup and cleanup branch between Fleet multi-site pages and legacy settings pages so the scenarios stay runnable in both local dev modes instead of requiring a flag-specific environment.
  • The miner fixture selection prefers an unracked paired miner when available, but falls back to any visible miner so the scenarios remain executable against the current local seed data.

Testing & validation

  • ./node_modules/.bin/eslint e2eTests/protoFleet/spec/minersReparent.spec.ts
  • PW_UI_NO_DEPS=1 npx playwright test spec/minersReparent.spec.ts --project=desktop
  • Not covered: mobile/browser-matrix runs and the broader ProtoFleet E2E suite.

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

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown

🔐 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 (0ef9ee230e6aac5fd94746ad1af8e332af6e7abd...4584cba07bee2bca077b5c1fa8b033e72066765a, 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] Best-effort rack cleanup can leave persistent test fixtures behind

  • Category: Reliability
  • Location: client/e2eTests/protoFleet/pages/racks.ts:551
  • Description: deleteRackByLabelIfVisible performs cleanup with short tryAction calls and returns if the rack row is not opened within that best-effort window. It also ignores whether validateRackDeletedToast() succeeded. The new minersReparent.spec.ts relies on this helper from finally blocks after creating temporary racks.
  • Impact: A slow list load, transient UI issue, or missed delete confirmation can leave temporary racks in the shared E2E database. Subsequent site cleanup is not a reliable substitute for deleting the rack, so later E2E runs can inherit polluted placement state and fail unrelated rack/miner count assertions.
  • Recommendation: Make cleanup deterministic: wait for the rack list to stabilize before deciding the rack is absent, perform the delete, then assert either the delete RPC succeeds or the row is gone. Only swallow the “already absent” case after a loaded list proves it.

Notes

The authoritative diff only changes ProtoFleet Playwright E2E code: fixtures, page objects, helpers, and a new minersReparent.spec.ts. I did not find direct PR changes to authentication, authorization, SQL, protobufs, backend handlers, plugins, infrastructure, mining pool assignment logic, or production frontend rendering paths.


Generated by Codex Security Review |
Triggered by: @edgars-avotins |
Review workflow run

@edgars-avotins edgars-avotins force-pushed the codex/protofleet-reparent-e2e-scenarios branch from 1447f24 to 2691f13 Compare June 25, 2026 08:40
@github-actions github-actions Bot added the github_actions Pull requests that update GitHub Actions code label Jun 25, 2026
@edgars-avotins edgars-avotins marked this pull request as ready for review June 26, 2026 10:34
@edgars-avotins edgars-avotins requested a review from a team as a code owner June 26, 2026 10:34
Copilot AI review requested due to automatic review settings June 26, 2026 10:34

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

Adds new ProtoFleet Playwright E2E coverage for “reparent” workflows (rack → building, miner → site, miner → rack), plus a small page-object tweak and a CI workflow adjustment to run the new spec without setup-project dependencies.

Changes:

  • Added minersReparent.spec.ts with three end-to-end reparent scenarios, including setup/cleanup and request-payload assertions.
  • Updated RacksPage.clickSaveRack() to scope the “Save” click to the full-screen modal.
  • Updated the ProtoFleet E2E GitHub Actions workflow to set PW_UI_NO_DEPS=1 for the new spec shard.

Reviewed changes

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

File Description
client/e2eTests/protoFleet/spec/minersReparent.spec.ts New Playwright spec covering three reparent operator workflows with request + UI assertions.
client/e2eTests/protoFleet/pages/racks.ts Makes rack “Save” interaction more specific by scoping it to the full-screen modal.
.github/workflows/protofleet-e2e-tests.yml Skips setup project dependencies for the new spec shard via PW_UI_NO_DEPS=1.

Comment on lines +354 to +358
const responsePromise = new Promise<VisibleMinerSnapshot[]>((resolve, reject) => {
const timeoutId = setTimeout(() => {
page.off("response", handleResponse);
resolve([]);
}, 10000);
Comment on lines +230 to +238
async function detectSiteManagementMode(page: Page): Promise<SiteManagementMode> {
await resetActiveSiteSelection(page);
await openSitesManagementPage(page, "fleet");
return "fleet";
}

async function openSitesManagementPage(page: Page, mode: SiteManagementMode) {
void mode;
await page.goto("/fleet/sites");
Comment on lines +287 to +292
if [ "$MATRIX_SPEC" = "minersReparent.spec.ts" ]; then
PLAYWRIGHT_BLOB_OUTPUT_FILE="blob-report/${MATRIX_PROJECT}-${SPEC_NAME}.zip" \
PWTEST_BLOB_DO_NOT_REMOVE=1 \
PW_UI_NO_DEPS=1 \
npx playwright test --project="$MATRIX_PROJECT" --reporter=blob "spec/$MATRIX_SPEC"
else
@github-actions github-actions Bot removed the github_actions Pull requests that update GitHub Actions code label Jun 26, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2666a50874

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

Comment on lines +297 to +299
const continueButton = this.page.getByRole("button", { name: "Continue", exact: true });
if (await continueButton.isVisible().catch(() => false)) {
await continueButton.click();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Wait for the site-move conflict dialog

When cleanup restores a miner that originally belonged to another site after the assign-to-rack scenario, the miner is still in the temporary rack/site, so AssignDevicesToSite first returns the cross-site rack conflict and renders a Continue dialog asynchronously. This immediate isVisible() check can run before that dialog appears, causing the helper to return without force-clearing the rack membership; the following cleanup then runs with the modal still open and can leave the miner/rack placement dirty. Wait briefly for either the dialog and click it, or for the picker to close, before returning.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants