Skip to content

fix(client): hide no-pools callout on the Pools settings null state#95

Merged
rongxin-liu merged 3 commits intomainfrom
fix/protoos-pools-null-state-callout
Apr 26, 2026
Merged

fix(client): hide no-pools callout on the Pools settings null state#95
rongxin-liu merged 3 commits intomainfrom
fix/protoos-pools-null-state-callout

Conversation

@rongxin-liu
Copy link
Copy Markdown
Contributor

@rongxin-liu rongxin-liu commented Apr 26, 2026

Summary

  • Fixes bug(protoOS): "No mining pools configured" alert duplicates the Pools page null state #94. In Proto OS, the global "No mining pools configured." alert banner stacks on top of the Pools settings page when no pools are configured — the page already shows its own null-state card with the same CTA, so the banner duplicates the empty-state prompt.
  • Centralized the visibility logic in a new pathname-aware helper (getNoPoolsCalloutState) and routed both layout-level consumers (App, KpiLayout) through it. The helper now derives the Pools settings match from settingsRouteMetadata.miningPools.path via matchPath, including the embedded /miners/:id/... route, so it stays aligned with router changes. The banner is suppressed only when the Pools page is in its null state; if pools are configured but every one is offline, the banner stays (that's a real connectivity error, not a null state).

Test plan

  • npx vitest run src/protoOS/components/App/App.test.tsx src/protoOS/components/NoPoolsCallout/utility.test.ts — added a utility-level matrix and three App-level integration tests; 29/29 passing.
  • npx tsc --noEmit from client/.
  • npx prettier --check on the touched files.
  • npx eslint --max-warnings 0 on the touched files.
  • Manual check on a Proto OS device: with no pools configured, navigate to Settings → Pools and confirm only the page's own null state is shown; navigate to any other page and confirm the global banner still appears.
  • Manual check on the Pools page with at least one configured-but-offline pool — banner should still surface there.

Made with Cursor

The global "No mining pools configured" banner is rendered above every
authenticated route in Proto OS. On the Pools settings page itself, the
page already shows its own null-state card with the same CTA, so the
banner stacks on top of it and duplicates the empty-state prompt.

Centralize the visibility logic in a pathname-aware helper and route
both layout consumers (App, KpiLayout) through it so the banner is
suppressed only when the Pools page is in its null state. The banner
still renders on the Pools page when pools are configured but offline,
since that's a real connectivity error rather than a null state.

Closes #94

Made-with: Cursor
Copilot AI review requested due to automatic review settings April 26, 2026 00:49
@rongxin-liu rongxin-liu requested a review from a team as a code owner April 26, 2026 00:49
@github-actions github-actions Bot added javascript Pull requests that update javascript code client labels Apr 26, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 26, 2026

🔐 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 (7465f46c8b936e7c93d2816646752f00fca177ed...66fdd4d88440ddbcaae28a747b057677267686ad, exact PR three-dot diff)
  • Model: gpt-5.4

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


Review Summary

Overall Risk: LOW

Findings

No concrete findings in the reviewed diff. The change is limited to ProtoOS frontend banner-routing logic and associated tests, and I did not identify a security, correctness, or reliability regression in the changed hunks.

Notes

  • Review scope was limited to .git/codex-review.diff, which only touched:
    client/src/protoOS/components/App/App.tsx,
    client/src/protoOS/components/App/App.test.tsx,
    client/src/protoOS/components/NoPoolsCallout/utility.ts,
    client/src/protoOS/components/NoPoolsCallout/utility.test.ts,
    and client/src/protoOS/features/kpis/components/KpiLayout/KpiLayout.tsx.
  • I did not see changes in backend auth/JWT handling, gRPC/Connect handlers, SQL, plugin boundaries, command execution, infrastructure, or pool/wallet mutation paths.
  • The new getNoPoolsCalloutState() helper is consistent with the mining-pools page’s own empty-state predicate: both treat a pool as “configured” based on a truthy url, which avoids an obvious mismatch between page null-state rendering and global callout suppression.
  • I could not run the targeted Vitest files in this environment because pnpm is not installed, so this is a source-inspection review rather than an executed-test verification.

Generated by Codex Security Review |
Triggered by: @rongxin-liu |
Review workflow run

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a ProtoOS UI duplication where the global “No mining pools configured.” banner appeared on the Pools settings page even when that page is already showing its own null-state card and CTA.

Changes:

  • Introduced a centralized, pathname-aware helper (getNoPoolsCalloutState) to compute when the NoPools callout should be shown.
  • Updated the two layout-level callout renderers (App, KpiLayout) to use the shared helper and suppress the banner only on /settings/mining-pools when pools are truly unconfigured.
  • Added unit tests for the helper plus App-level integration tests for the key visibility scenarios.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
client/src/protoOS/features/kpis/components/KpiLayout/KpiLayout.tsx Uses the shared helper (including pathname) to decide whether to render the NoPools callout.
client/src/protoOS/components/NoPoolsCallout/utility.ts New helper that centralizes pool live/configured detection and mining-pools-route suppression logic.
client/src/protoOS/components/NoPoolsCallout/utility.test.ts Adds a pathname + pool-state matrix to verify helper behavior.
client/src/protoOS/components/App/App.tsx Routes App-level callout rendering through the shared helper and suppresses on Pools null state.
client/src/protoOS/components/App/App.test.tsx Adds integration tests validating the banner suppression/visibility across routes and pool states.

Comment thread client/src/protoOS/components/NoPoolsCallout/utility.ts
Copy link
Copy Markdown
Collaborator

@mcharles-square mcharles-square left a comment

Choose a reason for hiding this comment

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

call out on using matched path seems valid

Derive the Pools settings route match from shared route metadata so the NoPools callout suppression stays aligned with router changes.

Made-with: Cursor
@rongxin-liu rongxin-liu merged commit 1b59a0a into main Apr 26, 2026
29 checks passed
@rongxin-liu rongxin-liu deleted the fix/protoos-pools-null-state-callout branch April 26, 2026 06:24
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.

bug(protoOS): "No mining pools configured" alert duplicates the Pools page null state

3 participants