fix(link): resolve issue where useHref is not working correctly #5933
Conversation
🦋 Changeset detectedLatest commit: 3f8e426 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
@hasegawa-101 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReorders prop merge in link logic so provider-generated href is preserved, adds a test that exercises HeroUIProvider's useHref, and adds a patch changeset for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/link/__tests__/link.test.tsx(2 hunks)packages/components/link/src/use-link.ts(1 hunks)
⏰ 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). (6)
- GitHub Check: Prettier
- GitHub Check: Continuous Release
- GitHub Check: ESLint
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: Tests
🔇 Additional comments (2)
packages/components/link/__tests__/link.test.tsx (1)
6-6: LGTM!The import of HeroUIProvider is correct and necessary for testing provider-driven useHref behavior.
packages/components/link/src/use-link.ts (1)
113-113: The merge order change is correct and poses no risk to isExternal behavior.React Aria's useLinkProps does not auto-set rel or target attributes, so
linkPropsreturned fromuseAriaLinkwill not contain these properties. WhenisExternalis true,relandtargetare set onotherProps(lines 91-94), and with the new merge order whereotherPropsprecedeslinkProps, theisExternalvalues will apply correctly sincelinkPropshas no conflicting properties. The existing test confirms this behavior continues to work as expected.
@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: |
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
Using this PR build https://pkg.pr.new/@heroui/react@5933 on the reproducible environment seems not able to produce correct result. Could you please take a look again?
|
@wingkwong pushed the fix! Please check it again when you have a moment. |
hasegawa-101
changed the title
|
@hasegawa-101 are you using this example to test? I downloaded it to my local and tried with |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/hooks/use-aria-link/src/index.ts(2 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/hooks/use-aria-link/src/index.ts
[error] 71-71: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (4)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: ESLint
- GitHub Check: Build
🔇 Additional comments (2)
packages/hooks/use-aria-link/src/index.ts (2)
83-83: Change is consistent with the href preprocessing approach.Using
processedHrefinstead ofprops.hrefinhandleLinkClickis logically consistent with the approach taken on lines 71-72. This ensures the click handler operates on the transformed href value.However, this depends on the critical issue on line 71 being resolved first.
71-72: Verify fix effectiveness and completeness of PR changes.The available information indicates potential issues with this fix:
Tester reported issue still reproducible: According to PR comments, @wingkwong confirmed the issue persists after these changes, suggesting the fix is incomplete or ineffective.
Conditional hook violation on line 71: The code
router.useHref ? router.useHref(props.href)violates React hooks rules. Hooks must be called unconditionally in every render, but this conditionally callsrouter.useHref()only if it exists. This pattern will cause the Rules of Hooks to fail ifrouter.useHrefavailability changes between renders.Unclear scope of PR changes: The PR objectives mention the root cause is incorrect
mergePropsorder inpackages/components/link/src/use-link.ts, but we cannot confirm whether that file was actually modified in this PR or whether both files needed changes.Before approving, manually confirm:
- Whether
packages/components/link/src/use-link.tswas modified and what the originalmergePropsorder was- Whether both files require changes or if one fix alone is sufficient
- Whether the conditional hook pattern on line 71 should be refactored to avoid violating React hooks rules
- Whether the reported issue can be reproduced after these changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/link/__tests__/link.test.tsx (1)
89-100: Good test coverage for the useHref fix.The test correctly validates that the provider's
useHreffunction transforms the href attribute. The use ofgetAttribute("href")appropriately checks the attribute value rather than the resolved URL.Consider optionally enhancing this test to also verify that click navigation uses the transformed href (i.e., that the mocked
navigatefunction receives "/example/test" when the link is clicked), which would provide more comprehensive coverage of the fix described in line 82 ofpackages/hooks/use-aria-link/src/index.ts.Example enhancement:
it("should apply useHref from provider", () => { const useHref = (href: string) => `/example${href}`; + const navigate = jest.fn(); const {getByRole} = render( - <HeroUIProvider navigate={jest.fn()} useHref={useHref}> + <HeroUIProvider navigate={navigate} useHref={useHref}> <Link href="/test">Test Link</Link> </HeroUIProvider>, ); const link = getByRole("link"); expect(link.getAttribute("href")).toBe("/example/test"); + + // Optionally verify click navigation uses transformed href + // await user.click(link); + // expect(navigate).toHaveBeenCalledWith(expect.stringContaining("/example/test")); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/link/__tests__/link.test.tsx(2 hunks)packages/hooks/use-aria-link/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/link/__tests__/link.test.tsx (2)
packages/components/link/src/index.ts (1)
Link(13-13)packages/core/theme/src/components/link.ts (1)
link(111-111)
⏰ 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). (4)
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: ESLint
- GitHub Check: Continuous Release
🔇 Additional comments (2)
packages/hooks/use-aria-link/src/index.ts (1)
82-82: LGTM! Click handler now uses the provider-transformed href.The change ensures that the click handler uses
routerLinkProps.href(which includes any transformation from the provider'suseHref) instead of the originalprops.href. This aligns the navigation behavior with the rendered anchor href, fixing the issue where the provider'suseHrefwas not being respected during link clicks.packages/components/link/__tests__/link.test.tsx (1)
6-6: Import is appropriate for testing provider functionality.The HeroUIProvider import is necessary for the new test case that validates the useHref transformation from the provider.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/system/src/provider-context.ts (1)
94-97: Minor typo in the JSDoc comment.The comment contains "
href" with backticks and spaces. It should likely be eitherhref(code format) or "href" (plain text) for consistency./** - * Convert a ` href ` provided to a link component to a native `href`. + * Convert an `href` provided to a link component to a native `href`. */ useHref?: (href: Href) => string;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/link/src/use-link.ts(2 hunks)packages/core/system/src/provider-context.ts(2 hunks)packages/core/system/src/provider.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/link/src/use-link.ts (2)
packages/core/system/src/index.ts (1)
mapPropsVariants(24-24)packages/core/theme/src/components/link.ts (1)
link(111-111)
🪛 Biome (2.1.2)
packages/components/link/src/use-link.ts
[error] 49-49: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
🔇 Additional comments (6)
packages/components/link/src/use-link.ts (4)
47-50: LGTM! Proper early href transformation.The logic correctly applies the provider's
useHreftransformation when available, with appropriate null checks for both the function and the href prop. This ensures the transformed href flows through the rest of the hook.
52-52: LGTM! Using resolved props for variant mapping.Correctly uses
resolvedPropsto ensure variant mapping is based on the transformed href rather than the original.
117-117: LGTM! Correct merge order preserves transformed href.This is the key fix: by placing
linkPropslast in the merge, the href processed by React Aria (which includes the provider'suseHreftransformation) now takes precedence overotherProps, preventing the original href from overriding the transformed value. This directly addresses the root cause described in the PR objectives.
47-50: Dismiss static analysis false positive.The Biome warning about "hook being called conditionally" is a false positive. The code is calling
globalContext.useHref(...), which is a regular function property, not a React hook. The conditional checkglobalContext?.useHref && originalProps.hrefis simply null-safety for calling the function, which is perfectly valid. The linter is misidentifying this as a hook call due to the "use" prefix in the name.packages/core/system/src/provider.tsx (2)
42-49: LGTM! Clear documentation for useHref prop.The JSDoc clearly explains the purpose of
useHrefand how it complements thenavigatefunction, providing good context for users of the provider.
56-56: LGTM! Complete wiring of useHref through provider.The implementation correctly:
- Destructures
useHreffrom props (line 56)- Passes it to React Aria's
RouterProvider(line 74)- Includes it in the provider context value (line 93)
- Adds it to the memoization dependency array (line 104)
This properly propagates
useHrefto all consumers, enabling the fix inuse-link.ts.Also applies to: 74-74, 93-93, 104-104
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/link/__tests__/link.test.tsx(1 hunks)
⏰ 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). (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
|
@hasegawa-101 lemme know once it is ready for review again. make sure you've tested it on that example first. |
|
@wingkwong Thanks. I've verified it in the codesandobox environment. Please review it again 🙏 https://codesandbox.io/p/github/hasegawa-101/sparkling-frog/main |
|
@hasegawa-101 the sandbox is not running 
|
|
@wingkwong 2025-11-25.16.15.27.mov |
|
Thanks for the feedback. I've reviewed the react-spectrum implementation as suggested. You are correct that Currently, If we aim to align with the react-spectrum implementation, I believe we would need to require I'd love to hear your thoughts on this 🙏 |
|
@hasegawa-101 thanks for the findings. Lemme take a look when I got more time later. |
|
I also checked their implementation and compared ours. The expected behaviour is that However, from those 2 linked issues, they also include |
2 participants
📝 Description
This PR fixes a bug in the Link component where the useHref function provided by the HeroUIProvider was not being correctly applied.
##⛳️ Current behavior (updates)
Currently, in the useLink hook, the props objects are merged in an incorrect order. Specifically, otherProps (containing the original href) is merged after linkProps (containing the href transformed by useHref). This causes the transformed href to be overridden by the original one, resulting in the useHref functionality not working as expected.
##🚀 New behavior
The order of arguments in the mergeProps function within the useLink hook (packages/components/link/src/use-link.ts) has been corrected. By merging linkProps last, we ensure that the correctly transformed href from the useHref function takes precedence and is applied to the final rendered tag. This resolves the bug and makes the Link component respect the useHref prop from the provider.
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.