Skip to content

Conversation

@birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 26, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Fixed link preload event handling to correctly reference the event target.
  • New Features

    • Link transition state is now dynamically detected and exposed in render props instead of being static.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

Walkthrough

The Link component in solid-router was refactored to use e.currentTarget instead of e.target in hover handlers and to expose isTransitioning as a dynamic getter that reads from the data-transitioning attribute instead of a static false value.

Changes

Cohort / File(s) Summary
Event handling and state exposure updates
packages/solid-router/src/link.tsx
Modified handleEnter and handleLeave to reference e.currentTarget for correct element targeting; changed isTransitioning in the render-prop from a static false to a dynamic getter that reads the data-transitioning attribute from linkProps

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Primary focus: Verify that e.currentTarget correctly targets the intended element in both hover handlers
  • Secondary focus: Ensure the dynamic getter for isTransitioning correctly reflects the data-transitioning attribute state and that existing render-prop consumers can adapt to the getter pattern
  • Consider: Test that the state exposure change doesn't break type compatibility or existing component usage patterns

Poem

🐰 A hop, a fix, event targets align,
From static bounds to dynamic design,
Transitions now dance in the render-prop light,
Where currentTarget points just right! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix(solid-router): isTransitioning and handleLeave/handleEnter" directly and specifically references the three key changes in the changeset: the dynamic isTransitioning getter, and the event target fixes in the handleEnter and handleLeave functions. The title is concise, clear, and conveys the primary changes without vague language or unnecessary details. A teammate scanning the git history would immediately understand that this is a bug fix related to these specific components in the solid-router Link component.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix(solid-router)--fix-isTransitioning-and-handleEnter/handleLeave

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e557f9 and 444a02f.

📒 Files selected for processing (1)
  • packages/solid-router/src/link.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • packages/solid-router/src/link.tsx
packages/{react-router,solid-router}/**

📄 CodeRabbit inference engine (AGENTS.md)

Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/

Files:

  • packages/solid-router/src/link.tsx
🧬 Code graph analysis (1)
packages/solid-router/src/link.tsx (1)
packages/router-core/src/link.ts (1)
  • LinkCurrentTargetElement (702-704)
🔇 Additional comments (2)
packages/solid-router/src/link.tsx (2)

330-330: Correct fix: use currentTarget for event handlers.

Using e.currentTarget instead of e.target is the right approach here. When a link contains child elements (like icons or spans), e.target would reference the child element that triggered the event, causing the preloadTimeout to be stored on the wrong element. With e.currentTarget, the timeout is consistently stored on the link element itself, ensuring proper timeout management regardless of which child element is hovered.

Also applies to: 346-346


575-577: Correct fix: expose transition state dynamically.

This properly fixes the non-functional isTransitioning feature. The previous implementation (per the AI summary) returned a static false, making the feature useless. The new getter correctly reads from the data-transitioning attribute, which is set at line 431 when the isTransitioning() signal is true. This pattern is consistent with how isActive is implemented (lines 572-574) and maintains proper reactivity in SolidJS.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Oct 26, 2025

View your CI Pipeline Execution ↗ for commit 444a02f

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 8m 13s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 27s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-26 01:36:19 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 26, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5630

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5630

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5630

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5630

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5630

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5630

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5630

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5630

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5630

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5630

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5630

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5630

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5630

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5630

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5630

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5630

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5630

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5630

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5630

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5630

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5630

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5630

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5630

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@5630

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5630

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5630

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5630

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5630

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5630

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5630

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5630

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5630

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5630

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5630

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5630

commit: 444a02f

@birkskyum birkskyum merged commit e7b9041 into main Oct 26, 2025
6 checks passed
@birkskyum birkskyum deleted the fix(solid-router)--fix-isTransitioning-and-handleEnter/handleLeave branch October 26, 2025 02:15
@birkskyum birkskyum added this to the catch up solid to react milestone Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants