Skip to content

Conversation

@Sheraff
Copy link
Contributor

@Sheraff Sheraff commented Nov 19, 2025

We can use typescript asserts to for the type of the Uint16Array buffer of parseSegment instead of returning it. It's not much, but it makes using this function a little cleaner, and maybe more clear that it modifies the input buffer (instead of returning a new buffer).

Summary by CodeRabbit

  • Refactor

    • Internal routing parsing was refactored to strengthen type-safety and unify how parsing state is handled, improving consistency and reducing subtle parsing inconsistencies without changing external routing behavior.
  • Tests

    • Parsing-related tests were updated to reflect the new internal parsing approach and ensure continued correctness.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

parseSegment's signature was changed from returning a ParsedSegment to an assertion (: asserts output is ParsedSegment). Call sites were updated to drop return-value assignments and rely on in-place mutation of the provided output buffer. Helper parameter renamed and internal returns converted to plain return.

Changes

Cohort / File(s) Summary
Function signature refactoring
packages/router-core/src/new-process-route-tree.ts
Changed parseSegment(path: string, start: number, output: Uint16Array = new Uint16Array(6)) return to : asserts output is ParsedSegment; replaced internal return output with plain return; renamed helper parameter from data to segment.
Call-site updates: runtime
packages/router-core/src/path.ts
Removed assignments that captured parseSegment's return value; now call parseSegment(...) and rely on the mutated segment buffer.
Call-site updates: tests
packages/router-core/tests/optional-path-params.test.ts, packages/router-core/tests/optional-path-params-clean.test.ts, packages/router-core/tests/path.test.ts
Replaced data = parseSegment(...) with parseSegment(...) in loops and test logic to depend on in-place mutation of the data buffer.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant parseSegment
    participant OutputBuffer

    rect rgb(230,240,250)
    Note over Caller,parseSegment: Old flow (return-based)
    Caller->>parseSegment: parseSegment(path, start, output)
    parseSegment->>OutputBuffer: mutate output buffer
    parseSegment-->>Caller: return modified buffer
    Caller->>Caller: assign returned buffer to local variable
    end

    rect rgb(240,250,240)
    Note over Caller,parseSegment: New flow (assert-based)
    Caller->>parseSegment: parseSegment(path, start, output)
    parseSegment->>OutputBuffer: mutate output buffer + assert type
    parseSegment-->>Caller: no value returned (type asserted)
    Caller->>OutputBuffer: access output directly (type narrowed)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas needing attention:
    • new-process-route-tree.ts changes to ensure all internal paths still populate the provided buffer correctly.
    • Tests updated to rely on mutation — verify no places expect a new instance.
    • Confirm TypeScript assertion narrows types in all call contexts.

Possibly related PRs

Suggested reviewers

  • schiller-manuel
  • nlynzaad

Poem

🐇
I hopped through bytes and tiny seams,
Returned no more — I assert my dreams.
A buffer shaped with gentle paws,
Types snugged tight without applause.
Hop on, compile — the rabbit chews the beams.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main refactoring: using TypeScript's asserts feature to assert buffer type without returning it, which is the primary change across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 refactor-router-core-parse-segment-infer-type-without-returning

📜 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 b9f2f02 and 4e8cce7.

📒 Files selected for processing (1)
  • packages/router-core/src/path.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
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-09-22T00:56:53.426Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.

Applied to files:

  • packages/router-core/src/path.ts
📚 Learning: 2025-09-22T00:56:49.237Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.

Applied to files:

  • packages/router-core/src/path.ts
📚 Learning: 2025-10-01T18:30:26.591Z
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.

Applied to files:

  • packages/router-core/src/path.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:

  • packages/router-core/src/path.ts
🧬 Code graph analysis (1)
packages/router-core/src/path.ts (1)
packages/router-core/src/new-process-route-tree.ts (1)
  • parseSegment (54-150)
⏰ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 19, 2025

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

To disable these notifications, a workspace admin can disable them in workspace settings.


View your CI Pipeline Execution ↗ for commit 4e8cce7

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ❌ Failed 1h 59m 31s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 27s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-19 22:49:05 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 19, 2025

More templates

@tanstack/arktype-adapter

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

@tanstack/directive-functions-plugin

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

@tanstack/eslint-plugin-router

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

@tanstack/history

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

@tanstack/nitro-v2-vite-plugin

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

@tanstack/react-router

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

@tanstack/react-router-devtools

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

@tanstack/react-router-ssr-query

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

@tanstack/react-start

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

@tanstack/react-start-client

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

@tanstack/react-start-server

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

@tanstack/router-cli

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

@tanstack/router-core

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

@tanstack/router-devtools

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

@tanstack/router-devtools-core

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-ssr-query-core

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

@tanstack/router-utils

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

@tanstack/router-vite-plugin

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

@tanstack/server-functions-plugin

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

@tanstack/solid-router

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

@tanstack/solid-router-devtools

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

@tanstack/solid-router-ssr-query

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

@tanstack/solid-start

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

@tanstack/solid-start-client

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

@tanstack/solid-start-server

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

@tanstack/start-client-core

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

@tanstack/start-plugin-core

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

@tanstack/start-server-core

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

@tanstack/start-static-server-functions

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

@tanstack/start-storage-context

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

@tanstack/valibot-adapter

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

@tanstack/virtual-file-routes

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

@tanstack/zod-adapter

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

commit: 4e8cce7

@Sheraff Sheraff closed this Nov 19, 2025
@Sheraff Sheraff deleted the refactor-router-core-parse-segment-infer-type-without-returning branch November 19, 2025 22:02
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