Skip to content

fix(ui-kit): edge-trigger StateBoundary failure notifications#7502

Closed
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/state-boundary-edge-notify-7436
Closed

fix(ui-kit): edge-trigger StateBoundary failure notifications#7502
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/state-boundary-edge-notify-7436

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Make StateBoundary's onFailureNotify effect edge-triggered (false→true of isError && errorLabel) via a wasFailureNotifyActive ref, matching the documented "flips into" intent.
  • Add a regression test that re-renders with fresh callback identities while still in error (exactly one notify), then leaves and re-enters error (second notify).

Closes #7436

Test plan

  • npm run build --workspace @loopover/ui-kit
  • npm run test --workspace @loopover/ui -- src/components/site/state-views.test.tsx (15 pass)
  • git diff --check
  • CI green

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 20, 2026 13:35
@github-actions
github-actions Bot had a problem deploying to preview/pr-7502 July 20, 2026 13:37 Failure
Closes JSONbored#7436

Co-authored-by: Cursor <cursoragent@cursor.com>
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-20 13:42:22 UTC

2 files · 1 AI reviewer · no blockers · readiness 95/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
Fixes StateBoundary's onFailureNotify effect to be edge-triggered via a wasFailureNotifyActive ref, correctly matching the documented 'flips into' intent and preventing re-notification when app wrappers pass fresh callback identities each render while still errored. The logic is sound: the ref tracks the previous active state, and the effect only fires notify on false→true transitions while still updating the ref every run regardless of firing. The added test exercises the real regression path (stale error + fresh callbacks, then leave/re-enter) and matches production usage shown in message-list.tsx and similar StateBoundary callers.

Nits — 3 non-blocking
  • The non-null assertion `errorLabel!` at state-views.tsx:277 works because `failureNotifyActive` already narrowed via `Boolean(isError && errorLabel)`, but TypeScript can't see that through the boolean coercion — consider `if (isError && errorLabel && !wasFailureNotifyActive.current)` to keep the type narrowing without the assertion.
  • The lengthy inline comment block at state-views.tsx:268-272 duplicates rationale that would fit better as a shorter comment plus a link to the issue/PR discussion.
  • Consider extracting the edge-trigger ref pattern into a small `usePrevious`/`useEdgeTrigger` hook if this pattern (referenced as also used by mcp-version-badge) recurs elsewhere in the codebase, to avoid duplicated boilerplate.

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 #7436
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low 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: 370 registered-repo PR(s), 171 merged, 34 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 370 PR(s), 34 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The StateBoundary effect now tracks previous error state via a wasFailureNotifyActive ref and only invokes onFailureNotify on the false→true transition, matching the edge-triggered pattern requested; the wrapper is left untouched per the issue's explicit instruction. A regression test in state-views.test.tsx re-renders with fresh callback identities while isError stays true (asserting exactly one

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 370 PR(s), 34 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

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

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 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 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required viewport × theme combination needs its own before/after image pair in a labeled table row (e.g. "Desktop · Light | before | after"). Still missing: Desktop · Dark, Tablet · Dark, Mobile · Dark.

Please resubmit with the remaining rows filled in.

See https://github.com/JSONbored/loopover/blob/main/.claude/skills/contributing-to-loopover/SKILL.md for the exact format and examples. This is an automated maintenance action.

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

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ui-kit): StateBoundary re-notifies on every re-render while isError is true, not once per transition

1 participant