Skip to content

Conversation

@birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 25, 2025

3 skipped link tests can be now be enabled. 2 due to timing, and 1 missed an accessor call

Summary by CodeRabbit

  • Bug Fixes

    • Fixed external links to correctly resolve to their intended URLs.
  • Tests

    • Re-enabled previously skipped test scenarios for navigation and search parameter updates.
    • Enhanced tests with explicit waits to ensure UI updates are captured before assertions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 25, 2025

Walkthrough

A bug fix in the solid-router link component where external link href values are now properly derived by invoking externalLink() instead of using the memo reference itself. Previously skipped tests are re-enabled with explicit waits for updated UI elements before assertions.

Changes

Cohort / File(s) Summary
Link component bug fix
packages/solid-router/src/link.tsx
Fixed useLinkProps to call externalLink() function and retrieve the actual URL string instead of passing the memo function reference as href.
Test updates and re-enablement
packages/solid-router/tests/link.test.tsx
Re-enabled previously skipped tests by removing test.skip decorators. Added explicit waits to refetch updated UI elements (current-page, current-filter) after navigation before asserting their values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • The bug fix is straightforward and isolated to a single function invocation
  • Test re-enablement follows a consistent pattern across multiple test cases
  • Changes are well-scoped with minimal file spread

Suggested labels

package: solid-router

Suggested reviewers

  • schiller-manuel
  • brenelz

Poem

🐰 A function wasn't called, just sitting there with glee,
Now it runs and returns the URL we need to see!
Tests once hidden, now awake and bright,
With waits for updates—the assertions shine 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 "test(solid-router): un-skip link tests" accurately describes a significant part of the changeset—specifically, the re-enabling of previously skipped tests in the test file. While the PR also includes a bug fix in the source file (link.tsx) that corrects how external link hrefs are handled, the title correctly refers to a real and concrete aspect of the change. The title is clear, specific, and uses proper conventional commit format, making it understandable to someone reviewing the repository history. The title is not misleading or off-topic, though it does not capture every detail of the changeset.
✨ 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 test(solid-router)--un-skip-link-tests

📜 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 b5c96d4 and 5a5285a.

📒 Files selected for processing (2)
  • packages/solid-router/src/link.tsx (1 hunks)
  • packages/solid-router/tests/link.test.tsx (5 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/solid-router/tests/link.test.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
  • packages/solid-router/tests/link.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
PR: TanStack/router#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:

  • packages/solid-router/tests/link.test.tsx
🔇 Additional comments (4)
packages/solid-router/src/link.tsx (1)

263-263: LGTM! Correct invocation of the memo accessor.

The fix properly invokes externalLink() to retrieve the string value instead of passing the memo function reference itself. This ensures external links receive the correct href string.

packages/solid-router/tests/link.test.tsx (3)

678-678: LGTM! Proper async handling added.

Un-skipping this test with explicit awaits for DOM updates after navigation ensures the UI reflects the new search parameters before assertions. This resolves the timing issue that originally caused the test to be skipped.

Also applies to: 776-778


1630-1630: LGTM! Timing fix for navigation assertion.

Un-skipping this test with an explicit await for the home heading ensures the navigation has completed and the UI has updated before asserting the pathname. This addresses the original timing issue.

Also applies to: 1748-1749


6349-6349: LGTM! Test un-skipped after external link fix.

This test validates href generation for links with rewritten origins. Un-skipping is appropriate now that line 263 in link.tsx properly invokes externalLink() to generate the href string value.


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 25, 2025

View your CI Pipeline Execution ↗ for commit 5a5285a

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

☁️ Nx Cloud last updated this comment at 2025-10-25 22:22:30 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 25, 2025

More templates

@tanstack/arktype-adapter

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

@tanstack/directive-functions-plugin

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

@tanstack/eslint-plugin-router

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

@tanstack/history

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

@tanstack/nitro-v2-vite-plugin

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

@tanstack/react-router

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

@tanstack/react-router-devtools

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

@tanstack/react-router-ssr-query

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

@tanstack/react-start

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

@tanstack/react-start-client

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

@tanstack/react-start-server

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

@tanstack/router-cli

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

@tanstack/router-core

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

@tanstack/router-devtools

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

@tanstack/router-devtools-core

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-ssr-query-core

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

@tanstack/router-utils

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

@tanstack/router-vite-plugin

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

@tanstack/server-functions-plugin

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

@tanstack/solid-router

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

@tanstack/solid-router-devtools

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

@tanstack/solid-router-ssr-query

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

@tanstack/solid-start

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

@tanstack/solid-start-client

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

@tanstack/solid-start-server

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

@tanstack/start-client-core

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

@tanstack/start-plugin-core

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

@tanstack/start-server-core

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

@tanstack/start-static-server-functions

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

@tanstack/start-storage-context

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

@tanstack/valibot-adapter

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

@tanstack/virtual-file-routes

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

@tanstack/zod-adapter

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

commit: 5a5285a

@birkskyum birkskyum merged commit 08d78c1 into main Oct 25, 2025
6 checks passed
@birkskyum birkskyum deleted the test(solid-router)--un-skip-link-tests branch October 25, 2025 22:26
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.

1 participant