fix(tabs): responsive resize cursor to match selected tab #5846
Conversation
🦋 Changeset detectedLatest commit: f314565 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
@adbjo is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReworks Tabs cursor to a DOM-ref-driven implementation: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Tabs as Tabs component
participant TabEl as Selected tab element (DOM)
participant Cursor as cursorRef (span)
participant RO as ResizeObserver
participant rAF as requestAnimationFrame
Tabs->>Cursor: render and attach ref (conditionally)
Tabs->>TabEl: locate selected tab element
Tabs->>RO: observe TabEl (on mount/selection)
Note over TabEl,RO: layout or selection changes
RO->>Tabs: notify on resize
Tabs->>Tabs: updateCursorPosition(selectedTab)
Tabs->>TabEl: getBoundingClientRect()
Tabs->>Tabs: getCursorStyles(tabRect, variant, isVertical)
Tabs->>Cursor: apply inline styles (left/top/width/height)
Tabs->>rAF: schedule setting data-[initialized]=true / data-[animated]
rAF->>Cursor: set data attributes to enable CSS transitions
Note over Cursor: CSS transitions run only after data attributes set
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential focus areas:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/components/tabs/src/tabs.tsx (4)
53-72: Consider making magic numbers configurable or documented.The hardcoded values for the underlined variant (10% left offset, 80% width, 2px bottom offset) work but could be fragile if the design evolves. Consider extracting these as constants with descriptive names or making them configurable through props/theme.
Example refactor:
+const UNDERLINED_CURSOR_INSET = 0.1; // 10% horizontal inset +const UNDERLINED_CURSOR_BOTTOM_OFFSET = 2; // px from bottom + const getCursorStyles = useCallback( (tabRect: DOMRect) => { if (variant === "underlined") { return { - left: `${tabRect.left + tabRect.width * 0.1}px`, - top: `${tabRect.top + tabRect.height - 2}px`, - width: `${tabRect.width * 0.8}px`, + left: `${tabRect.left + tabRect.width * UNDERLINED_CURSOR_INSET}px`, + top: `${tabRect.top + tabRect.height - UNDERLINED_CURSOR_BOTTOM_OFFSET}px`, + width: `${tabRect.width * (1 - 2 * UNDERLINED_CURSOR_INSET)}px`, height: "", }; }
84-89: Type casting to DOMRect is misleading.The manually constructed object is missing many DOMRect properties (x, y, right, bottom, toJSON, etc.). While it works because
getCursorStylesonly uses the provided properties, theas DOMRectcast is technically incorrect and could be confusing.Consider using a more accurate type:
+ type TabRect = Pick<DOMRect, 'width' | 'height' | 'left' | 'top'>; + const tabRect = { width: selectedTab.offsetWidth, height: selectedTab.offsetHeight, left: selectedTab.offsetLeft, top: selectedTab.offsetTop, - } as DOMRect; + } as TabRect;And update
getCursorStylessignature:-const getCursorStyles = useCallback( - (tabRect: DOMRect) => { +const getCursorStyles = useCallback( + (tabRect: TabRect) => {
98-98: Add explanatory comment for requestAnimationFrame usage.The
requestAnimationFrameensures the data-initialized attribute is set after the initial styles are applied, preventing unwanted animation on first render. This pattern could benefit from a brief comment for future maintainers.+ // Set initialized flag after paint to prevent animating from default position requestAnimationFrame(() => cursorRef.current?.setAttribute("data-initialized", "true"));
108-112: Consider reusing ResizeObserver for better performance.Creating a new
ResizeObserverinstance on everyselectedKeychange works but could be optimized. Consider maintaining a single observer and updating what it observes, or verify if the current approach causes any performance issues with rapid tab switching.Example optimization (if needed):
useEffect(() => { const observer = new ResizeObserver(updateCursorPosition); return () => observer.disconnect(); }, [updateCursorPosition]); useEffect(() => { const selectedTab = domRef.current?.querySelector(`[data-key="${selectedKey}"]`); if (!selectedTab || !observerRef.current) return; observerRef.current.disconnect(); observerRef.current.observe(selectedTab); }, [domRef, selectedKey]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/swift-kiwis-knock.md(1 hunks)packages/components/tabs/src/tabs.tsx(3 hunks)
🔇 Additional comments (5)
.changeset/swift-kiwis-knock.md (1)
1-5: LGTM!The changeset correctly documents a minor version bump with an appropriate description that aligns with the feature enhancement.
packages/components/tabs/src/tabs.tsx (4)
4-4: LGTM!The additional React hooks (
useEffect,useCallback) are appropriately imported for the new cursor positioning logic.
47-47: LGTM!The
cursorRefis properly declared to manage the cursor element.
121-127: Excellent solution for preventing initial animation flash!The className logic using
[&:not([data-initialized])]modifiers effectively hides the cursor and disables transitions until it's properly positioned, preventing the visual issue of animating from a default position on initialization. This directly addresses one of the key objectives mentioned in the PR description.
103-113: No action required—data-key attribute is reliably set on Tab components.The
data-keyattribute used in the selector is explicitly set in the Tab component (packages/components/tabs/src/tab.tsx: data-key={key}), confirming the querySelector will work correctly. The useEffect hook dependencies and ResizeObserver cleanup logic are sound.
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Before: After:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/tabs/src/tabs.tsx (1)
80-85: Consider defining a proper type for tab rectangle.The object literal on lines 80-85 only has four properties but is cast
as DOMRect. This type assertion bypasses type checking, sinceDOMRectincludes many additional properties (x, y, right, bottom, toJSON, etc.). While this works becausegetCursorStylesonly accesses these four properties, it's a type safety smell.Consider defining a custom type:
+type TabRect = { + width: number; + height: number; + left: number; + top: number; +}; + -const getCursorStyles = useCallback( - (tabRect: DOMRect) => { +const getCursorStyles = useCallback( + (tabRect: TabRect) => { // ... }, [variant], ); // ... const updateCursorPosition = useCallback( (selectedTab: HTMLElement) => { if (!cursorRef.current) return; const tabRect = { width: selectedTab.offsetWidth, height: selectedTab.offsetHeight, left: selectedTab.offsetLeft, top: selectedTab.offsetTop, - } as DOMRect; + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/tabs/src/tabs.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
📚 Learning: 2025-10-25T17:11:59.319Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
Applied to files:
packages/components/tabs/src/tabs.tsx
📚 Learning: 2025-10-25T17:08:46.274Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:156-157
Timestamp: 2025-10-25T17:08:46.274Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes `variant` and `isVertical` to prevent potential side-effects, even though they might appear redundant based on static analysis.
Applied to files:
packages/components/tabs/src/tabs.tsx
🔇 Additional comments (7)
packages/components/tabs/src/tabs.tsx (7)
4-4: LGTM!The new imports and ref declarations are appropriate for the ref-driven cursor positioning approach.
Also applies to: 48-48, 51-53
55-74: LGTM!The cursor positioning math is correct. For the underlined variant, the 10% offset and 80% width properly center the cursor with margins on each side.
103-113: LGTM!The
contentRectcheck correctly guards against positioning the cursor for tabs that haven't rendered content yet. Based on learnings, this approach was intentionally chosen to handle tabs with only padding/borders but no content.
115-125: LGTM!The effect correctly wires the ResizeObserver to the selected tab, with proper cleanup to prevent memory leaks. When
selectedKeychanges, the observer is recreated for the new tab, and the initial observation fires immediately to position the cursor.
133-133: LGTM!The
cursorRefis correctly attached to the cursor span, enabling direct style updates without triggering React re-renders.
167-167: Dependency array consistency maintained.As per learnings,
cursorRefis intentionally included to maintain consistency with howdomRefis handled in dependency arrays, even though ref objects have stable identity across renders.
168-169: Dependency array safety maintained.As per learnings,
variantandisVerticalare intentionally included to prevent potential side-effects, even though static analysis might suggest they're redundant.
adbjo
force-pushed
the
fix/tabs_resize_cursor_with_selected_tab
branch
from
879c3ac to
8f16d30
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/tabs/src/tabs.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
📚 Learning: 2025-10-25T17:11:59.319Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
Applied to files:
packages/components/tabs/src/tabs.tsx
📚 Learning: 2025-10-25T17:08:46.274Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:156-157
Timestamp: 2025-10-25T17:08:46.274Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes `variant` and `isVertical` to prevent potential side-effects, even though they might appear redundant based on static analysis.
Applied to files:
packages/components/tabs/src/tabs.tsx
🔇 Additional comments (7)
packages/components/tabs/src/tabs.tsx (7)
4-4: LGTM!The added imports are necessary for the new cursor positioning implementation.
48-53: LGTM!The ref declarations and state extractions are clean and necessary for the cursor positioning logic.
55-74: LGTM!The cursor style calculation correctly handles both underlined and other variants, with appropriate memoization.
103-113: LGTM!The ResizeObserver callback correctly handles tab content resizing and is properly memoized.
133-133: LGTM!Correctly attaches the cursor ref to enable the new positioning logic.
167-169: LGTM!The dependency array correctly includes
cursorReffor consistency with the existingdomRefpattern, as well asvariantandisVerticalto guard against potential side-effects.Based on learnings
98-98: Add null check for unmount safety.The
requestAnimationFramecallback doesn't verify thatcursorRef.currentstill exists before callingsetAttribute. If the component unmounts between scheduling and execution, this could cause an error.Apply this diff:
- requestAnimationFrame(() => cursorRef.current?.setAttribute("data-initialized", "true")); + requestAnimationFrame(() => { + if (cursorRef.current) { + cursorRef.current.setAttribute("data-initialized", "true"); + } + });Likely an incorrect or invalid review comment.
Toggling isVertical had same issue as variant, fixed it now too |
adbjo
force-pushed
the
fix/tabs_resize_cursor_with_selected_tab
branch
from
80769a3 to
d2f97e0
Compare
Also saw that there was unwanted blinking when toggling between variants and isVertical. Added another fix for this, final final fix |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/tabs/src/tabs.tsx (2)
56-75: Consider extracting magic numbers for the underlined variant positioning.The calculations on lines 60-62 use magic numbers (0.1, 0.8, -2) that could be more maintainable as named constants. This would make the positioning logic clearer and easier to adjust.
Example:
+ const UNDERLINED_INSET_RATIO = 0.1; + const UNDERLINED_WIDTH_RATIO = 0.8; + const UNDERLINED_VERTICAL_OFFSET = 2; + const getCursorStyles = useCallback( (tabRect: DOMRect) => { if (variant === "underlined") { return { - left: `${tabRect.left + tabRect.width * 0.1}px`, - top: `${tabRect.top + tabRect.height - 2}px`, - width: `${tabRect.width * 0.8}px`, + left: `${tabRect.left + tabRect.width * UNDERLINED_INSET_RATIO}px`, + top: `${tabRect.top + tabRect.height - UNDERLINED_VERTICAL_OFFSET}px`, + width: `${tabRect.width * UNDERLINED_WIDTH_RATIO}px`, height: "", }; }
93-114: Consider improving type safety for the tabRect object.Line 102 asserts a plain object as
DOMRect, which is not fully type-safe. While this works at runtime since only the four properties are used, it would be clearer to define a custom type or use a partial type.+type TabRect = { + width: number; + height: number; + left: number; + top: number; +}; + const updateCursorPosition = useCallback( (selectedTab: HTMLElement) => { if (!cursorRef.current) return; const tabRect = { width: selectedTab.offsetWidth, height: selectedTab.offsetHeight, left: selectedTab.offsetLeft, top: selectedTab.offsetTop, - } as DOMRect; + } as TabRect; const styles = getCursorStyles(tabRect);Also update the
getCursorStylessignature to acceptTabRectinstead ofDOMRect.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/tabs/src/tabs.tsx(3 hunks)packages/core/theme/src/components/tabs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/theme/src/components/tabs.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.272Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:115-125
Timestamp: 2025-10-27T21:52:33.304Z
Learning: In packages/components/tabs/src/tabs.tsx, the useEffect dependency array at line 125 intentionally uses `domRef.current` rather than `domRef` because domRef.current can change between renders (when React sets it during the commit phase), whereas domRef itself has stable identity and won't change.
📚 Learning: 2025-10-27T21:48:35.272Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.272Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Applied to files:
packages/components/tabs/src/tabs.tsx
📚 Learning: 2025-10-25T17:11:59.319Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.319Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
Applied to files:
packages/components/tabs/src/tabs.tsx
📚 Learning: 2025-10-25T17:08:46.274Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:156-157
Timestamp: 2025-10-25T17:08:46.274Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes `variant` and `isVertical` to prevent potential side-effects, even though they might appear redundant based on static analysis.
Applied to files:
packages/components/tabs/src/tabs.tsx
📚 Learning: 2025-10-27T21:52:33.304Z
Learnt from: adbjo
PR: heroui-inc/heroui#5846
File: packages/components/tabs/src/tabs.tsx:115-125
Timestamp: 2025-10-27T21:52:33.304Z
Learning: In packages/components/tabs/src/tabs.tsx, the useEffect dependency array at line 125 intentionally uses `domRef.current` rather than `domRef` because domRef.current can change between renders (when React sets it during the commit phase), whereas domRef itself has stable identity and won't change.
Applied to files:
packages/components/tabs/src/tabs.tsx
🔇 Additional comments (5)
packages/components/tabs/src/tabs.tsx (5)
4-4: LGTM!The additional React hooks imports are necessary and correctly added for the new cursor positioning and animation logic.
47-54: LGTM!The ref declarations and state tracking setup are well-structured. The
previousVariantandpreviousIsVerticalrefs enable proper animation reset detection, and the cursor ref enables direct DOM manipulation for performant positioning.
77-91: LGTM!The animation reset wrapper correctly prevents transition animations when variant or orientation changes. Using
requestAnimationFrameto re-add the attributes ensures proper timing after layout updates.
116-138: LGTM!The ResizeObserver implementation correctly handles cursor repositioning when the selected tab's dimensions change. The effect properly observes the currently selected tab and cleans up the observer on unmount or when the selection changes.
The contentRect check on line 121 correctly guards against positioning updates before the tab has rendered content, as discussed in previous reviews.
Based on learnings
140-184: LGTM!The cursor span correctly receives the
cursorReffor DOM manipulation, and the dependency array includes all necessary values. The conditional rendering ensures the cursor only appears when animations are enabled and a tab is selected.Based on learnings
2 participants
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
See description :)
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
Performance
UX
Refactor