fix(router-core): allow to return false to skip navigation using view transition types
#5492
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe router's view-transition Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Router
participant V as types resolver
participant S as startViewTransition
participant U as Update callback
R->>V: resolved = types(locationChangeInfo)
alt resolved === false
note right of R#lightblue: Short-circuit (new)
R->>U: run update immediately
note right of R#lightblue: skip startViewTransition
else resolved is string[]
R->>S: startViewTransition(() => U())
S-->>R: transition completes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
2178-2181: LGTM! Short-circuit logic is correct.The strict equality check and immediate execution correctly implement the skip behavior when the types resolver returns
false. The implementation properly bypassesstartViewTransitionand executes the update function directly.Consider adding a brief comment explaining the short-circuit behavior:
+ // Skip transition if types resolver returns false if (resolvedViewTransitionTypes === false) { fn() return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/router.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:
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
🔇 Additional comments (1)
packages/router-core/src/router.ts (1)
768-778: LGTM! Type signature correctly reflects the new skip behavior.The addition of
falseas a possible return type is backward compatible and clearly documents that the types function can now signal to skip the transition entirely.
|
can you please update the docs? |
|
View your CI Pipeline Execution ↗ for commit fc8f773 ☁️ Nx Cloud last updated this comment at |
SeanCassiere
changed the title
false to skip navigation using view transition types
Sure! Done. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/router/framework/react/api/router/NavigateOptionsType.md (1)
47-56: Incomplete documentation of newfalsereturn capability.Line 53 updates the description but doesn't explicitly mention that the
typesfunction can now returnfalseto skip the view transition. The current wording only mentions thattypes"will determine the strings array," which omits the new short-circuit behavior.Additionally, the sentence structure is convoluted with two separate conditions about browser support (starting with "If the browser does not support..."), making it unclear which fallback applies in which scenario. Consider restructuring this section to:
- Clearly state that
typescan return either an array of strings orfalseto skip the transition- Separate the browser support fallback logic more clearly (distinguish between support for
startViewTransitionitself vs. support for types)Suggested revision for line 53:
- If [`ViewTransitionOptions`](../ViewTransitionOptionsType.md), route navigations will be called using `document.startViewTransition({update, types})` where `types` will determine the strings array passed with `ViewTransitionOptions["types"]`. If the browser does not support viewTransition types, the navigation will fall back to normal `document.startTransition()`, same as if `true` was passed. + If [`ViewTransitionOptions`](../ViewTransitionOptionsType.md), the `types` option determines the strings array (or can return `false` to skip the transition entirely) passed to `document.startViewTransition({update, types})`. If the browser does not support the View Transitions API, this option is ignored and navigation proceeds normally.
🧹 Nitpick comments (1)
docs/router/framework/react/api/router/ViewTransitionOptionsType.md (1)
27-39: Minor formatting inconsistency and potential missing punctuation.The documentation clearly explains the updated
typesproperty and its dual return capability. However, there are minor issues:
Line 39 appears incomplete: The text ends with "or
falseto cancel the view transition" without a closing period. This should conclude the list item.Consistency: Consider whether the term "cancel" (used here) should align more closely with the terminology used elsewhere in the codebase. The PR description mentions "skip navigation" while this doc uses "cancel the view transition"—both are correct but subtle terminology differences could create confusion.
Formatting: Lines 36 and 38 both reference
document.startViewTransition({update, types}) callwith identical wording. This is fine for clarity, but the nested bullet structure could be more visually distinct.Consider closing line 39 with a period and optionally expanding the explanation slightly:
- - or `false` to cancel the view transition + - or `false` to skip the view transition (the update will run immediately without starting a transition)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/router/framework/react/api/router/NavigateOptionsType.md(1 hunks)docs/router/framework/react/api/router/ViewTransitionOptionsType.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Use internal docs links relative to the docs/ folder (e.g., ./guide/data-loading)
Files:
docs/router/framework/react/api/router/ViewTransitionOptionsType.mddocs/router/framework/react/api/router/NavigateOptionsType.md
docs/{router,start}/**
📄 CodeRabbit inference engine (AGENTS.md)
Place router docs under docs/router/ and start framework docs under docs/start/
Files:
docs/router/framework/react/api/router/ViewTransitionOptionsType.mddocs/router/framework/react/api/router/NavigateOptionsType.md
🪛 LanguageTool
docs/router/framework/react/api/router/ViewTransitionOptionsType.md
[grammar] ~36-~36: There might be a mistake here.
Context: ...y of strings that will be passed to the document.startViewTransition({update, types}) call - A function that accepts `locationChangeI...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...onChangeInfo` object and returns either: - An array of strings that will be passed ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...y of strings that will be passed to the document.startViewTransition({update, types}) call - or false to cancel the view transition...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...or false to cancel the view transition
(QB_NEW_EN)
docs/router/framework/react/api/router/NavigateOptionsType.md
[style] ~53-~53: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... with ViewTransitionOptions["types"]. If the browser does not support viewTransi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~53-~53: Ensure spelling is correct
Context: ...pes"]`. If the browser does not support viewTransition types, the navigation will fall back to...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~53-~53: There might be a mistake here.
Context: ...k to normal document.startTransition(), same as if true was passed. - If the ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...sition(), same as if true` was passed. - If the browser does not support this api...
(QB_NEW_EN)
schiller-manuel
changed the title
false to skip navigation using view transition typesfalse to skip navigation using view transition types
2 participants
…p transition
Summary by CodeRabbit
New Features
Improvements
Documentation