fix: router history bouncing back on initial navigation #2585
Conversation
✅ Deploy Preview for vue-router canceled.
|
WalkthroughA test case and refactoring address an initial navigation bug where replaced browser history states are ignored. A new Changes
Sequence DiagramsequenceDiagram
participant App as Vue App
participant Router as Router
participant History as Browser History
rect rgb(200, 220, 255)
Note over App,History: Before: Initial Navigation uses stale location
App->>Router: createRouter()
Router->>History: Read location (caches it)
App->>History: replaceState("/bar")
App->>Router: use(router)
Router->>Router: finalizeNavigation
Router->>History: push(cachedLocation) ✗ Wrong!
end
rect rgb(200, 255, 220)
Note over App,History: After: Initial Navigation reads current location
App->>Router: createRouter()
App->>History: replaceState("/bar")
App->>Router: use(router)
Router->>Router: finalizeNavigation
Router->>History: createCurrentLocation(base, window.location)
Router->>History: replace(currentLocation) ✓ Correct!
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: 0
🧹 Nitpick comments (1)
packages/router/src/router.ts (1)
739-756: Core fix correctly addresses the bouncing-back issue.The implementation properly resolves issue #2584 by using the current browser location (
createCurrentLocation(routerHistory.base, location)) during first navigation instead of a potentially stale cached location. This ensures that anyhistory.replaceStatecalls made between router creation and mounting are respected.Key improvements:
- The condition
replace || isFirstNavigationcorrectly handles both explicit replace operations and initial navigation- The
isBrowsercheck safely guards against SSR environments before accessing the globallocationobject- Existing behavior for subsequent navigations is preserved
Consider renaming
toLocationPathtotoLocationHrefor similar, since it contains the full location (path + search + hash) rather than just the path:- const toLocationPath = + const toLocationHref = isFirstNavigation && isBrowser ? createCurrentLocation(routerHistory.base, location) : toLocation.fullPath routerHistory.replace( - toLocationPath, + toLocationHref,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/router/__tests__/initialNavigation.spec.ts(2 hunks)packages/router/src/history/common.ts(2 hunks)packages/router/src/history/html5.ts(1 hunks)packages/router/src/router.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:22:32.526Z
Learnt from: posva
Repo: vuejs/router PR: 2415
File: packages/router/src/experimental/index.ts:43-44
Timestamp: 2025-08-11T15:22:32.526Z
Learning: In the Vue Router codebase, files within the src directory should not import from src/index to avoid circular dependencies. Deep imports like `../matcher/types` are intentional and necessary for maintaining proper dependency hierarchy.
Applied to files:
packages/router/__tests__/initialNavigation.spec.ts
🧬 Code graph analysis (3)
packages/router/__tests__/initialNavigation.spec.ts (1)
packages/router/__tests__/utils.ts (1)
newRouter(171-178)
packages/router/src/router.ts (2)
packages/router/src/utils/env.ts (1)
isBrowser(1-1)packages/router/src/history/common.ts (1)
createCurrentLocation(192-210)
packages/router/src/history/common.ts (1)
packages/router/src/location.ts (1)
stripBase(153-158)
🔇 Additional comments (6)
packages/router/__tests__/initialNavigation.spec.ts (1)
79-90: LGTM! Test effectively validates the fix.The test case correctly verifies that when
window.history.replaceStateis called after router creation but before the app mounts, the router respects the replaced state rather than bouncing back to a stale location. This directly addresses issue #2584.packages/router/src/history/html5.ts (2)
10-10: Good refactoring to use shared implementation.Importing
createCurrentLocationfrom the common module eliminates code duplication and centralizes location normalization logic.
50-50: Consistent use of shared location normalization.Both call sites correctly use the shared
createCurrentLocationfunction for normalizing browser locations.Also applies to: 163-163
packages/router/src/history/common.ts (2)
2-2: Appropriate import for the new function.The
stripBaseimport is correctly added to support thecreateCurrentLocationimplementation.
187-210: Well-implemented location normalization function.The
createCurrentLocationfunction correctly handles both hash-based and normal routing modes:
- For hash bases, it extracts the path from
location.hashand ensures proper formatting- For normal bases, it strips the base from the pathname and preserves search/hash parameters
This centralized implementation will help maintain consistency across history implementations.
packages/router/src/router.ts (1)
75-75: Appropriate import for initial navigation fix.The
createCurrentLocationimport is correctly added to support capturing the current browser location during first navigation.
1 participant
fix: #2584
In the first navigation, we regard the current browser location as the target, as the state of routerHistory maybe stale when initializing the router.
Summary by CodeRabbit
Bug Fixes
Tests