Skip to content

fix(ui): return focus to Compare trigger when SubnetCompareDrawer closes#6512

Closed
thomasalvaedison7777-lgtm wants to merge 2 commits into
JSONbored:mainfrom
thomasalvaedison7777-lgtm:fix/ui-subnet-compare-drawer-focus-6420
Closed

fix(ui): return focus to Compare trigger when SubnetCompareDrawer closes#6512
thomasalvaedison7777-lgtm wants to merge 2 commits into
JSONbored:mainfrom
thomasalvaedison7777-lgtm:fix/ui-subnet-compare-drawer-focus-6420

Conversation

@thomasalvaedison7777-lgtm

Copy link
Copy Markdown
Contributor

Summary

  • Wire SubnetCompareDrawer's "Compare" button through <SheetTrigger asChild> so the Sheet owns the trigger (it was a plain sibling with a manual onClick).
  • Closing via Escape/X now returns focus to the Compare button, matching the other Sheet focus-return fixes in this batch.

Closes #6420

Test plan

  • Open a subnet detail page (e.g. /subnets/1)
  • Tab to / click Compare to open the drawer
  • Close with Escape — focus returns to Compare
  • Re-open, close with the X button — focus returns to Compare
  • Confirm drawer open/close and peer compare still work as before

Notes

A11y/focus behavior only — no visual layout change, so no before/after screenshot table.

…ation

Updated the SubnetCompareDrawer component to utilize the Sheet component from the UI kit, improving the structure and accessibility of the comparison interface. Added SheetTrigger for the compare button and streamlined the form layout for better user experience. The input field and buttons were reorganized for clarity, and the clear button functionality was preserved. This refactor enhances the overall usability of the subnet comparison feature.
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.85%. Comparing base (ff38590) to head (b10a9e8).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6512   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files         174      174           
  Lines       22196    22196           
  Branches     8798     8798           
=======================================
  Hits        21721    21721           
  Misses         81       81           
  Partials      394      394           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Updated the description text in the SubnetCompareDrawer component to improve readability by adjusting line breaks. This change enhances the user experience when comparing subnets.
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix or unsolicited PR — scores a 0.05x multiplier. label Jul 17, 2026
@loopover-orb

loopover-orb Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-17 13:23:38 UTC

1 file · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): apps/ui/src/components/metagraphed/subnet-compare-drawer.tsx (matched apps/ui/**).

Review summary
This is a clean, minimal a11y fix: it moves the plain "Compare" button inside `<SheetTrigger asChild>` so Radix's Sheet owns and manages the trigger element, which is the standard pattern for automatic focus-return on close (Escape/X) — matching the sibling fix in stake-unstake-modal.tsx-style components. The refactor is purely structural (indentation/JSX nesting), the button's props/classes/children and all form/CompareBody logic are untouched, and the diff correctly imports `SheetTrigger` from `@​jsonbored/ui-kit`. The `ui` CI failure is not explained by anything visible in this diff and should be checked, but nothing in the file content suggests a defect this change itself introduces.

Nits — 7 non-blocking
  • No test coverage was added for the focus-return behavior; consider an interaction test asserting focus moves back to the Compare trigger after Escape/X close, consistent with the pattern used elsewhere in this batch.
  • The external brief's magic-number/label-control flags on lines unrelated to this diff's actual changes (the `1024` and the `htmlFor`/`label` pairing both predate this PR) don't reflect new regressions here.
  • The `ui` CI job is marked FAILED; worth confirming it isn't related to this change (e.g., a snapshot/test expecting the old sibling-button DOM structure) even though the source diff itself looks correct.
  • Add a focus-management test (e.g. Testing Library) that opens the drawer, closes via Escape and via the X button, and asserts `document.activeElement` is the Compare trigger.
  • Since the PR notes this is one of several similar Sheet focus-return fixes, confirm the other fixed components share the same `SheetTrigger asChild` pattern for consistency.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ⚠️ Gate result — Not blocking (Advisory; not blocking this PR.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6420
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: 21 registered-repo PR(s), 13 merged, 1 issue(s).
Contributor context ✅ Confirmed Gittensor contributor thomasalvaedison7777-lgtm; Gittensor profile; 21 PR(s), 1 issue(s).
Linked issue satisfaction

Addressed
The diff wraps the Compare button in <SheetTrigger asChild> and moves it inside the Sheet component, exactly matching the issue's requested fix, which should restore native focus-return behavior on close.

Review context
  • Author: thomasalvaedison7777-lgtm
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, TypeScript
  • Official Gittensor activity: 21 PR(s), 1 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.

🟩 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 17, 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 · Light, Desktop · Dark, Tablet · Light, Tablet · Dark, Mobile · Light, Mobile · Dark.

Please resubmit with the remaining rows filled in.

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

@loopover-orb loopover-orb Bot closed this Jul 17, 2026
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 or unsolicited PR — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SubnetCompareDrawer doesn't return focus to its trigger button on close

1 participant