fix: Links always scroll to URL hash on hover (#5930) #5944
Conversation
github-actions
bot
added
documentation
|
Warning Rate limit exceeded@birkskyum has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughHash-scroll behavior was made conditional: Transitioner computes a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant Transitioner
participant Store
rect rgb(230,240,255)
note over User,Transitioner: Hover / Intent preload (no URL change)
User->>Router: Hover link (intent)
Router->>Transitioner: start transition (pending → resolved)
Transitioner->>Transitioner: changeInfo = getLocationChangeInfo(router.state) (hrefChanged=false)
Transitioner->>Store: router.__store.setState(...) %% updater uses typed param
Note over Transitioner: handleHashScroll NOT invoked (hrefChanged=false)
end
rect rgb(230,255,230)
note over User,Transitioner: Actual click navigation to hash
User->>Router: Click link with hash
Router->>Transitioner: start transition (pending → resolved)
Transitioner->>Transitioner: changeInfo = getLocationChangeInfo(router.state) (hrefChanged=true)
Transitioner->>Store: router.__store.setState(...)
Transitioner->>Router: handleHashScroll(router) invoked (hrefChanged=true)
Router->>User: Browser scrolls to hash anchor
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
@birkskyum It reflects the changed files. Please review this PR. |
|
View your CI Pipeline Execution ↗ for commit a8fcb2e
☁️ Nx Cloud last updated this comment at |
birkskyum
force-pushed
the
fix/hash-hover-scroll
branch
from
dd1226e to
a8916e5
Compare
|
Great! could we add a test for this based on the issue reproduction to avoid regressions later? |
|
@birkskyum Thanks for the review. I can add a test, but I haven't written tests in this repo before, so I’m not sure about the expected location. Should this go inside the existing |
|
In the e2e/react-router/basic-file-based/ (same for solid) you can add a new route and test file there |
|
I'll add the tests in e2e/react-router/basic-file-based/tests/ and same for solid-router. Thanks for the help! |
|
@birkskyum I added descriptive tests. Please review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
1-22: Test scenario looks good; consider hardening scroll + hash handling assumptionsThis nicely captures the regression and keeps the assertion focused on scroll position, not navigation. Two small robustness nits:
scrollBy(0, -300)assumes 300px is always enough to moveat-the-bottomfully out of the viewport. Depending on viewport size or layout tweaks, this can become borderline and cause flakes. You might usewindow.innerHeight(or a larger constant, e.g.-window.innerHeight) to guarantee the target is well off-screen.toRuntimePath('/normal-page#at-the-bottom')implicitly assumestoRuntimePathpreserves hash fragments. Worth double-checking that helper (or adding a quick assertion in another test) so this doesn’t silently stop exercising the hash behavior iftoRuntimePathever changes.If the current setup has been stable in CI, these can stay as-is, but they’re easy guardrails against future layout/runtime changes.
e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
1-23: Keep Solid test in lockstep with React variant and consider shared helperThis Solid test mirrors the React one well, which is great for adapter parity. The same comments apply here:
- The fixed
scrollBy(0, -300)may be brittle if viewport or layout changes; consider basing it onwindow.innerHeight(or a larger constant) to ensureat-the-bottomis definitely off-screen before the hover.- Ensure
toRuntimePathcontinues to preserve the#at-the-bottomhash so the test actually exercises hash-scroll behavior.Optionally, you could factor the “navigate-with-hash → verify in-view → scroll out → hover link → assert still out-of-view” flow into a small shared helper used by both React and Solid tests to keep behavior and tweaks in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.tse2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.tse2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
🧬 Code graph analysis (2)
e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
e2e/e2e-utils/src/index.ts (1)
toRuntimePath(3-3)
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
e2e/e2e-utils/src/index.ts (1)
toRuntimePath(3-3)
|
@birkskyum fixed errors. Please Review. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(22 hunks)e2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/lazy-page.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(22 hunks)e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/lazy-page.tsx(1 hunks)e2e/solid-start/basic-cloudflare/worker-configuration.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsxe2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsxe2e/react-router/basic-file-based/src/routes/lazy-page.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsxe2e/react-router/basic-file-based/src/routes/lazy-page.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
🧬 Code graph analysis (4)
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx (1)
e2e/solid-router/basic-file-based/src/routes/lazy-page.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsx (1)
e2e/react-router/basic-file-based/src/routes/lazy-page.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/lazy-page.tsx (2)
e2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/lazy-page.tsx (1)
Route(3-5)
e2e/solid-router/basic-file-based/src/routes/lazy-page.tsx (1)
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx (1)
Route(3-5)
🪛 ESLint
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx
[error] 1-1: Member 'Link' of the import declaration should be sorted alphabetically.
(sort-imports)
e2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsx
[error] 1-1: Member 'Link' of the import declaration should be sorted alphabetically.
(sort-imports)
🔇 Additional comments (4)
e2e/solid-start/basic-cloudflare/worker-configuration.d.ts (1)
5-7: Type-only tweaks look correct and non-breakingThe updates to
Cloudflare.Env.MY_VAR,StringifyValues, andNodeJS.ProcessEnvare syntactic/formatting-only and keep the same literal types and mapped-type behavior, so they should have no functional or type-safety impact. Consistent with typical Wrangler-generated declarations.Also applies to: 11-12, 14-14
e2e/react-router/basic-file-based/src/routes/lazy-page.tsx (1)
1-14: Lazy page React route looks correctRoute path, component wiring, and JSX structure all look consistent with other file-based routes and suitable for the new e2e tests.
e2e/solid-router/basic-file-based/src/routes/lazy-page.tsx (1)
1-14: Lazy page Solid route looks correctSolid route configuration and JSX (including
classusage) match the expected pattern and align with the React variant.e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
1-2487: Skipping review of autogenerated file.This file is automatically generated by TanStack Router and should not be manually modified or reviewed. The changes reflect the addition of new route files elsewhere in the codebase.
Based on learnings.
e2e/react-router/basic-file-based/src/routes/(tests)/normal-page.tsx
Outdated
Show resolved
Hide resolved
e2e/solid-router/basic-file-based/src/routes/(tests)/normal-page.tsx
Outdated
Show resolved
Hide resolved
|
not so sure if this is the actual fix here. need to discuss with @birkskyum |
e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
1-264: Route and hash target look consistent and suitable for the test
/hover-preload-hashis wired correctly, theLinkhash (position1) matches the<h2 id="position1">target, and the long content gives enough scroll distance for the viewport assertions. Only tiny nit: the "Hello from About!" copy is slightly mismatched to the route name, but that’s purely cosmetic for an e2e fixture.e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
1-24: E2E scenario accurately captures the regression; only minor concern is the fixed timeoutThe flow (navigate → click hash link → scroll away → hover preloading link → assert no jump) matches the described bug and should guard against regressions. The only soft spot is
waitForTimeout(400), which can be a bit brittle; if there’s an existing helper or event you can await for “preload settled”, that would be slightly more robust, but it’s acceptable as-is for this narrow case.e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
1-264: Solid route mirrors the React variant correctlyThe Solid route for
/hover-preload-hashmatches the React version in path, hash target, and layout, and uses Solid’sclassattribute appropriately. This should give the Solid e2e test the same behavior surface. As with the React file, the “Hello from About!” text is slightly misleading but harmless for a test fixture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx(1 hunks)e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsxe2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
🧬 Code graph analysis (1)
e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
Route(3-5)
🔇 Additional comments (2)
e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
15-2465: Autogenerated route tree – relying on generator outputThis
routeTree.gen.tsfile is auto-generated; per project guidance and learnings, its structure shouldn’t be hand-edited or deeply reviewed. The new/hover-preload-hashentries appear to be the expected generator output corresponding to the added route file.e2e/solid-router/basic-file-based/src/routeTree.gen.ts (1)
15-2488: Autogenerated Solid route tree – deferring to the generatorThis
routeTree.gen.tsis generated by the router tooling; per project guidance, it shouldn’t be manually edited or deeply reviewed. The/hover-preload-hashadditions look like the expected mirror of the new Solid route file.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx(1 hunks)e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-router/basic-file-based/tests/hover-preload-hash.spec.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.tse2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsxe2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts
🧬 Code graph analysis (1)
e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
e2e/e2e-utils/src/index.ts (1)
toRuntimePath(3-3)
🪛 Biome (2.1.2)
e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx
[error] 260-260: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx
[error] 260-260: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
🔇 Additional comments (3)
e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
260-260: Static analysis parse error is a false positive.The reported parse error at line 260 is a false positive. The JSX closing syntax
)is valid and correctly closes the return statement.e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx (1)
260-260: Static analysis parse error is a false positive.The reported parse error at line 260 is a false positive. The JSX closing syntax
)is valid and correctly closes the return statement.e2e/react-router/basic-file-based/tests/hover-preload-hash.spec.ts (1)
19-19: Original review comment is incorrect. The testid exists in the codebase.The testid
'link-to-only-route-inside-group'is defined ine2e/react-router/basic-file-based/src/routes/__root.tsxat line 69, which is the root layout file. Since it's defined in the shared root layout rather than the route-specific file, it's available across all routes including/hover-preload-hash. The test will execute correctly and find the element without issues.Likely an incorrect or invalid review comment.
|
|
||
| return <> | ||
|
|
||
| <div className="p-2">Hello from About!</div> |
There was a problem hiding this comment.
Update component text to match route purpose.
The text displays "Hello from About!" but this route is /hover-preload-hash, not an about page.
Apply this diff:
- <div className="p-2">Hello from About!</div>
+ <div className="p-2">Hello from Hover Preload Hash!</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="p-2">Hello from About!</div> | |
| <div className="p-2">Hello from Hover Preload Hash!</div> |
🤖 Prompt for AI Agents
In e2e/react-router/basic-file-based/src/routes/hover-preload-hash.tsx around
line 11, update the displayed text so it reflects this route instead of saying
"Hello from About!"; replace that string with something appropriate like "Hello
from hover-preload-hash!" or "Hover preload hash route" so the component text
matches the route purpose.
|
|
||
| return <> | ||
|
|
||
| <div class="p-2">Hello from About!</div> |
There was a problem hiding this comment.
Update component text to match route purpose.
The text displays "Hello from About!" but this route is /hover-preload-hash, not an about page.
Apply this diff:
- <div class="p-2">Hello from About!</div>
+ <div class="p-2">Hello from Hover Preload Hash!</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="p-2">Hello from About!</div> | |
| <div class="p-2">Hello from Hover Preload Hash!</div> |
🤖 Prompt for AI Agents
In e2e/solid-router/basic-file-based/src/routes/hover-preload-hash.tsx around
line 11, the rendered text incorrectly says "Hello from About!"; update that
string to something appropriate for the route such as "Hello from Hover Preload
Hash!" (or another concise label reflecting /hover-preload-hash) so the
component text matches the route purpose.
|
@birkskyum Thank you! |
3 participants
closes #5930
Fix: Prevent hash scroll on preload-only transitions by gating handleHashScroll to true navigations
What was the issue (#5930)
Hovering a Link with defaultPreload set to "intent" (or any programmatic preload) caused the router to trigger a hash scroll even though no navigation occurred. This resulted in the page unexpectedly jumping to an element associated with a hash fragment (for example, #id) when the user only hovered over a link.
In versions prior to v1.134.19, this did not occur. Between v1.134.18 and v1.134.20, router-core began running updateMatch inside router.startTransition. As a result, framework adapters (React and Solid) emitted onResolved during preload transitions. Both adapters unconditionally called handleHashScroll inside onResolved, so preloads triggered hash scrolling despite no URL change.
This caused the following behavior:
The fix
In both framework Transitioner implementations (React and Solid):
This restores the intended behavior:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.