refactor(router-core,react-router,solid-router): avoid copying arrays just to .reverse() them #4980
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… just to .reverse() them
Contributor
WalkthroughRefactors meta tag processing in React and Solid HeadContent components to use reverse-indexed for-loops and local arrays; adds a new findLast utility and updates RouterCore.buildLocation to use it for last-match lookups. No exported/public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Router as RouterCore.buildLocation
participant Utility as findLast
participant Matches as allCurrentLocationMatches
Router->>Matches: gather matches array
Router->>Utility: findLast(allCurrentLocationMatches, comparer)
Utility-->>Router: last matching entry (or undefined)
Router->>Router: use matchedFrom / matchedCurrent in buildLocation logic
sequenceDiagram
participant Head as HeadContent.useTags
participant Routes as routeMeta()
participant Result as resultMeta
Head->>Routes: read routeMetasArray
Head->>Routes: iterate routes in reverse (for i = len-1 .. 0)
Head->>Routes: iterate each route's metas in reverse (for j = len-1 .. 0)
alt meta is title and no title set
Head->>Result: set title placeholder
else duplicate or falsy
Head->>Head: continue (skip)
else regular meta
Head->>Result: push meta tag
end
Head->>Result: push title (if found) and reverse resultMeta before return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 86875e2
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router-core/src/utils.ts (1)
486-497: AlignfindLastwith ES2023 semantics and supportReadonlyArray(drop non-null assertions).Good addition and matches the PR intent. To make this utility more generally usable and type-safe:
- Accept
ReadonlyArray<T>since we don't mutate the input.- Pass
(item, index, array)to the predicate to mirrorArray.prototype.findLast.- Avoid
!assertions to be robust with sparse arrays and unions that includeundefined.Suggested diff:
-export function findLast<T>( - array: Array<T>, - predicate: (item: T) => boolean, -): T | undefined { - for (let i = array.length - 1; i >= 0; i--) { - if (predicate(array[i]!)) { - return array[i]! - } - } - return undefined -} +export function findLast<T>( + array: ReadonlyArray<T>, + predicate: (item: T, index: number, array: ReadonlyArray<T>) => boolean, +): T | undefined { + for (let i = array.length - 1; i >= 0; i--) { + const item = array[i] + if (predicate(item, i, array)) { + return item + } + } + return undefined +}packages/react-router/src/HeadContent.tsx (1)
37-37: Usingcontinuefor duplicate meta attributes is correct and clearer.Skips duplicates without extra closures and matches previous guard behavior.
packages/solid-router/src/HeadContent.tsx (1)
39-39: Use ofcontinueto skip duplicates is correct and avoids nested returns.Matches React implementation for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/react-router/src/HeadContent.tsx(3 hunks)packages/router-core/src/router.ts(2 hunks)packages/router-core/src/utils.ts(1 hunks)packages/solid-router/src/HeadContent.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (1)
packages/router-core/src/utils.ts (1)
findLast(487-497)
⏰ 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 (4)
packages/router-core/src/router.ts (2)
11-11: ImportingfindLastis appropriate for de-duping reverse scans.No concerns with adding this import alongside existing utils.
1445-1451: Eliminate transient array copies for last-match lookups (LGTM)File: packages/router-core/src/router.ts
Lines: 1445–1451const matchedFrom = findLast(allCurrentLocationMatches, (d) => { return comparePaths(d.fullPath, fromPath) }) const matchedCurrent = findLast(allFromMatches, (d) => { return comparePaths(d.fullPath, currentLocation.pathname) })Replacing the
[…arr].reverse().find(…)pattern withfindLastremoves unnecessary allocations in this hot path while preserving behavior. The dev-only validation remains clear and semantically equivalent.Verified: no remaining
[...arr].reverse().<method>()patterns found in the repo.packages/react-router/src/HeadContent.tsx (1)
20-25: Reverse-indexed loops avoid extra array allocations and preserve output order (LGTM).Iterating from the end and reversing
resultMetaat the end retains the prior behavior while removing spread+reverse overhead. Good call keepingtitlehandling intact.Also applies to: 50-52
packages/solid-router/src/HeadContent.tsx (1)
21-27: Solid: reverse loops + memoizedrouteMetasArrayreduce allocations (LGTM).
- Reverse iteration removes spread+reverse temporary arrays.
- Caching
routeMetasArray = routeMeta()avoids repeated accessor calls.- Final
resultMeta.reverse()maintains prior output order.Also applies to: 52-53
levrik
reviewed
Aug 25, 2025
Comment on lines
+487
to
+496
| export function findLast<T>( | ||
| array: ReadonlyArray<T>, | ||
| predicate: (item: T) => boolean, | ||
| ): T | undefined { | ||
| for (let i = array.length - 1; i >= 0; i--) { | ||
| const item = array[i]! | ||
| if (predicate(item)) return item | ||
| } | ||
| return undefined | ||
| } |
Contributor
There was a problem hiding this comment.
Is there anything that speaks against using the built-in?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLast
Contributor
Author
There was a problem hiding this comment.
.findLast is still a little bit recent (safari 15.4), and I didn't want this PR to be a breaking change. But I'd love to use the built-in array method instead.
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 26, 2025
… just to .reverse() them (#4980) The pattern of copying an array just to `.reverse()` it and iterate it from the end ends up creating a transient object (a copy of the array) that needs to be garbage collected. For example: ```ts [...array].reverse().find(() => {/*...*/}) ``` or ```ts [...array].reverse().forEach(() => {/*...*/}) ``` Instead we can use a regular `for` loop to iterate it from the end. This will be mostly beneficial in `router-core > router > buildLocation` that gets called pretty often. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved head metadata processing in React and Solid routers to be more efficient and readable, reducing work for large route metadata sets. - Simplified route matching logic to consistently pick the last relevant match, improving clarity and maintainability. - **Chores** - Added a utility to locate the last list item meeting a condition, supporting cleaner routing internals. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The pattern of copying an array just to
.reverse()it and iterate it from the end ends up creating a transient object (a copy of the array) that needs to be garbage collected.For example:
or
Instead we can use a regular
forloop to iterate it from the end. This will be mostly beneficial inrouter-core > router > buildLocationthat gets called pretty often.Summary by CodeRabbit
Refactor
Chores