Skip to content

Conversation

@birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 28, 2025

Summary by CodeRabbit

  • Chores

    • Bumped runtime store dependencies across router packages to the latest minor release (no public API changes). Estimated review effort: medium.
  • Bug Fixes

    • Restored prior state-update behavior so components reliably update when selector results are value-equivalent, preserving external API.
  • Tests

    • Disabled some end-to-end tests, removed a large params validation test, and adjusted one test expectation related to render counts.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Bumped TanStack store dependencies in three package manifests, added a deep-equality comparator to the Solid router state hook, and adjusted/removed several Solid e2e tests (two tests skipped, a large params test removed, and one assertion expectation changed). No public API signatures changed.

Changes

Cohort / File(s) Summary
Manifests (TanStack store bumps)
packages/solid-router/package.json, packages/react-router/package.json, packages/router-core/package.json
Updated store dependencies from 0.7.0 to ^0.8.0 (@tanstack/solid-store, @tanstack/react-store, @tanstack/store).
Solid router state hook
packages/solid-router/src/useRouterState.tsx
Added a deepEqual helper and passed it as the equal option to useStore(...) so selector results are compared by deep equality before triggering updates.
Solid e2e tests — skipped tests
e2e/solid-router/basic-file-based/tests/app.spec.ts
Two occurrences of "Should not remount deps when remountDeps does not change" changed to test.skip(...).
Solid e2e tests — removed / modified tests
e2e/solid-router/basic-file-based/tests/params.spec.ts
Removed a large test block for multiple applicable named params; adjusted one assertion (expected render count changed from 1 to 2) and minor formatting whitespace change.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Component as Router Consumer
  participant Hook as useRouterState
  participant Store as Solid Store
  rect #E8F5E9
    Note over Hook,Store: deepEqual comparator added to useStore(equal)
  end
  Component->>Hook: subscribe(selector)
  Hook->>Store: useStore(selector, { equal: deepEqual })
  Store-->>Hook: selectorResult (new ref or equal value)
  alt deepEqual(selectorResult, prev) == true
    Hook->>Component: no update (skip render)
  else
    Hook->>Component: emit update (re-render)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Focus areas:
    • packages/solid-router/src/useRouterState.tsx — correctness and performance of deepEqual, edge cases, and interaction with solid-store ^0.8.0 semantics.
    • Manifest bumps — ensure dependency compatibility across packages and CI.
    • Adjusted/removed tests — verify intended test coverage and reasons for skipping/removal.

Possibly related PRs

Suggested reviewers

  • schiller-manuel

Poem

🐇 I hopped a patch through package lanes,
I tuned the stores and checked their brains.
Deep‑eye listens, skips the noise,
Tests trimmed gently, tidy joys.
Tiny hop — the routers sing.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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 PR title "fix(router): bump tanstack store to ^0.8" directly and accurately reflects the primary change in the pull request. The main objective is to upgrade the @TanStack store dependencies across three packages (solid-router, react-router, and router-core) from version 0.7.0 to 0.8.0. While the PR includes secondary supporting changes such as introducing a deep equality helper in useRouterState and adjusting tests, these are necessary adaptations to accommodate the new store version and do not represent the primary purpose of the changeset. The title is concise, clear, and specific enough that a developer reviewing the commit history would understand this is a dependency upgrade.
✨ 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 chore(solid-router)--bump-solid-store-to-0.8

📜 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 0ae52f2 and 3d294d2.

📒 Files selected for processing (2)
  • e2e/solid-router/basic-file-based/tests/app.spec.ts (1 hunks)
  • e2e/solid-router/basic-file-based/tests/params.spec.ts (2 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:

  • e2e/solid-router/basic-file-based/tests/params.spec.ts
  • e2e/solid-router/basic-file-based/tests/app.spec.ts
e2e/**

📄 CodeRabbit inference engine (AGENTS.md)

Store end-to-end tests under the e2e/ directory

Files:

  • e2e/solid-router/basic-file-based/tests/params.spec.ts
  • e2e/solid-router/basic-file-based/tests/app.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Preview
  • GitHub Check: Test
🔇 Additional comments (2)
e2e/solid-router/basic-file-based/tests/params.spec.ts (2)

248-248: The render count change is expected and correct.

When navigating from /foo/bar/1 to /foo/bar/1/baz, the router state changes (the baz parameter is added to the params object). The deepEqual function in useRouterState.tsx correctly detects this state difference by comparing object keys and values recursively. This triggers a legitimate re-render of the fooBar component (1 → 2), which is the intended behavior of the deep equality logic introduced for the solid-store 0.8 upgrade. The test expectation is accurate.


1-369: Unable to verify review comment — manual verification required

Shell script execution failed due to environment constraints; cannot access repository history or file system to verify whether the test block was actually removed, check git diffs, or confirm alternative test coverage.

To proceed, manually verify:

  • Whether a "multiple-applicable params behavior" test block existed in a prior version
  • If removed, confirm git commit details and reason
  • Search for related param scenario coverage elsewhere in the e2e suite
  • Check changelog or PR notes for store 0.8 compatibility context

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

View your CI Pipeline Execution ↗ for commit 3d294d2

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

☁️ Nx Cloud last updated this comment at 2025-10-29 01:09:30 UTC

@birkskyum birkskyum changed the title chore(solid-router): bump solid-store to 0.8 chore(router): bump tanstack store to 0.8 Oct 29, 2025
@birkskyum birkskyum changed the title chore(router): bump tanstack store to 0.8 fix(router): bump tanstack store to 0.8 Oct 29, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 29, 2025

More templates

@tanstack/arktype-adapter

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

@tanstack/directive-functions-plugin

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

@tanstack/eslint-plugin-router

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

@tanstack/history

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

@tanstack/nitro-v2-vite-plugin

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

@tanstack/react-router

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

@tanstack/react-router-devtools

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

@tanstack/react-router-ssr-query

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

@tanstack/react-start

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

@tanstack/react-start-client

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

@tanstack/react-start-server

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

@tanstack/router-cli

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

@tanstack/router-core

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

@tanstack/router-devtools

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

@tanstack/router-devtools-core

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-ssr-query-core

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

@tanstack/router-utils

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

@tanstack/router-vite-plugin

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

@tanstack/server-functions-plugin

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

@tanstack/solid-router

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

@tanstack/solid-router-devtools

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

@tanstack/solid-router-ssr-query

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

@tanstack/solid-start

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

@tanstack/solid-start-client

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

@tanstack/solid-start-server

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

@tanstack/start-client-core

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

@tanstack/start-plugin-core

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

@tanstack/start-server-core

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

@tanstack/start-static-server-functions

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

@tanstack/start-storage-context

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

@tanstack/valibot-adapter

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

@tanstack/virtual-file-routes

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

@tanstack/zod-adapter

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

commit: 3d294d2

@birkskyum birkskyum changed the title fix(router): bump tanstack store to 0.8 fix(router): bump tanstack store to ^0.8 Oct 29, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2c4729 and 51de201.

📒 Files selected for processing (1)
  • packages/solid-router/src/useRouterState.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/useRouterState.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/useRouterState.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-30T09:12:13.852Z
Learnt from: Sheraff
PR: TanStack/router#5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.852Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.

Applied to files:

  • packages/solid-router/src/useRouterState.tsx
🧬 Code graph analysis (1)
packages/solid-router/src/useRouterState.tsx (2)
packages/router-core/src/router.ts (1)
  • state (1082-1084)
packages/react-router/src/useRouterState.tsx (1)
  • UseRouterStateResult (26-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test
🔇 Additional comments (2)
packages/solid-router/src/useRouterState.tsx (2)

1-1: LGTM: ESLint disable is justified.

The type assertion on line 71 justifies this directive.


58-71: The equal option signature is compatible with solid-store 0.8.0 API.

The implementation correctly uses the equal option with the expected (a, b) => boolean signature supported by useStore in @tanstack/solid-store v0.8.0. The API accepts an options object where you can pass an equality function as { equal?: (a, b) => boolean }, which matches how deepEqual is passed in the code.

Comment on lines +11 to +35
// Deep equality check to match behavior of solid-store 0.7.0's reconcile()
function deepEqual(a: any, b: any): boolean {
if (Object.is(a, b)) return true

if (
typeof a !== 'object' ||
a === null ||
typeof b !== 'object' ||
b === null
) {
return false
}

const keysA = Object.keys(a)
const keysB = Object.keys(b)

if (keysA.length !== keysB.length) return false

for (const key of keysA) {
if (!Object.prototype.hasOwnProperty.call(b, key)) return false
if (!deepEqual(a[key], b[key])) return false
}

return true
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Consider array handling, circular reference protection, and using an established library.

The deepEqual implementation has several considerations:

  1. Array vs. object distinction: The function doesn't differentiate between arrays and plain objects. For example, deepEqual([1, 2], {0: 1, 1: 2}) would incorrectly return true. If RouterState contains arrays, this could produce unexpected behavior.

  2. Circular reference protection: No cycle detection is implemented. While RouterState is unlikely to contain circular references, adding protection would prevent potential stack overflows.

  3. Library preference: Per the coding guidelines, prefer well-known libraries over custom implementations for common tasks like deep equality. Consider using a battle-tested library (e.g., fast-deep-equal, Lodash's isEqual) instead of maintaining custom logic.

Apply this diff to add array handling:

 function deepEqual(a: any, b: any): boolean {
   if (Object.is(a, b)) return true
 
   if (
     typeof a !== 'object' ||
     a === null ||
     typeof b !== 'object' ||
     b === null
   ) {
     return false
   }
+
+  // Handle arrays
+  const aIsArray = Array.isArray(a)
+  const bIsArray = Array.isArray(b)
+  if (aIsArray !== bIsArray) return false
+  if (aIsArray && bIsArray) {
+    if (a.length !== b.length) return false
+    for (let i = 0; i < a.length; i++) {
+      if (!deepEqual(a[i], b[i])) return false
+    }
+    return true
+  }
 
   const keysA = Object.keys(a)
   const keysB = Object.keys(b)

Alternative (recommended): Use an established library

import { deepEqual } from 'fast-deep-equal'
// or
import { isEqual as deepEqual } from 'lodash-es'
🤖 Prompt for AI Agents
In packages/solid-router/src/useRouterState.tsx around lines 11 to 35, the
custom deepEqual implementation fails to distinguish arrays from plain objects,
lacks circular-reference protection, and duplicates common functionality better
handled by a library; replace the custom function by importing a well-tested
deep equality implementation (e.g., import deepEqual from 'fast-deep-equal' or
import { isEqual as deepEqual } from 'lodash-es'), remove the local deepEqual
function, and update any references to use the imported function so arrays are
correctly compared and you get robust handling for edge cases without
maintaining custom logic.

@birkskyum birkskyum force-pushed the chore(solid-router)--bump-solid-store-to-0.8 branch from 51de201 to 0ae52f2 Compare October 29, 2025 00:48
@birkskyum birkskyum merged commit f9bf3a5 into main Oct 29, 2025
6 checks passed
@birkskyum birkskyum deleted the chore(solid-router)--bump-solid-store-to-0.8 branch October 29, 2025 01:09
@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