Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the exported LinkCurrentTargetElement type; replaced per-element preload timeouts with a module-level WeakMap debounce in solid- and vue-router adapters; tightened react-router to only preload when preload === 'intent'; added/updated tests for intent vs non-intent preload behavior. Changes
Sequence Diagram(s)mermaid User->>LinkComponent: focus / hover / touchstart Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 135abf1
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/solid-router/tests/link.test.tsx (1)
4484-4486: Misleading test name for theundefinedcase.
test.eachstringifies values, soundefinedrenders as'Link.preload="undefined"'in the test output. This implies thepreloadprop is explicitly set to"undefined", whereas the test actually omits the prop entirely (via{}spread). A label override makes the intent clear:✏️ Proposed fix
- test.each([undefined, false, 'render', 'viewport'] as const)( - 'Link.preload="%s" should not preload on focus, hover, or touchstart', + test.each([ + [undefined, 'not set'], + [false, 'false'], + ['render', 'render'], + ['viewport', 'viewport'], + ] as const)( + 'Link.preload="%s" should not preload on focus, hover, or touchstart', + async (preloadMode, _label) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/tests/link.test.tsx` around lines 4484 - 4486, The test title for test.each([undefined, false, 'render', 'viewport']...) misleadingly shows Link.preload="undefined" for the case where the prop is omitted; update the test data to include explicit labels so the case where preload is omitted is shown as e.g. "no prop" (or "omitted") instead of "undefined" — modify the invocation of test.each used with the test name 'Link.preload="%s" should not preload on focus, hover, or touchstart' to supply tuples of [value, label] or otherwise provide a custom label for the undefined case (referencing the existing test.each and the preloadMode variable/Link.preload mention) so the test output reflects the intent.
🤖 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/tests/link.test.tsx`:
- Around line 4535-4537: The test uses waitFor around a negative assertion on
preloadRouteSpy which can false-pass because waitFor resolves immediately if the
assertion is already true; change the pattern to first await sleep(...) to drain
timers (consistent with other tests) then perform a direct synchronous
expect(preloadRouteSpy).toHaveBeenCalledTimes(baselineCalls) so asynchronous
preloads scheduled with defaultPreloadDelay: 0 are allowed to run before the
assertion; update the assertion code that currently references waitFor and
preloadRouteSpy accordingly.
In `@packages/vue-router/src/link.tsx`:
- Around line 362-390: The intent preload should bypass scheduling when
preloadDelay.value === 0 so events fire immediately and cannot be cancelled by
handleLeave; in enqueueIntentPreload (and where delay-based scheduling happens)
add an early branch: if preloadDelay.value === 0 then call doPreload() and
return (do not touch timeoutMap or setTimeout). Keep existing guards
(options.disabled, preload.value !== 'intent') but ensure zero-delay fast path
avoids storing timeouts so quick focus/hover/touch can't be deduped/cancelled.
---
Nitpick comments:
In `@packages/solid-router/tests/link.test.tsx`:
- Around line 4484-4486: The test title for test.each([undefined, false,
'render', 'viewport']...) misleadingly shows Link.preload="undefined" for the
case where the prop is omitted; update the test data to include explicit labels
so the case where preload is omitted is shown as e.g. "no prop" (or "omitted")
instead of "undefined" — modify the invocation of test.each used with the test
name 'Link.preload="%s" should not preload on focus, hover, or touchstart' to
supply tuples of [value, label] or otherwise provide a custom label for the
undefined case (referencing the existing test.each and the preloadMode
variable/Link.preload mention) so the test output reflects the intent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-router/src/link.tsxpackages/react-router/tests/link.test.tsxpackages/solid-router/src/link.tsxpackages/solid-router/tests/link.test.tsxpackages/vue-router/src/link.tsxpackages/vue-router/tests/link.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vue-router/tests/link.test.tsx (2)
4563-4613: No coverage forhandleLeavedebounce cancellation — the core behavioral change of this PR.The PR replaces per-element
preloadTimeoutproperties with a module-levelWeakMap-basedtimeoutMap. The most important invariant of that change is that amouseleaveorblurevent cancels a pending preload timer before it fires. The intent test only validates the positive path (events trigger preloads); it never verifies that leaving the element before the timer fires suppresses the preload.A minimal coverage addition, e.g.:
// Verify that mouseleave cancels a pending intent preload const router2 = createRouter({ routeTree: rootRoute.addChildren([aboutRoute, indexRoute]), defaultPreload: false, defaultPreloadDelay: 50, // non-zero so we can interleave leave before fire history, }) vi.useFakeTimers() const spy2 = vi.spyOn(router2, 'preloadRoute') render(<RouterProvider router={router2} />) const link2 = await screen.findByRole('link', { name: 'About Link' }) fireEvent.mouseOver(link2) // schedules timer fireEvent.mouseLeave(link2) // should cancel it vi.runAllTimers() expect(spy2).not.toHaveBeenCalled() vi.useRealTimers()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/tests/link.test.tsx` around lines 4563 - 4613, The test currently only asserts positive preload calls but misses verifying that handleLeave cancels pending intent preloads; add a second case that creates a router with defaultPreloadDelay > 0, uses vi.useFakeTimers(), spies router.preloadRoute, renders RouterProvider, finds the Link, fires mouseOver to schedule a timer, then fires mouseLeave (or blur) to cancel it, advances timers (vi.runAllTimers or equivalent) and asserts preloadRoute was not called, finally call vi.useRealTimers(); reference symbols: timeoutMap/handleLeave in the module and router.preloadRoute and Link component in the test.
4554-4559: Negative assertion usingsleep(1)may be flaky under load.With
defaultPreloadDelay: 0, the intent handler schedulessetTimeout(fn, 0).await sleep(1)relies on real wall-clock time to allow that 0ms callback to fire before checking. This is sufficient in most environments but can race under heavy CI load.Consider using fake timers for a deterministic boundary:
♻️ Suggested refactor using fake timers
+ vi.useFakeTimers() fireEvent.focus(aboutLink) fireEvent.mouseOver(aboutLink) fireEvent.touchStart(aboutLink) - await sleep(1) + vi.runAllTimers() expect(preloadRouteSpy).toHaveBeenCalledTimes(baselineCalls) + + vi.useRealTimers()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/tests/link.test.tsx` around lines 4554 - 4559, The negative assertion using sleep(1) is flaky; switch the test to use fake timers instead of waiting real time: enable fake timers (e.g., jest.useFakeTimers()), trigger the events on aboutLink (fireEvent.focus/mouseOver/touchStart), then advance or run timers deterministically (e.g., jest.advanceTimersByTime(0) or jest.runOnlyPendingTimers()) to let the defaultPreloadDelay: 0 scheduled callback run, and finally assert preloadRouteSpy has been called the expected number of times; restore real timers after the test. Ensure you update the test surrounding preloadRouteSpy and any setup/teardown to use jest.useFakeTimers()/jest.useRealTimers().
🤖 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/vue-router/tests/link.test.tsx`:
- Around line 4563-4613: The test currently only asserts positive preload calls
but misses verifying that handleLeave cancels pending intent preloads; add a
second case that creates a router with defaultPreloadDelay > 0, uses
vi.useFakeTimers(), spies router.preloadRoute, renders RouterProvider, finds the
Link, fires mouseOver to schedule a timer, then fires mouseLeave (or blur) to
cancel it, advances timers (vi.runAllTimers or equivalent) and asserts
preloadRoute was not called, finally call vi.useRealTimers(); reference symbols:
timeoutMap/handleLeave in the module and router.preloadRoute and Link component
in the test.
- Around line 4554-4559: The negative assertion using sleep(1) is flaky; switch
the test to use fake timers instead of waiting real time: enable fake timers
(e.g., jest.useFakeTimers()), trigger the events on aboutLink
(fireEvent.focus/mouseOver/touchStart), then advance or run timers
deterministically (e.g., jest.advanceTimersByTime(0) or
jest.runOnlyPendingTimers()) to let the defaultPreloadDelay: 0 scheduled
callback run, and finally assert preloadRouteSpy has been called the expected
number of times; restore real timers after the test. Ensure you update the test
surrounding preloadRouteSpy and any setup/teardown to use
jest.useFakeTimers()/jest.useRealTimers().
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/tests/link.test.tsxpackages/solid-router/tests/link.test.tsxpackages/vue-router/tests/link.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-router/tests/link.test.tsx
- packages/solid-router/tests/link.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/vue-router/tests/link.test.tsx (1)
4507-4561: LGTM — minor: the'render'baseline guard is missing for'viewport'The
if (preloadMode === 'render')block correctly waits for the initial render-triggered call before capturingbaselineCalls. For'viewport'this is safe only because thegetIntersectionObserverMockhelper never auto-fires the intersection callback — an implicit assumption. If the mock is ever updated to simulate visible elements,preloadRoutewould be called asynchronously afterfindByRolereturns but before or duringsleep(100), and the baseline would be stale.Consider symmetrically guarding both:
♻️ Suggested guard for `viewport` and `render` modes
- if (preloadMode === 'render') { + if (preloadMode === 'render' || preloadMode === 'viewport') { await waitFor(() => expect(preloadRouteSpy.mock.calls.length).toBeGreaterThan(0), ) }Or, if the intent is to assert that
'viewport'mode never callspreloadRouteuntil the IntersectionObserver fires (i.e.,baselineCallsshould always be 0 for viewport), document that assumption with a comment:+ // Note: the IntersectionObserver mock does not auto-fire the callback, + // so viewport preload is never triggered during render in this test environment. const baselineCalls = preloadRouteSpy.mock.calls.length🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/tests/link.test.tsx` around lines 4507 - 4561, The test misses the same async baseline guard for preloadMode === 'viewport' as it has for 'render', which can make baselineCalls stale if getIntersectionObserverMock ever auto-fires; update the conditional that waits for an initial render-triggered preload to cover both modes (e.g., if (preloadMode === 'render' || preloadMode === 'viewport') await waitFor(() => expect(preloadRouteSpy.mock.calls.length).toBeGreaterThan(0))); touch the test's preloadMode check around preloadRouteSpy and/or add a brief comment referencing getIntersectionObserverMock to document the assumption if you prefer to keep the original behavior.
🤖 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/vue-router/tests/link.test.tsx`:
- Around line 4507-4561: The test misses the same async baseline guard for
preloadMode === 'viewport' as it has for 'render', which can make baselineCalls
stale if getIntersectionObserverMock ever auto-fires; update the conditional
that waits for an initial render-triggered preload to cover both modes (e.g., if
(preloadMode === 'render' || preloadMode === 'viewport') await waitFor(() =>
expect(preloadRouteSpy.mock.calls.length).toBeGreaterThan(0))); touch the test's
preloadMode check around preloadRouteSpy and/or add a brief comment referencing
getIntersectionObserverMock to document the assumption if you prefer to keep the
original behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/tests/link.test.tsxpackages/solid-router/tests/link.test.tsxpackages/vue-router/tests/link.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-router/tests/link.test.tsx
- packages/solid-router/tests/link.test.tsx
Sheraff
changed the title
1 participant
Summary by CodeRabbit