fix: delay focus intent preload and clear on blur across adapters#6744
fix: delay focus intent preload and clear on blur across adapters#6744
Conversation
📝 WalkthroughWalkthroughThis PR adds onBlur prop support to link components and refactors preload handling across React, Solid, and Vue router packages. A new enqueueIntentPreload mechanism replaces previous focus/enter logic with centralized timeout-based preload scheduling, using currentTarget for event tracking via WeakMap, and onBlur now clears pending preload timeouts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 89db2d8
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Sheraff
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react-router/src/link.tsx (2)
727-727: Module-leveltimeoutMap— verify cleanup on unmount.Since
timeoutMapis module-scoped, a pending timeout will survive component unmount and firedoPreloadfrom a stale closure. This is likely benign (just a wasted network request), but if strict cleanup is desired, consider clearing the element's entry in auseEffectcleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/link.tsx` at line 727, Module-level timeoutMap can leave timers firing after a Link unmount and calling doPreload from a stale closure; modify the Link-related component (where timeoutMap and doPreload are used) to clear and delete the element's timeout in a useEffect cleanup: when you set a timeout for an EventTarget put the timer id into timeoutMap, and in the component's cleanup clearTimeout(timeoutId) and timeoutMap.delete(element) (also ensure any event handlers that call doPreload check element presence or cancelation state) so pending timers are properly cleaned up on unmount.
663-697:enqueueIntentPreload+handleLeavelook correct and well-structured.The
WeakMapapproach avoids polluting DOM elements and naturally GCs when elements are removed. The early-return at line 666 for zero delay is a nice optimization over the Solid/Vue implementations.One very minor note: if
preloadDelayis non-zero whenenqueueIntentPreloadfires but becomes zero by the timehandleLeaveruns (e.g. router config changes dynamically), the guard at line 690 (!preloadDelay) would skip cleanup, leaving a stale timeout in the map. This is extremely unlikely in practice, but for robustness you could drop the!preloadDelayguard fromhandleLeave:Suggested tightening of handleLeave guard
const handleLeave = (e: React.MouseEvent | React.FocusEvent) => { - if (disabled || !preload || !preloadDelay) return + if (disabled || !preload) return const eventTarget = e.currentTarget const id = timeoutMap.get(eventTarget) if (id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/link.tsx` around lines 663 - 697, The cleanup in handleLeave can miss clearing timeouts if preloadDelay changes to zero between enqueueIntentPreload and handleLeave; update handleLeave to always attempt cleanup regardless of preloadDelay by removing the "!preloadDelay" guard so it checks only disabled and preload and then looks up the timer in timeoutMap (use the existing timeoutMap.get/clearTimeout/timeoutMap.delete logic), referencing the existing enqueueIntentPreload, handleLeave, timeoutMap, preloadDelay, and doPreload symbols.packages/solid-router/src/link.tsx (1)
351-363: Solid'senqueueIntentPreloadalways usessetTimeout, unlike React's zero-delay fast path.In React, when
preloadDelayis0,doPreload()fires synchronously and no timeout is set. Here in Solid,setTimeout(() => …, 0)always creates a macrotask, meaning a synchronous blur after focus can cancel a zero-delay preload. This is arguably better for the PR's goal (avoiding unnecessary preloads during fast keyboard traversal), but it's a subtle cross-adapter inconsistency.Not necessarily something to change — just flagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/src/link.tsx` around lines 351 - 363, enqueueIntentPreload currently always uses setTimeout even when preloadDelay() === 0, causing zero-delay preloads to run async; update enqueueIntentPreload so that when preloadDelay() === 0 it calls doPreload() synchronously (without creating/preparing eventTarget.preloadTimeout), otherwise keep the existing setTimeout behavior using eventTarget.preloadTimeout and clearing it to null after firing; reference enqueueIntentPreload, eventTarget.preloadTimeout, preloadDelay(), and doPreload() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-router/src/link.tsx`:
- Line 727: Module-level timeoutMap can leave timers firing after a Link unmount
and calling doPreload from a stale closure; modify the Link-related component
(where timeoutMap and doPreload are used) to clear and delete the element's
timeout in a useEffect cleanup: when you set a timeout for an EventTarget put
the timer id into timeoutMap, and in the component's cleanup
clearTimeout(timeoutId) and timeoutMap.delete(element) (also ensure any event
handlers that call doPreload check element presence or cancelation state) so
pending timers are properly cleaned up on unmount.
- Around line 663-697: The cleanup in handleLeave can miss clearing timeouts if
preloadDelay changes to zero between enqueueIntentPreload and handleLeave;
update handleLeave to always attempt cleanup regardless of preloadDelay by
removing the "!preloadDelay" guard so it checks only disabled and preload and
then looks up the timer in timeoutMap (use the existing
timeoutMap.get/clearTimeout/timeoutMap.delete logic), referencing the existing
enqueueIntentPreload, handleLeave, timeoutMap, preloadDelay, and doPreload
symbols.
In `@packages/solid-router/src/link.tsx`:
- Around line 351-363: enqueueIntentPreload currently always uses setTimeout
even when preloadDelay() === 0, causing zero-delay preloads to run async; update
enqueueIntentPreload so that when preloadDelay() === 0 it calls doPreload()
synchronously (without creating/preparing eventTarget.preloadTimeout), otherwise
keep the existing setTimeout behavior using eventTarget.preloadTimeout and
clearing it to null after firing; reference enqueueIntentPreload,
eventTarget.preloadTimeout, preloadDelay(), and doPreload() when making the
change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/vue-router/src/link.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
215-216: Document the implicit dependency on the defaultpreloadDelay.The
100is meaningful: it must be greater than the router's defaultpreloadDelay(50 ms) to avoid re-introducing the exact-delay race this change is fixing. Without a comment, future readers won't know why this value was chosen or that the assertion on line 228 will silently break if the default delay changes.✏️ Suggested annotation
fireEvent.focus(link) - await new Promise((resolve) => setTimeout(resolve, 100)) + // Wait longer than defaultPreloadDelay (50 ms) so the intent-preload + // timer fires before the click; previously 50 ms caused an exact-delay race. + await new Promise((resolve) => setTimeout(resolve, 100)) fireEvent.click(link)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/tests/store-updates-during-navigation.test.tsx` around lines 215 - 216, Add a short comment immediately above the await new Promise(...) after fireEvent.focus(link) that explains the magic 100ms value: it must be greater than the router's default preloadDelay (50ms) to avoid re-introducing the race this test guards against, and the later expect(...) in this test relies on that; note that if preloadDelay changes the test should be updated (or the test should instead reference a shared preloadDelay constant or mock the delay). This makes the dependency explicit and prevents silent breaks of the assertion that follows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/solid-router/src/link.tsx`:
- Around line 351-365: The enqueueIntentPreload handler currently runs for any
truthy preload() value, causing conflicts with other preload strategies; update
the guard at the start of enqueueIntentPreload to only allow intent-only modes
by checking that local.disabled is false and preload() is exactly true or the
string 'intent' (i.e., replace the truthy preload() check with a strict check
like preload() === true || preload() === 'intent'); locate this in the
enqueueIntentPreload function (references: enqueueIntentPreload, doPreload,
preloadDelay, LinkCurrentTargetElement) and return early for all other modes.
---
Nitpick comments:
In `@packages/react-router/tests/store-updates-during-navigation.test.tsx`:
- Around line 215-216: Add a short comment immediately above the await new
Promise(...) after fireEvent.focus(link) that explains the magic 100ms value: it
must be greater than the router's default preloadDelay (50ms) to avoid
re-introducing the race this test guards against, and the later expect(...) in
this test relies on that; note that if preloadDelay changes the test should be
updated (or the test should instead reference a shared preloadDelay constant or
mock the delay). This makes the dependency explicit and prevents silent breaks
of the assertion that follows.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/src/link.tsx
| const enqueueIntentPreload = (e: MouseEvent | FocusEvent) => { | ||
| if (local.disabled || !preload()) return | ||
| const eventTarget = (e.currentTarget || | ||
| e.target || | ||
| {}) as LinkCurrentTargetElement | ||
|
|
||
| if (eventTarget.preloadTimeout) { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| const handleTouchStart = handleFocus | ||
| eventTarget.preloadTimeout = setTimeout(() => { | ||
| eventTarget.preloadTimeout = null | ||
| doPreload() | ||
| }, preloadDelay()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for preload definition and its possible values
rg -n "preload\s*[:=]|preload\s*\(\)" packages/solid-router/src -g '*.ts' -g '*.tsx' -A 2 -B 2Repository: TanStack/router
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Look for preload type definition and enum/union values
rg -n "type.*preload|enum.*preload|'intent'|'render'|'viewport'" packages/solid-router/src -g '*.ts' -g '*.tsx' -B 2 -A 2Repository: TanStack/router
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Find how enqueueIntentPreload is called and what context it's in
rg -n "enqueueIntentPreload|handleMouseEnter|handleFocus" packages/solid-router/src/link.tsx -B 3 -A 3Repository: TanStack/router
Length of output: 980
🏁 Script executed:
#!/bin/bash
# Check the full link.tsx file to understand preload flow and other related functions
wc -l packages/solid-router/src/link.tsxRepository: TanStack/router
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Get the full preload() definition
sed -n '199,220p' packages/solid-router/src/link.tsxRepository: TanStack/router
Length of output: 710
🏁 Script executed:
#!/bin/bash
# Search for preload type or interface definition
rg -n "type.*Preload|interface.*Preload|export.*preload" packages/solid-router -g '*.ts' -g '*.tsx' -B 2 -A 4Repository: TanStack/router
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for LinkProps or link options type definition to see preload type
rg -n "preload\s*\?\s*:" packages/solid-router/src -g '*.ts' -g '*.tsx' -B 1 -A 1Repository: TanStack/router
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Search for type definitions in shared router packages
rg -n "type.*Preload|'viewport'|'render'|'intent'" packages -g '*.ts' -g '*.tsx' | head -30Repository: TanStack/router
Length of output: 2874
🏁 Script executed:
#!/bin/bash
# Look at the LinkProps interface/type definition in link.tsx
rg -n "interface.*Link|type.*Link" packages/solid-router/src/link.tsx -A 15 | head -50Repository: TanStack/router
Length of output: 2249
🏁 Script executed:
#!/bin/bash
# Search for LinkOptions type to see preload property definition
rg -n "interface LinkOptions|type LinkOptions" packages/solid-router -g '*.ts' -g '*.tsx' -A 20 | head -60Repository: TanStack/router
Length of output: 1338
🏁 Script executed:
#!/bin/bash
# Search for defaultPreload type/definition in router options
rg -n "defaultPreload|options\s*:" packages/core/src -g '*.ts' -A 5 | grep -A 5 "preload"Repository: TanStack/router
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find the core router source
find packages -name "*.ts" -type f | grep -E "(core|router)" | head -20Repository: TanStack/router
Length of output: 1416
🏁 Script executed:
#!/bin/bash
# Search in the entire packages directory for defaultPreload and preload type
rg -n "defaultPreload\s*[:\?]|preload\s*[:\?].*'viewport'|'intent'|'render'" packages -g '*.ts' -B 2 -A 2 | head -80Repository: TanStack/router
Length of output: 1778
🏁 Script executed:
#!/bin/bash
# Look at the test cases for preload behavior to understand intent-specific behavior
rg -n "enqueueIntentPreload|intent.*preload|preload.*intent" packages/solid-router/tests/link.test.tsx -B 5 -A 5 | head -100Repository: TanStack/router
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Check how handleTouchStart and enqueueIntentPreload differ in intent handling
sed -n '351,375p' packages/solid-router/src/link.tsxRepository: TanStack/router
Length of output: 681
Gate enqueueIntentPreload explicitly to intent-only modes.
The current truthy check !preload() allows this function to execute for any preload mode including 'render' and 'viewport', which conflicts with their own preload strategies. enqueueIntentPreload should only queue delayed preloads for 'intent' mode (or true, if applicable), not for modes handled elsewhere (viewport via intersection observer at line 275, render via explicit check at line 282).
Update the guard to:
if (local.disabled || (preload() !== true && preload() !== 'intent')) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-router/src/link.tsx` around lines 351 - 365, The
enqueueIntentPreload handler currently runs for any truthy preload() value,
causing conflicts with other preload strategies; update the guard at the start
of enqueueIntentPreload to only allow intent-only modes by checking that
local.disabled is false and preload() is exactly true or the string 'intent'
(i.e., replace the truthy preload() check with a strict check like preload() ===
true || preload() === 'intent'); locate this in the enqueueIntentPreload
function (references: enqueueIntentPreload, doPreload, preloadDelay,
LinkCurrentTargetElement) and return early for all other modes.
2 participants
Summary
Linkpreload behavior in React/Solid/Vue adapters sofocususes the same delayed intent-preload handler as hover/enterblurhandling to clear pending intent-preload timeouts (same cleanup path as mouse leave)touchstartas immediate preload (no delay)handleFocusalias and wireonFocusdirectly to the shared enter handlerstore-updates-during-navigationto the new baseline after delayed-focus behavior (16 -> 15) and avoid the exact-delay race (50ms -> 100mswait)Why
Keyboard navigation across many
Linkelements withpreload="intent"was triggering immediate preloads on focus, which can cause dropped frames/lag. Focus now follows the same intent delay semantics as hover, and blur now cancels pending intent preloads.Testing
CI=1 NX_DAEMON=false pnpm nx run-many --target=test:unit --projects=@tanstack/react-router,@tanstack/solid-router,@tanstack/vue-router --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run-many --target=test:types --projects=@tanstack/react-router,@tanstack/solid-router,@tanstack/vue-router --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run-many --target=test:eslint --projects=@tanstack/react-router,@tanstack/solid-router,@tanstack/vue-router --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
New Features
Tests