-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(devtools): Handle some additional query devtools accessibility findings #9961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(devtools): Handle some additional query devtools accessibility findings #9961
Conversation
🦋 Changeset detectedLatest commit: 2f4bdba The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughBumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 2f4bdba
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9961 +/- ##
===========================================
- Coverage 45.89% 20.91% -24.99%
===========================================
Files 200 42 -158
Lines 8437 2458 -5979
Branches 1942 637 -1305
===========================================
- Hits 3872 514 -3358
+ Misses 4116 1697 -2419
+ Partials 449 247 -202
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-devtools/src/Devtools.tsx (1)
592-651: Excellent keyboard accessibility implementation for panel resizing.The separator role with proper ARIA attributes and keyboard navigation is well done. The direction-aware logic correctly handles all four panel positions.
Consider adding
aria-valueminandaria-valuemaxattributes to provide screen readers with the full value range context, aligning with the minHeight/minWidth constraints already defined.aria-valuenow={ position() === 'top' || position() === 'bottom' ? Number(props.localStore.height || DEFAULT_HEIGHT) : Number(props.localStore.width || DEFAULT_WIDTH) } + aria-valuemin={ + position() === 'top' || position() === 'bottom' + ? convertRemToPixels(3.5) + : convertRemToPixels(12) + } tabindex="0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/silver-peas-end.md(1 hunks)packages/query-devtools/src/Devtools.tsx(20 hunks)
🔇 Additional comments (11)
.changeset/silver-peas-end.md (1)
1-5: LGTM!Minor version bump is appropriate for these accessibility enhancements since they add new functionality (keyboard navigation, ARIA attributes) without breaking changes.
packages/query-devtools/src/Devtools.tsx (10)
420-425: Good accessibility practice: auto-focusing close button on panel mount.This ensures keyboard and screen reader users have immediate focus context when the devtools panel opens.
857-865: LGTM!Clear and descriptive aria-label for the view toggle radio group.
931-957: LGTM!Appropriate aria-labels for both query and mutation sort selectors.
1280-1286: LGTM!The aria-label clearly describes the purpose of this setting control.
1476-1493: Good pattern: comprehensive aria-label with hidden decorative indicators.The aria-label correctly includes disabled/static states, and
aria-hidden="true"on the visual indicators prevents redundant announcements.
2013-2019: LGTM!Using
role="heading"witharia-level="2"provides proper document structure for assistive technologies navigating the Query Details section.
2249-2252: LGTM!Clear aria-label for the error type selector.
2317-2324: LGTM!The aria-label "Edit query data as JSON" clearly communicates both the purpose and expected format.
3006-3014: Good focus-visible implementation for the drag handle.The
:focus-visibleoutline ensures keyboard users see a clear focus indicator while avoiding focus rings on mouse interactions. The background color change on:focusprovides a fallback visual cue.
2445-2451: LGTM!Consistent heading semantics applied to MutationDetails sections, matching the QueryDetails pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-devtools/src/Devtools.tsx (1)
485-495: Bug:removeEventListeneruses'mouseUp'instead of'mouseup', leaving a dangling listenerIn the drag resize logic, the cleanup unsubscribes
mousemovecorrectly but attempts to remove a'mouseUp'listener, while the event was added as'mouseup'. This means themouseuplistener will never be removed.const unsubscribe = () => { if (isResizing()) { setIsResizing(false) } document.removeEventListener('mousemove', runDrag, false) - document.removeEventListener('mouseUp', unsubscribe, false) + document.removeEventListener('mouseup', unsubscribe, false) } document.addEventListener('mousemove', runDrag, false) document.addEventListener('mouseup', unsubscribe, false)Fixing the event name prevents unnecessary residual listeners and potential subtle bugs in long‑lived devtools sessions.
🧹 Nitpick comments (4)
packages/query-devtools/src/Devtools.tsx (4)
420-425: Close button auto‑focus on mount looks good, but consider a small guardFocusing the close button on mount is a solid accessibility improvement for keyboard and screen‑reader users. To be extra defensive against any lifecycle edge cases, you could guard the ref before calling
focus().// Focus the close button when the panel opens for screen reader accessibility onMount(() => { - closeBtnRef.focus() + closeBtnRef && closeBtnRef.focus() })This doesn’t change behavior in the normal case but avoids a possible runtime error if the ref isn’t set for any reason.
Also applies to: 654-662
592-605: Keyboard-resizable separator and focus styles are well done; consider adding value range ARIAThe separator + keyboard resizing logic lines up correctly with the panel position, and the focus/
focus-visiblestyles on the drag handle provide a clear visible focus indication without double outlines. This is a nice accessibility win.One improvement to make the control more self‑describing to assistive tech: expose the minimum (and optionally maximum) sizes via ARIA so
aria-valuenowisn’t “floating” without context.For example:
<div role="separator" aria-orientation={ position() === 'top' || position() === 'bottom' ? 'horizontal' : 'vertical' } aria-label="Resize devtools panel" aria-valuenow={ position() === 'top' || position() === 'bottom' ? Number(props.localStore.height || DEFAULT_HEIGHT) : Number(props.localStore.width || DEFAULT_WIDTH) } + aria-valuemin={ + position() === 'top' || position() === 'bottom' + ? convertRemToPixels(3.5) + : convertRemToPixels(12) + } + // Optionally expose a max value too, if you enforce one + // aria-valuemax={...} tabindex="0"This keeps the current UX but improves screen‑reader output about the control’s range.
Also applies to: 612-651, 3004-3012
1476-1476: Query row ARIA label and hidden indicators are a good pattern; consider a more human‑friendly keyEncoding the disabled/static state once in the button
aria-labeland marking the visual “disabled/static” indicators asaria-hidden="true"is a clean way to avoid redundant announcements.Depending on how readable
queryHashtends to be, you might want to expose a more human‑friendly description (e.g., derived fromqueryKey) in the label, and possibly fall back to the hash:- aria-label={`Query key ${props.query.queryHash}${isDisabled() ? ', disabled' : ''}${isStatic() ? ', static' : ''}`} + aria-label={`Query ${ + displayValue(props.query.queryKey, true) || props.query.queryHash + }${isDisabled() ? ', disabled' : ''}${isStatic() ? ', static' : ''}`}This will make screen‑reader output less cryptic when query hashes are long or opaque.
Also applies to: 1485-1487, 1490-1492
2013-2019: Section headings and live status regions are structured well; minor ARIA polish possibleUsing
<div role="heading" aria-level="2">for the various “Query Details”, “Actions”, “Data Explorer”, “Query Explorer”, and mutation detail sections gives assistive tech a clear document outline without disturbing existing layout/styles. Likewise, marking the status chips withrole="status"andaria-live="polite"will help announce state changes non‑intrusively.Small optional polish:
role="status"already impliesaria-live="polite, so you can drop the explicitaria-liveif you’d like to reduce redundancy, though it’s not harmful to keep.No required changes here.
Also applies to: 2032-2034, 2050-2055, 2271-2277, 2356-2360, 2445-2450, 2470-2472, 2487-2492, 2506-2511, 2525-2530, 2544-2549
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-devtools/src/Devtools.tsx(21 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/Devtools.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/query-devtools/src/Devtools.tsx (1)
861-861: ARIA labels on toggles, selects, and textarea are clear and descriptiveThe added
aria-labels for:
- the view toggle radio group (“Toggle between queries and mutations view”),
- sort
<select>s (“Sort queries by” / “Sort mutations by”),- the “Hide disabled queries” radio group,
- the error‑type
<select>,- and the JSON edit
<textarea>,are all specific and accurately describe the control purpose. This should meaningfully improve screen‑reader navigation without changing visual behavior.
No changes requested here.
Also applies to: 935-935, 948-948, 1282-1283, 2250-2250, 2318-2318
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-devtools/src/Devtools.tsx (1)
593-656: Consider addingaria-valuemaxfor complete range information.The ARIA attributes and keyboard navigation implementation look excellent. However, the separator is missing an
aria-valuemaxattribute. While the maximum is dynamic (viewport size), you could set it to a reasonable approximation (e.g., 90% of viewport) or update it dynamically. This would help screen reader users understand the full resize range.Consider adding:
aria-valuenow={ position() === 'top' || position() === 'bottom' ? Number(props.localStore.height || DEFAULT_HEIGHT) : Number(props.localStore.width || DEFAULT_WIDTH) } + aria-valuemax={ + position() === 'top' || position() === 'bottom' + ? typeof window !== 'undefined' ? Math.floor(window.innerHeight * 0.9) : undefined + : typeof window !== 'undefined' ? Math.floor(window.innerWidth * 0.9) : undefined + } tabindex="0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-devtools/src/Devtools.tsx(29 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/Devtools.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (11)
packages/query-devtools/src/Devtools.tsx (11)
420-425: Good accessibility enhancement with a minor consideration.Auto-focusing the close button when the panel opens helps keyboard and screen reader users quickly discover the panel and have a clear exit point. However, be aware this moves focus away from where the user was previously interacting, which could be disruptive in some workflows (e.g., if they triggered the panel via keyboard shortcut while focused on application content).
865-865: LGTM!The aria-label clearly describes the purpose of the radio group toggle.
939-939: LGTM!Clear aria-labels for the sort select elements, properly differentiated for queries vs mutations.
Also applies to: 953-953
1287-1287: LGTM!Descriptive aria-label for the disabled queries visibility setting.
1481-1481: Excellent accessibility pattern!The aria-label enhancement to include disabled and static states, combined with marking the visual indicators as
aria-hidden="true", is the correct approach. This prevents duplicate announcements while ensuring screen reader users receive all relevant information.Also applies to: 1490-1497
1786-1786: LGTM!Proper use of aria-label to convey status information and aria-hidden to suppress the decorative visual indicator.
Also applies to: 1818-1818
2020-2024: LGTM!Proper use of
role="heading"witharia-level="2"to provide semantic document structure for assistive technologies without changing the visual hierarchy. This significantly improves navigation for screen reader users.Also applies to: 2056-2060, 2284-2288, 2369-2373
2039-2040: LGTM!Correct use of
role="status"witharia-live="polite"for status indicators. The "polite" setting appropriately ensures status changes are announced without interrupting other screen reader output.Also applies to: 2484-2485
2080-2081: LGTM!Proper use of
aria-hidden="true"on decorative status indicators. The button labels already convey the action, so hiding the visual dots prevents redundant announcements.Also applies to: 2106-2106, 2131-2131, 2157-2157, 2207-2207, 2237-2237, 2256-2256
2332-2332: LGTM!The textarea aria-label clearly describes its purpose. The heading structure in MutationDetails consistently follows the same accessible pattern as QueryDetails, providing proper semantic navigation for screen reader users.
Also applies to: 2458-2561
3018-3026: Review focus management pattern.The
:focus-visibleimplementation is good and provides proper keyboard focus indication. However, the plain:focusrule (lines 3018-3021) removes the outline, which could cause accessibility issues in browsers that don't fully support:focus-visible. While the background color change provides some visual feedback, relying solely on it may not meet WCAG contrast requirements in all cases.Consider whether the plain
:focusrule is necessary, or if you can rely on default browser behavior +:focus-visibleoverride for better accessibility coverage.Since this is in the chill mode review and the background color change provides some visual feedback, this is more of a "good to verify" than a critical issue.
| } | ||
| document.removeEventListener('mousemove', runDrag, false) | ||
| document.removeEventListener('mouseUp', unsubscribe, false) | ||
| document.removeEventListener('mouseup', unsubscribe, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was suggested by coderabbit - it's outside of the current PR's scope, but figured it worth updating while here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-devtools/src/Devtools.tsx (2)
593-609: Consider addingaria-valuemaxfor completeness.The separator ARIA attributes are well-implemented. However,
aria-valuemaxis typically paired witharia-valuenowandaria-valueminto give screen reader users a complete picture of the resize range.You could derive it from viewport dimensions or use a reasonable maximum (e.g., 90% of viewport as suggested by the CSS
max-height: 90%).🔎 Suggested addition
aria-valuenow={ position() === 'top' || position() === 'bottom' ? Number(props.localStore.height || DEFAULT_HEIGHT) : Number(props.localStore.width || DEFAULT_WIDTH) } + aria-valuemax={ + position() === 'top' || position() === 'bottom' + ? Math.floor(window.innerHeight * 0.9) + : Math.floor(window.innerWidth * 0.9) + } tabindex="0"
2020-2026: Consider using native<h2>elements instead ofrole="heading".While
role="heading"witharia-level="2"is valid ARIA, native semantic elements like<h2>are generally preferred as they provide better browser/AT compatibility and don't require additional ARIA attributes.🔎 Suggested change
- <div - role="heading" - aria-level="2" + <h2 class={cx(styles().detailsHeader, 'tsqd-query-details-header')} > Query Details - </div> + </h2>Apply similar changes to other heading divs in this component.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-devtools/src/Devtools.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/Devtools.tsx
🧬 Code graph analysis (1)
packages/query-devtools/src/Devtools.tsx (3)
packages/query-devtools/src/utils.tsx (2)
convertRemToPixels(148-150)displayValue(84-88)packages/query-devtools/src/constants.ts (2)
DEFAULT_HEIGHT(12-12)DEFAULT_WIDTH(14-14)packages/query-core/src/query.ts (2)
isDisabled(274-283)isStatic(285-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (15)
packages/query-devtools/src/Devtools.tsx (15)
420-425: Good accessibility pattern for focus management.Auto-focusing the close button when the panel opens is a solid approach for screen reader users, ensuring they land in a predictable location within the panel.
Consider whether this behavior might be unexpected for sighted keyboard users who may prefer focus to remain where it was, especially if the panel opens while they're interacting with something else. This is a minor UX consideration rather than a blocking issue.
490-490: Good fix for event listener cleanup.DOM event names are case-sensitive;
'mouseUp'would fail to remove the listener added with'mouseup'. This corrects a potential memory leak and duplicate handler issue.
617-656: Well-implemented keyboard resizing logic.The arrow key handling correctly adjusts direction based on panel position, providing intuitive keyboard navigation. The minimum size constraints properly mirror the mouse drag behavior.
862-890: Good ARIA labeling for the view toggle.The
aria-labelclearly describes the purpose of the radio group for screen reader users.
936-961: Clear ARIA labels for sort selects.Both sort select elements now have descriptive
aria-labelattributes that clearly communicate their purpose.
1285-1290: Appropriate ARIA label for settings radio group.The label clearly identifies the purpose of this setting control.
1481-1481: Enhanced query row accessibility.Including disabled and static states in the
aria-labelprovides screen reader users with complete context about each query's status.
1489-1498: Correct use ofaria-hiddenfor redundant indicators.Since the disabled/static states are already included in the button's
aria-label, hiding these visual indicators from assistive technology avoids duplicate announcements.
1786-1786: Clear status button labeling.The
aria-labelformat of"{Label}: {Count}"provides concise, meaningful information to screen reader users.
1817-1828: Appropriate hiding of decorative status dot.The colored dot is purely visual; the status information is conveyed through the label text, so hiding it from AT is correct.
2037-2043: Good use of live region for status updates.This ensures screen readers announce status changes. Note that
role="status"implicitly includesaria-live="polite", so the explicit attribute is technically redundant but doesn't cause issues.
2080-2086: Correct hiding of decorative action indicators.The colored indicator spans in action buttons are purely visual; the button text conveys the action, so hiding these from AT is appropriate.
2263-2265: Clear label for error type select.The
aria-labelclearly describes the purpose of this select element.
2330-2337: Appropriate label for data editor textarea.The
aria-label="Edit query data as JSON"provides clear context for the textarea's purpose.
3018-3026: Good focus styles for keyboard accessibility.The
:focus-visiblestyling provides a clear visual indicator for keyboard users while:focushandles mouse interactions gracefully with a background color change.
🎯 Changes
This pull request introduces significant accessibility improvements to the TanStack Query Devtools panel. The changes mainly enhance keyboard navigation, screen reader support, and semantic markup for better usability and compliance with accessibility standards.
Accessibility improvements for screen readers and keyboard navigation:
aria-hiddenwhere appropriate, preventing redundant or confusing information for screen readers. [1] [2] [3] [4] [5] [6] [7] [8](References: [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25] [26] [27] [28] [29]
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.