fix(router-core): correctly resolve custom params from declarative masks when building the masked url #5756
Conversation
WalkthroughRoute masking now applies per-mask parameter customization: the router reads a mask's Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant MaskDef as MaskDefinition
participant Builder as MaskedDestinationBuilder
Client->>Router: navigate(pathname, params)
Router->>MaskDef: find matching mask for pathname
alt mask found
MaskDef-->>Router: return mask (from,to,maskParams,maskProps)
Router->>Router: evaluate maskParams
alt maskParams === false or null
Router-->>Router: nextParams = {}
else maskParams === true or undefined
Router-->>Router: nextParams = current params
else maskParams is function/object
Router-->>Router: nextParams = functionalUpdate(maskParams, current params)
end
Router->>Builder: build maskedDest (to-pathname, params = nextParams, attach maskProps/search/hash/state/from)
Builder-->>Router: masked destination (maskedLocation, href)
Router-->>Client: return maskedLocation (original destination retained for navigation)
else no mask
Router-->>Client: return original destination
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-core/src/router.ts (2)
1800-1801: Clarify the comment to cover all maskParams cases.The comment mentions "params function" but the logic handles multiple scenarios (false, null, true, undefined, function, object). Consider updating it to describe all cases for better maintainability.
- // If mask has a params function, call it with the matched params as context - // Otherwise, use the matched params or the provided params value + // Compute params for the masked destination: + // - false/null: no params + // - true/undefined: use matched params + // - function/object: merge matched params with the result of functionalUpdate
1807-1807: Consider using spread operator to avoid mutation.
Object.assign(params, ...)mutates theparamsobject. While this may not cause issues in the current scope, using the spread operator would be safer and align better with immutability principles.- : Object.assign(params, functionalUpdate(maskParams, params)) + : { ...params, ...functionalUpdate(maskParams, params) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/router.ts(1 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/router-core/src/router.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.ts
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (1)
packages/router-core/src/utils.ts (1)
functionalUpdate(203-212)
|
we definitely need tests here. ideally start with the tests first and then the solution |
|
@schiller-manuel thanks for the reply. Since I couldn't find any tests for masks specifically in the router-core I added a new file and tried to cover different cases with masks and params. Let me know what you think! |
|
View your CI Pipeline Execution ↗ for commit 913be4e
☁️ Nx Cloud last updated this comment at |
|
Hi @schiller-manuel, just checking in on this PR. Any sense of whether it might be merged or if I should work around it? Thanks! |
|
Hi @hakanshehu thanks for this, I had a quick look through and it seems good. I want to have thorough look at it tomorrow to make sure I understand the problem and proposed solution fully and will advise. |
nlynzaad
left a comment
nlynzaad
left a comment
There was a problem hiding this comment.
code changes looks good and overall the tests are quite extensive. there are a few comments on the tests we need to resolve.
If possible I would like it if we can add some e2e tests on react and solid to confirm the final outcome not just on the pathname but also on the params. Doesn't need to be as extensive as the the ones in mask.test.ts, just a quick check that the final output delivers what we expect after its gone through all the other code paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/solid-router/basic-file-based/src/main.tsx (1)
12-21: Consider adding type safety and validation to the params function.The
routeTree: null as anytype assertion bypasses type checking. Additionally, the params function assumesprev.userIdexists without validation, which could cause runtime errors if the parameter is missing.Apply this diff to add defensive checks:
routeMasks: [ { - routeTree: null as any, + routeTree: routeTree, from: '/masks/admin/$userId', to: '/masks/public/$username', - params: (prev: any) => ({ - username: `user-${prev.userId}`, - }), + params: (prev: any) => { + if (!prev?.userId) { + throw new Error('userId parameter is required for mask transformation') + } + return { + username: `user-${prev.userId}`, + } + }, }, ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
e2e/react-router/basic-file-based/src/main.tsx(1 hunks)e2e/react-router/basic-file-based/src/routeTree.gen.ts(21 hunks)e2e/react-router/basic-file-based/src/routes/__root.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/masks.public.$username.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/masks.tsx(1 hunks)e2e/react-router/basic-file-based/tests/mask.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/main.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(21 hunks)e2e/solid-router/basic-file-based/src/routes/__root.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/masks.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/mask.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/solid-router/basic-file-based/src/routes/masks.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
e2e/solid-router/basic-file-based/src/routes/__root.tsxe2e/solid-router/basic-file-based/src/main.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/react-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/solid-router/basic-file-based/tests/mask.spec.tse2e/react-router/basic-file-based/tests/mask.spec.tse2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/main.tsxe2e/react-router/basic-file-based/src/routes/masks.tsxe2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx
**/src/routes/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use file-based routing in src/routes/ directories or code-based routing with route definitions
Files:
e2e/solid-router/basic-file-based/src/routes/__root.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/react-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/routes/masks.tsxe2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx
🧠 Learnings (8)
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to packages/solid-router/**/*.{ts,tsx} : Solid Router components and primitives should use the tanstack/solid-router package
Applied to files:
e2e/solid-router/basic-file-based/src/routes/__root.tsxe2e/solid-router/basic-file-based/src/main.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/solid-router/basic-file-based/tests/mask.spec.tse2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/routes/masks.tsxe2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to **/src/routes/**/*.{ts,tsx} : Use file-based routing in src/routes/ directories or code-based routing with route definitions
Applied to files:
e2e/solid-router/basic-file-based/src/routes/__root.tsxe2e/solid-router/basic-file-based/src/main.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/react-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/main.tsxe2e/react-router/basic-file-based/src/routes/masks.tsxe2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to packages/react-router/**/*.{ts,tsx} : React Router components and hooks should use the tanstack/react-router package
Applied to files:
e2e/solid-router/basic-file-based/src/routes/__root.tsxe2e/solid-router/basic-file-based/src/main.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/react-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/main.tsxe2e/react-router/basic-file-based/src/routes/masks.tsxe2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx
📚 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:
e2e/solid-router/basic-file-based/src/main.tsxe2e/react-router/basic-file-based/src/main.tsx
📚 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/main.tsxe2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsxe2e/solid-router/basic-file-based/tests/mask.spec.tse2e/react-router/basic-file-based/tests/mask.spec.tse2e/react-router/basic-file-based/src/routes/__root.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/tests/mask.spec.tse2e/react-router/basic-file-based/tests/mask.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.tse2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
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 (3)
e2e/solid-router/basic-file-based/src/routes/__root.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
e2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx (3)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
Route(12-22)e2e/react-router/basic-file-based/src/routes/masks.public.$username.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/masks.tsx (1)
Route(8-10)
🔇 Additional comments (12)
e2e/react-router/basic-file-based/src/routes/__root.tsx (2)
142-148: Verify the ts-expect-error removal is intentional.The removal of the
ts-expect-errordirective for the non-existent route link appears unrelated to the route masking feature. This change will now expose a TypeScript error for a route that doesn't exist in the route tree, which may be intentional to catch invalid routes, but should be confirmed.
149-157: LGTM!The new Masks navigation link is properly structured with a test ID and follows the same pattern as the other navigation links in the root component.
e2e/react-router/basic-file-based/src/routes/masks.public.$username.tsx (1)
1-15: LGTM!The public username route is properly structured:
- Correct file-based route definition at
/masks/public/$username- Component correctly reads and displays the
usernameparam usingRoute.useParams()- Test IDs are in place for e2e testing
e2e/react-router/basic-file-based/src/routes/masks.tsx (1)
1-38: LGTM!The Masks layout route is well-structured for testing masking behavior:
- Correctly uses
useRouterStateto access location information- Displays both the actual pathname and the masked pathname for verification
- Link to the admin route properly passes the
userIdparam- Safe access to
maskedLocationusing optional chaining- Test IDs enable proper e2e test assertions
e2e/react-router/basic-file-based/tests/mask.spec.ts (1)
1-26: LGTM!The e2e test properly validates the route masking behavior:
- Tests the complete navigation flow from root to masked route
- Verifies URL transformation from
/masks/admin/$userIdto/masks/public/$username- Confirms the admin component renders with correct userId (42)
- Asserts both the actual pathname (
/masks/admin/42) and masked pathname (/masks/public/user-42) are displayed correctly- Uses appropriate Playwright assertions and wait strategies
e2e/react-router/basic-file-based/src/routes/masks.admin.$userId.tsx (1)
1-15: LGTM!The admin user route is correctly implemented:
- Proper file-based route definition at
/masks/admin/$userId- Component reads and displays the
userIdparam usingRoute.useParams()- Test IDs (
admin-user-component,admin-user-id) are in place for e2e testing- Mirrors the structure of the public username route consistently
e2e/react-router/basic-file-based/src/main.tsx (1)
13-22: Clarify therouteTree: null as anysetting.The route mask configuration correctly transforms params from
userIdtousernamewith theuser-prefix. However, settingrouteTree: null as anyon Line 15 requires clarification.Is this a placeholder for future functionality, or is the route tree not required for mask definitions? Consider adding a comment explaining why this is null.
Otherwise, the params transformation logic looks correct and aligns with the test expectations (userId: '42' → username: 'user-42').
e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
1-2537: Skipping review of autogenerated file.This file is automatically generated by TanStack Router and should not be manually modified or reviewed for issues, as indicated by the file header comments.
Based on learnings, autogenerated
routeTree.gen.tsfiles are excluded from review.e2e/solid-router/basic-file-based/src/routes/__root.tsx (1)
149-158: LGTM!The new navigation link follows the established pattern and is properly configured with test ID and active styling.
e2e/solid-router/basic-file-based/tests/mask.spec.ts (1)
1-26: LGTM! Comprehensive test coverage for route masking.The test validates the core functionality of the PR by verifying:
- Parameter transformation (userId 42 → username user-42)
- URL masking (admin path → public path in browser)
- Both actual and masked pathname exposure
e2e/solid-router/basic-file-based/src/routes/masks.public.$username.tsx (1)
1-15: LGTM! Clean route implementation.The file follows Solid Router conventions correctly, using
createFileRouteand properly accessing route params via the signal-basedparams()accessor.e2e/solid-router/basic-file-based/src/routes/masks.admin.$userId.tsx (1)
1-15: LGTM! Clean route implementation.The file correctly implements a Solid Router file-based route with proper param handling and test IDs that align with the e2e test expectations.
|
Hi @nlynzaad thanks for the feedback. I addressed the comments in the core mask tests. I've also added some routes and a simple e2e test for react and solid. Let me know if I should use another approach or improve anything for the new tests. |
nlynzaad
changed the title
…sks when building the masked url (TanStack#5756) * Use params from declarative masks * Add tests for masks * Include the necessary changes * Update mask tests * Add e2e tests * minor nitpicks --------- Co-authored-by: Nico Lynzaad <nlynzaad@zylangroup.com>
3 participants
This PR fixes #5755 , where custom params passed to declarative route masks are ignored. When the masked params differ from the original URL params, the masked URL can display
undefinedinstead of the expected values.I don't have full context on all internal router behaviors, so please treat this as a proposal. If there's a better approach, I’m happy to adjust the implementation.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.