Skip to content

refactor(miner-ui): adopt shared StateBoundary in the portfolio route - #6588

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/miner-ui-portfolio-stateboundary-v2
Jul 16, 2026
Merged

refactor(miner-ui): adopt shared StateBoundary in the portfolio route#6588
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/miner-ui-portfolio-stateboundary-v2

Conversation

@luciferlive112116

Copy link
Copy Markdown
Contributor

Summary

Both components in this route hand-rolled the same loading/error/empty triple as literal <p> tags, and the loading branch was a flat sentence re-rendered on every 10s poll. This swaps both onto the ui-kit's StateBoundary plus its Skeleton primitive — which this app already depends on but had never imported.

Prerequisite confirmed before starting, as the issue requires: the state-views.tsx port has landed — @loopover/ui-kit/components/state-views exports StateBoundary/LoadingState/ErrorState/EmptyState. I import the real component from the package; nothing is forked, copied, or reached into from apps/loopover-ui.

Design decisions

  • Each async flow keeps its OWN boundary, deliberately. The summary read and the queue-actions read are independent fetches; one shared boundary would let a queue-actions failure blank the summary, or a summary failure hide the actions.

  • Skeletons are shaped like the real content — three status cards over table rows, and rows for the actions table — so the layout settles when the poll lands instead of jumping. A single generic bar would just move the jump later.

  • Every user-visible sentence is preserved exactly, character for character. This is the one part worth scrutinising, because the obvious way to adopt this primitive gets it wrong. StateBoundary wants a title + a description, which invites splitting each existing sentence across the two — a rewrite, however reasonable, of copy the issue says not to reword. Instead each whole original sentence is passed as the title and the description is suppressed, so the rendered text is identical to the <p> it replaces. The suppression is not symmetric, and the difference matters:

    • EmptyState has no default description and Shell renders {description && …}, so emptyDescription={null} renders nothing.
    • ErrorState resolves description ?? <default>, so null would restore its default copy — it needs errorDescription="" to stay suppressed.

    The result: none of StateBoundary's own boilerplate ("This view has no records to show.", "Something went wrong fetching this data.") reaches the user. ErrorState emits role="alert" itself, so failures keep announcing exactly as the hand-rolled <p role="alert"> did.

Frozen underneath — the part worth checking

Per the issue, the action surface is behaviourally untouched, and the diff proves it:

  • lib/portfolio-queue.ts, lib/portfolio-queue-actions.ts, lib/use-polled-fetch.tsunchanged (git diff against main for src/lib/ is empty).
  • The Release/Requeue Buttons and their onClick/disabled={pending} wiring to onRelease/onRequeuereleaseItem/requeueItem — unchanged. Only the chrome around them moved.

The strongest evidence is the tests: every pre-existing assertion in both suites passes unmodified, including the release/requeue POST-wiring tests, the pending-disable test, and the #6090 regression (a failing release renders the error without a false re-fetch). Those untouched tests are what prove the action surface really didn't move.

Tests

The two loading assertions had to change, because StateBoundary renders loadingSkeleton ?? <LoadingState> — with a skeleton supplied, the literal "Loading local portfolio queue…" / "Loading actionable queue items…" text is intentionally gone. Both now assert the skeleton placeholder instead, exactly as the issue directs.

Beyond that, each of the four boundary branches is now pinned to its exact rendered sentence, since preserving that copy is the main risk in this change and a loose regex would not have caught a reworded split:

Suite Branch Asserts
portfolio-queue.test.tsx loading skeleton present, real table not yet rendered
empty the exact sentence, and no This view has no records to show
error the exact sentence, and no Something went wrong fetching this data
portfolio-queue-actions.test.tsx loading skeleton present, real table not yet rendered
empty (new) the exact sentence, and no boilerplate — this branch had no test at all before
error the exact sentence (existing assertion, unmodified)

Validation

  • Both portfolio suites — 36/36 pass, with every pre-existing assertion unmodified.
  • Whole miner-ui app — 223/223 pass via npm --workspace @loopover/ui-miner run test (vitest run --coverage), no coverage-threshold failure. That's the operative local gate.
  • eslint0 errors on my three files. The 3 remaining warnings (react-refresh/only-export-components, two react-hooks/exhaustive-deps) are pre-existing on main, not introduced here. prettier --write applied.
  • git diff --check clean; the tree contains only the three permitted files (no generated routeTree.gen.ts churn). Rebased on latest main — no base conflict.

One pre-existing failure, not mine: ui:typecheck reports vite-chat-api.ts(43,34) TS2352. I verified it fails identically on clean main with my work stashed.

Coverage

Not scored by codecov/patch: apps/loopover-miner-ui sits under apps/**, which codecov.yml's ignore: list excludes (Codecov collects only src/**, packages/loopover-engine/src/**, packages/loopover-miner/lib/**). Flagging that explicitly so a reviewer isn't confused by the absent check — the app's own local coverage floor above is the real gate, and it's green.

Scope

  • Exactly the three files the issue permits: routes/portfolio.tsx, portfolio-queue.test.tsx, portfolio-queue-actions.test.tsx. run-history.tsx, ledgers.tsx, index.tsx, __root.tsx untouched — they're separate issues.
  • No new component library, no ad-hoc CSS, no new design tokens — only existing @loopover/ui-kit primitives.
  • No secrets; no changelog, site/, CNAME, or lovable changes.

Safety

  • Rendering-only: no fetcher, endpoint, poll-cadence, or button semantic changes, so there is exactly one write path into the portfolio queue (the existing one) both before and after.

Closes #6511

…JSONbored#6511)

Replace the hand-rolled loading/error/empty <p> tags in PortfolioQueueView and
PortfolioQueueActionsSection with the ui-kit StateBoundary, plus content-shaped
Skeleton placeholders so the layout does not jump when the 10s poll lands.

Each user-visible sentence is passed as the whole EmptyState/ErrorState title with
the description suppressed, so the rendered copy is unchanged from the <p> tags it
replaces. Each fetch keeps its own boundary: the summary and queue-actions reads are
independent, so one failing must not blank the other.

Closes JSONbored#6511
@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 16:13:43 UTC

3 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR replaces hand-rolled loading/error/empty `<p>` blocks in the portfolio route's two components with the shared `StateBoundary` + `Skeleton` primitives, following the exact pattern already established in `routes/index.tsx` (#6509). The tricky part — passing the whole original sentence as `title` while suppressing `description` differently for `ErrorState` (`errorDescription=""`, since it falls back via `??`) versus `EmptyState` (`emptyDescription={null}`, which has no default) — is correctly reasoned and verified by tests asserting byte-identical copy and the absence of `StateBoundary`'s own boilerplate. Tests were updated in lockstep to assert skeleton test-ids and exact sentence text instead of the old loose regexes, and each async flow correctly keeps its own boundary so one fetch's failure can't blank the other's UI.

Nits — 4 non-blocking
  • `PortfolioQueueView`'s body grew to ~62 lines in one function (portfolio.tsx:64) per the size-smell flag — consider extracting the summary cards/table JSX into a small subcomponent the way `Stat`/`SummaryCard` were factored out in routes/index.tsx.
  • The PR description doesn't state which issue this closes in the diff itself; confirm Redesign: Portfolio route — StateBoundary + skeletons, keep release/requeue actions intact #6511 is the linked issue since the brief shows only partial coverage of it.
  • Factor the repeated `result !== null && !result.ok` / `summary !== null` null-narrowing checks (used 3–4 times each per component) into a local `const isError = …` / `const isEmpty = …` for readability, mirroring how `summary` is already destructured once at the top of `PortfolioQueueView`.
  • Consider whether `QueueActionsSkeleton`/`PortfolioQueueSkeleton` could share a small row-skeleton helper with `CardStatsSkeleton` in routes/index.tsx to avoid near-duplicate skeleton row markup across routes.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6511
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 159 registered-repo PR(s), 96 merged, 31 issue(s).
Contributor context ✅ Confirmed Gittensor contributor luciferlive112116; Gittensor profile; 159 PR(s), 31 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff ports StateBoundary/Skeleton usage into both PortfolioQueueView and PortfolioQueueActionsSection with separate boundaries per fetch, preserves exact user-visible copy and role="alert" semantics, leaves the release/requeue Button call sites and fetchers/poll cadence untouched, only modifies the three files the issue permits, and updates (rather than deletes) the loading-state tests as requ

Review context
  • Author: luciferlive112116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 159 PR(s), 31 issue(s).
  • Related work: Titles/paths share 8 meaningful terms. (issue #6510, issue #6511)
  • Related work: Titles/paths share 8 meaningful terms. (issue #6511, issue #6512)
Contributor next steps
  • Start here: Review top overlaps.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/portfolio desktop before /portfolio
before /portfolio
after /portfolio
after /portfolio
/portfolio mobile before /portfolio (mobile)
before /portfolio (mobile)
after /portfolio (mobile)
after /portfolio (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot 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.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 32d0ef8 into JSONbored:main Jul 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redesign: Portfolio route — StateBoundary + skeletons, keep release/requeue actions intact

1 participant