fix: allow typed NavigationItem as TNavItem#18
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis pull request introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/navigation-item-base.test.ts (1)
93-98: Clarify thenevervariance wording.The concrete ctx type is a supertype of
never, not “narrower thannever”. The comment below already has the right reasoning, so this is just a wording fix.Proposed wording tweak
- it("accepts a `to` function whose ctx is narrower than `never`", () => { + it("accepts a `to` function whose ctx is a supertype of `never`", () => { // Function parameters are contravariant — any concrete ctx widens to // satisfy `(ctx: never) => string`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/navigation-item-base.test.ts` around lines 93 - 98, Update the inline comment in the test case that currently says "accepts a `to` function whose ctx is narrower than `never`" to correctly state that the concrete ctx type is a supertype (or "wider than") `never`; adjust the phrasing near the `it` block and the explanatory line "// Function parameters are contravariant — any concrete ctx widens to satisfy `(ctx: never) => string`." so the wording consistently refers to the concrete ctx being a supertype/wider than `never` (reference: the `it` block and the comment above it in navigation-item-base.test.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/navigation.test.ts`:
- Line 3: The tests reference the NavigationItem type but only import
NavigationItemBase and AnyModuleDescriptor; add NavigationItem to the import
list so TypeScript can resolve the type. Update the import statement that
currently imports AnyModuleDescriptor and NavigationItemBase to also include
NavigationItem (so symbols NavigationItem, NavigationItemBase, and
AnyModuleDescriptor are imported together) and run the tests/TS compile to
confirm the missing type error is resolved.
---
Nitpick comments:
In `@packages/core/src/navigation-item-base.test.ts`:
- Around line 93-98: Update the inline comment in the test case that currently
says "accepts a `to` function whose ctx is narrower than `never`" to correctly
state that the concrete ctx type is a supertype (or "wider than") `never`;
adjust the phrasing near the `it` block and the explanatory line "// Function
parameters are contravariant — any concrete ctx widens to satisfy `(ctx: never)
=> string`." so the wording consistently refers to the concrete ctx being a
supertype/wider than `never` (reference: the `it` block and the comment above it
in navigation-item-base.test.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb2b201c-07b2-41b6-a5e7-8d36fa614222
📒 Files selected for processing (22)
packages/core/src/define-module.tspackages/core/src/index.tspackages/core/src/navigation-item-base.test.tspackages/core/src/navigation.test.tspackages/core/src/navigation.tspackages/core/src/runtime-types.tspackages/core/src/slots.tspackages/core/src/types.tspackages/react-router-core/src/define-module.tspackages/react-router-core/src/index.tspackages/react-router-core/src/types.tspackages/react-router-runtime/src/providers.tsxpackages/react-router-runtime/src/registry.tspackages/react-router-runtime/src/types.tspackages/react/src/index.tspackages/react/src/navigation-context.tsxpackages/tanstack-router-core/src/define-module.tspackages/tanstack-router-core/src/index.tspackages/tanstack-router-core/src/types.tspackages/tanstack-router-runtime/src/providers.tsxpackages/tanstack-router-runtime/src/registry.tspackages/tanstack-router-runtime/src/types.ts
Every `TNavItem extends NavigationItem` constraint resolved to `NavigationItem<string, void, unknown>`, pinning `to` to `string`. Consumers aliasing `NavigationItem<Keys, Ctx, Meta>` couldn't pass it as a type argument without `@ts-expect-error` / `as unknown as`.
53bd1af to
6aad521
Compare
Summary
TNavItem extends NavigationItemresolved toNavigationItem<string, void, unknown>,pinning
totostring. Consumers aliasingNavigationItem<Keys, Ctx, Meta>couldn'tpass it as a type argument without
@ts-expect-error/as unknown asat every callto
createRegistry,useNavigation,defineModule,buildNavigationManifest,AnyModuleDescriptor, andNavigationManifest.Fix
Add
NavigationItemBase— a structural upper bound using(ctx: never) => stringfor the function branch of
to. Function parameters are contravariant, so anyconcrete
(ctx: TContext) => stringsatisfies it. Noany, no soundness loss.Replace every
TNavItem extends NavigationItem→TNavItem extends NavigationItemBaseacross core, react, react-router-{core,runtime}, tanstack-router-{core,runtime}.
Before — consumer forced into workarounds at every call site:
After — clean end-to-end:
Module descriptors are static objects defined at load time (outside React), so
tocan't close over runtime values like
workspaceId. The function form defersinterpolation —
resolveNavHref(item, { workspaceId })supplies the value at rendertime. That pattern is only ergonomic if the type system allows non-void
TContextwithout friction.
Notes
Additive change.
NavigationItemis unchanged; existingto: stringconsumersare unaffected. No runtime change.
Summary by CodeRabbit
Release Notes
New Features
NavigationItemBasetype across core packages, enabling more flexible and compatible navigation item typing throughout the framework.Tests