fix(history): remove visibilitychange listener to prevent focus steal…#2704
fix(history): remove visibilitychange listener to prevent focus steal…#2704Mini-Sylar wants to merge 1 commit into
Conversation
… in Edge Fixes focus stealing in edge, does not affect Safari IOS scroll position
✅ Deploy Preview for vue-router canceled.
|
📝 WalkthroughWalkthroughThis change simplifies scroll state persistence in the Vue Router's HTML5 history implementation. The ChangesScroll State Persistence Simplification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (1)
packages/router/src/history/html5.ts (1)
129-129: 💤 Low valueOptional rename:
beforeUnloadListener→pageHideListener.The function is bound to
pagehide, notbeforeunload. The name is a pre-existing artefact but now that the listener is the only scroll-persistence handler, the mismatch is a bit more prominent.♻️ Proposed rename
- function beforeUnloadListener() { + function pageHideListener() { const { history } = window if (!history.state) return history.replaceState( assign({}, history.state, { scroll: computeScrollPosition() }), '' ) } function destroy() { for (const teardown of teardowns) teardown() teardowns = [] window.removeEventListener('popstate', popStateHandler) - window.removeEventListener('pagehide', beforeUnloadListener) + window.removeEventListener('pagehide', pageHideListener) } // ... - window.addEventListener('pagehide', beforeUnloadListener) + window.addEventListener('pagehide', pageHideListener)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/history/html5.ts` at line 129, Rename the function beforeUnloadListener to pageHideListener to match its actual usage (it is bound to the "pagehide" event and is the only scroll-persistence handler); update the function declaration (beforeUnloadListener) and all references where it's added/removed as an event listener (e.g., addEventListener("pagehide", ...) and removeEventListener calls) to the new name so identifiers remain consistent and descriptive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/router/src/history/html5.ts`:
- Line 129: Rename the function beforeUnloadListener to pageHideListener to
match its actual usage (it is bound to the "pagehide" event and is the only
scroll-persistence handler); update the function declaration
(beforeUnloadListener) and all references where it's added/removed as an event
listener (e.g., addEventListener("pagehide", ...) and removeEventListener calls)
to the new name so identifiers remain consistent and descriptive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c1b8256-e3dc-43b6-9155-274cd48759c9
📒 Files selected for processing (1)
packages/router/src/history/html5.ts
2 participants
Summary
Fixes a regression introduced in Fixes a regression introduced in #2537 - discussed in #2693. where adding a
visibilitychangelistener causes Edge on macOS (and Windows) to steal window focus during development.Root Cause
visibilitychangewas firing too broadly even when a tab goes to the background, including during Vite HMR updates on a background tab. When that happens,history.replaceState()is called, and Edge on macOS activates the window at the OS level in response. Chrome is unaffected by the same call.Fix
Remove the
visibilitychangelistener and rely solely onpagehide, which:Testing
Summary by CodeRabbit