Conversation
🦋 Changeset detectedLatest commit: 1020489 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMigrates imperative DOM lookups (querySelector*/querySelectorAll) to React ref registries across RovingFocusGroup, Tree, Slider, and NavigationMenu; components now register element refs in context and resolve focus/interaction targets from those registries instead of global selectors. Changes
Sequence Diagram(s)sequenceDiagram
participant TreeItem as "TreeItem\n(creates ref)"
participant TreeRoot as "TreeRoot\n(context)"
participant Roving as "RovingFocusGroup\n(context)"
participant User as "User\n(key/navigation)"
rect rgba(200,230,255,0.5)
TreeItem->>TreeRoot: registerItemRef(id, ref)
TreeRoot->>Roving: expose itemRefs via context
Roving->>Roving: recompute focusItems from itemRefs (sort by document order)
end
rect rgba(220,255,220,0.5)
User->>Roving: key navigation (e.g., ArrowDown)
Roving->>Roving: determine target id from focusItems
Roving->>TreeItem: itemRefs.get(id).current.focus()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kotAPI
reviewed
Apr 4, 2026
| @@ -0,0 +1,264 @@ | |||
| # Components Using querySelector That Can Be Replaced with Refs | |||
Collaborator
There was a problem hiding this comment.
Let this file just be a rule of why we should use refs instead of dom queries, these will go out of date and don't really help us in the long run
| @@ -0,0 +1,161 @@ | |||
| # querySelector to Refs Refactoring - Complete | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
We can just use one file, we can delete this
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/Tree/fragments/TreeItem.tsx (1)
145-160:⚠️ Potential issue | 🟠 MajorDon't use
item.labelas the React key for children — provide a persistent unique identifier.Line 155 keys children by
subItem.label, but labels are not reliable React keys if they're non-unique or mutable. With the ref registry keyed by instance identity (useId), a label collision or change will cause React to reuse the wrongTreeItemcomponent, breaking the ref lookups in focus management (e.g., line 79 when collapsing to find the parent ref). If tree items don't have a built-in unique ID, add one to the data model or construct a stable key from multiple fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Tree/fragments/TreeItem.tsx` around lines 145 - 160, The children are keyed by mutable/non-unique subItem.label which breaks component identity and ref lookups (affecting TreeItem, handleFirstChildRef and collapse/focus logic); change the key on the mapped TreeItem to a stable unique identifier (prefer an explicit id property like subItem.id) and fall back to a deterministic composite key if no id exists (for example combine the parent's id and the child index or another immutable field) so React preserves instance identity for the ref registry and focus management.
🧹 Nitpick comments (1)
src/components/ui/Slider/fragments/SliderThumb.tsx (1)
22-36: Register the thumb ref during commit instead of inuseEffect.
SliderRootaccessesthumbRefsArray.current[0]synchronously on the initial pointerdown event to focus the first thumb in single-value mode. With the current code, this registry is populated only afteruseEffectruns (post-paint). Moving the registration into themergedRefcallback ensures the thumb is available during the commit phase, before any user interaction can occur.♻️ Suggested change
const mergedRef = React.useMemo(() => { return (node: HTMLDivElement | null) => { (thumbRef as any).current = node; + if (registerThumbRef && node) { + registerThumbRef(index, thumbRef); + } if (typeof ref === 'function') ref(node); else if (ref) (ref as any).current = node; }; - }, [ref]); - - // Register this thumb ref with the slider root - React.useEffect(() => { - if (registerThumbRef && thumbRef.current) { - registerThumbRef(index, thumbRef); - } - }, [index, registerThumbRef]); + }, [index, ref, registerThumbRef]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Slider/fragments/SliderThumb.tsx` around lines 22 - 36, The thumb registration must happen during commit, not in useEffect: move the registerThumbRef(index, thumbRef) call into the mergedRef callback so the slider root sees thumbRefsArray synchronously; update the mergedRef function (currently setting (thumbRef as any).current and forwarding ref) to also call registerThumbRef(index, thumbRef) when thumbRef becomes non-null, and remove the separate React.useEffect that registers the thumb; keep references to mergedRef, thumbRef, registerThumbRef and index when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knowledge/querySelector-to-refs/refactoring-summary.md`:
- Around line 126-159: Update the "Remaining Work" and "Known Issues" sections
in refactoring-summary.md to match the completed refactor: remove or mark
resolved the entries for RovingFocusItem.tsx, RovingFocusGroup.tsx, and
NavigationMenuContent.tsx (or explicitly scope the doc to the earlier
tree/slider phase if you want to keep historical notes), change the Portal
Components note to reflect that theme container lookup now uses ThemeContext
refs only, and resolve the "Vertical Slider Visual Inversion" entry by adding
the investigation outcome (tested on main branch and whether it was
pre-existing, CSS-related, or a calculation bug) plus any follow-up action.
In `@src/components/ui/NavigationMenu/fragments/NavigationMenuContent.tsx`:
- Around line 21-22: Restore an explicit "focus-on-open" step that runs when the
menu's open state becomes true, but drive it through the roving-focus registry
instead of using querySelector; in practice, add an effect that watches the menu
open prop and the RovingFocusGroup's focusedItemId and calls the
RovingFocusGroup/RovingFocusItem registry focus helper (use the component ref or
registry API exposed by RovingFocusGroup — e.g.,
rovingGroupRef.focus(focusedItemId) or a focusFirst() helper) to move DOM focus
into the content when opened, rather than invoking element.querySelector(...).
Ensure the effect uses the existing RovingFocusGroup ref/registry and cleans up
appropriately.
In `@src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx`:
- Around line 62-75: The tree-mode focus list currently uses
Array.from(itemRefsMap.current.keys()) which preserves registration order and
causes focus order to drift; update addFocusItem (and unregisterItemRef) so that
when SHOULD_RECOMPUTE_FOCUS_ITEMS is true you derive focusItems from the current
visible DOM order instead of insertion order: iterate over the rendered
container (e.g., querySelectorAll for focusable item elements or map
itemRefsMap.current to existing DOM nodes and sort by compareDocumentPosition)
to build the ordered array, call setFocusItems with that ordered list, and
ensure unregisterItemRef also removes the id from focusItems via setFocusItems
to avoid leaving stale IDs. Use the existing symbols itemRefsMap, addFocusItem,
unregisterItemRef, setFocusItems, and SHOULD_RECOMPUTE_FOCUS_ITEMS to locate
where to apply these changes.
---
Outside diff comments:
In `@src/components/ui/Tree/fragments/TreeItem.tsx`:
- Around line 145-160: The children are keyed by mutable/non-unique
subItem.label which breaks component identity and ref lookups (affecting
TreeItem, handleFirstChildRef and collapse/focus logic); change the key on the
mapped TreeItem to a stable unique identifier (prefer an explicit id property
like subItem.id) and fall back to a deterministic composite key if no id exists
(for example combine the parent's id and the child index or another immutable
field) so React preserves instance identity for the ref registry and focus
management.
---
Nitpick comments:
In `@src/components/ui/Slider/fragments/SliderThumb.tsx`:
- Around line 22-36: The thumb registration must happen during commit, not in
useEffect: move the registerThumbRef(index, thumbRef) call into the mergedRef
callback so the slider root sees thumbRefsArray synchronously; update the
mergedRef function (currently setting (thumbRef as any).current and forwarding
ref) to also call registerThumbRef(index, thumbRef) when thumbRef becomes
non-null, and remove the separate React.useEffect that registers the thumb; keep
references to mergedRef, thumbRef, registerThumbRef and index when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ad6f4be-6543-4011-a675-c3bccb6a3153
📒 Files selected for processing (14)
knowledge/querySelector-to-refs/components-analysis.mdknowledge/querySelector-to-refs/refactoring-complete.mdknowledge/querySelector-to-refs/refactoring-summary.mdsrc/components/ui/NavigationMenu/fragments/NavigationMenuContent.tsxsrc/components/ui/Slider/context/SliderContext.tsxsrc/components/ui/Slider/fragments/SliderRangeSlider.tsxsrc/components/ui/Slider/fragments/SliderRoot.tsxsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/components/ui/Tree/contexts/TreeContext.tsxsrc/components/ui/Tree/fragments/TreeItem.tsxsrc/components/ui/Tree/fragments/TreeRoot.tsxsrc/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsxsrc/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsxsrc/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
Comment on lines
+21
to
+22
| // RovingFocusGroup automatically handles focus management | ||
| // No need for manual querySelector and focus |
Contributor
There was a problem hiding this comment.
Opening the menu no longer moves DOM focus into the content.
RovingFocusGroup only initializes focusedItemId; the actual .focus() call still happens later from RovingFocusItem navigation helpers. After removing the old effect, opening the menu leaves focus on the trigger/body, so keyboard users can’t start interacting with the content immediately. Keep an explicit focus-on-open step, but drive it through the roving-focus ref registry instead of querySelector.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/NavigationMenu/fragments/NavigationMenuContent.tsx` around
lines 21 - 22, Restore an explicit "focus-on-open" step that runs when the
menu's open state becomes true, but drive it through the roving-focus registry
instead of using querySelector; in practice, add an effect that watches the menu
open prop and the RovingFocusGroup's focusedItemId and calls the
RovingFocusGroup/RovingFocusItem registry focus helper (use the component ref or
registry API exposed by RovingFocusGroup — e.g.,
rovingGroupRef.focus(focusedItemId) or a focusFirst() helper) to move DOM focus
into the content when opened, rather than invoking element.querySelector(...).
Ensure the effect uses the existing RovingFocusGroup ref/registry and cleans up
appropriately.
Collaborator
Author
There was a problem hiding this comment.
WIP very early comp
Contributor
There was a problem hiding this comment.
@GoldGroove06, understood — thanks for the heads-up! I'll leave this comment open as a reminder. When you're ready to finalize this component, the key thing to address is restoring an explicit focus-on-open step (without reverting to querySelector) — for example, by exposing a focusFirst() helper or similar API from RovingFocusGroup and calling it in an effect that watches itemOpen. Happy to help work through the implementation when you get there. 🐇
🧠 Learnings used
Learnt from: GoldGroove06
Repo: rad-ui/ui PR: 1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx`:
- Around line 62-69: unregisterItemRef currently removes the id from itemRefsMap
and focusItems but doesn't clear the provider's focusedItemId, leaving a
dangling focus target; update the unregisterItemRef callback to also check the
current focusedItemId and, if it equals the unregistered id, call the focused-id
setter (e.g. setFocusedItemId(null) or setFocusedItemId(undefined) as used in
this module) to clear it, while retaining the existing removal of focusItems and
respecting SHOULD_RECOMPUTE_FOCUS_ITEMS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19f0a632-65dc-4004-97ce-69a98fab80a3
📒 Files selected for processing (5)
.changeset/chatty-showers-repeat.mdknowledge/querySelector-to-refs/components-analysis.mdsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsxsrc/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
✅ Files skipped from review due to trivial changes (3)
- .changeset/chatty-showers-repeat.md
- src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
- knowledge/querySelector-to-refs/components-analysis.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Slider/fragments/SliderThumb.tsx
Comment on lines
+62
to
+69
| const unregisterItemRef = React.useCallback((id: string) => { | ||
| itemRefsMap.current.delete(id); | ||
|
|
||
| // Remove from focusItems to avoid stale IDs | ||
| if (SHOULD_RECOMPUTE_FOCUS_ITEMS) { | ||
| setFocusItems((prev) => prev.filter((itemId) => itemId !== id)); | ||
| } | ||
| }, [SHOULD_RECOMPUTE_FOCUS_ITEMS]); |
Contributor
There was a problem hiding this comment.
Clear focusedItemId when the active item unregisters.
Lines 65-68 prune focusItems, but the provider can still point at the removed id. That leaves the group without a valid roving target after a focused item is collapsed or unmounted.
💡 Proposed fix
const unregisterItemRef = React.useCallback((id: string) => {
itemRefsMap.current.delete(id);
+ setFocusedItemId((prev) => (prev === id ? null : prev));
// Remove from focusItems to avoid stale IDs
if (SHOULD_RECOMPUTE_FOCUS_ITEMS) {
setFocusItems((prev) => prev.filter((itemId) => itemId !== id));
}
}, [SHOULD_RECOMPUTE_FOCUS_ITEMS]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx` around lines
62 - 69, unregisterItemRef currently removes the id from itemRefsMap and
focusItems but doesn't clear the provider's focusedItemId, leaving a dangling
focus target; update the unregisterItemRef callback to also check the current
focusedItemId and, if it equals the unregistered id, call the focused-id setter
(e.g. setFocusedItemId(null) or setFocusedItemId(undefined) as used in this
module) to clear it, while retaining the existing removal of focusItems and
respecting SHOULD_RECOMPUTE_FOCUS_ITEMS.
kotAPI
approved these changes
Apr 4, 2026
kotAPI
added
the
automerge
Contributor
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1727
Summary by CodeRabbit
Refactor
Documentation