Skip to content

Conversation

@babu-ch
Copy link
Contributor

@babu-ch babu-ch commented Jan 16, 2026

fixes #2601

Summary by CodeRabbit

  • Bug Fixes

    • Improved route-guard registration and lifecycle with kept-alive/shared components so guards are associated with the correct active route, re-registered on reactivation, and emit developer warnings when active records are missing.
  • Tests

    • Added comprehensive tests verifying guard invocation, activation/deactivation counts, reactivation across routes, and onBeforeRouteUpdate/onBeforeRouteLeave behavior for shared KeepAlive components.

✏️ Tip: You can customize this high-level summary in your review settings.

@netlify
Copy link

netlify bot commented Jan 16, 2026

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit c4255ba
🔍 Latest deploy log https://app.netlify.com/projects/vue-router/deploys/696f880321118a0008a57a89

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Refactors guard registration to use a ComputedRef for the active route record so guards can be re-registered when a KeepAlive-wrapped component is reactivated for a different route. Adds tests for onBeforeRouteLeave and onBeforeRouteUpdate covering shared KeepAlive components and tracks activation/deactivation and guard invocation/registration.

Changes

Cohort / File(s) Summary
Guard registration core
packages/router/src/navigationGuards.ts
Accept and use a ComputedRef<RouteRecordNormalized | undefined> when registering guards; resolve the active record at registration and on activation, re-register guards if the active record changes, and emit dev warnings when no active record is found.
onBeforeRouteLeave tests
packages/router/__tests__/guards/onBeforeRouteLeave.spec.ts
Add test "triggers when shared KeepAlive component is reactivated for a different route": introduces SharedComponent with onBeforeRouteLeave, onActivated, onDeactivated, mounts a keep-alive wrapped RouterView, navigates through routes, and asserts guard registration/invocation and activation/deactivation counts; adds imports for onActivated, onDeactivated, and mount; uses RouteRecordRaw.
onBeforeRouteUpdate tests
packages/router/__tests__/guards/onBeforeRouteUpdate.spec.ts
Add test covering onBeforeRouteUpdate with a shared KeepAlive component across routes and query updates; tracks activation/deactivation, simulates navigations and query change to assert guard firing; switches to type RouteRecordRaw import and adds onActivated/onDeactivated imports.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Router as Router
    participant KeepAlive as KeepAlive
    participant Component as Component
    participant GuardRegistry as Guard Registry
    participant RouteRecord as Route Record

    Client->>Router: navigate(to Route A)
    Router->>KeepAlive: render/update <rgba(100,150,240,0.5)>
    KeepAlive->>Component: mount / onActivated
    Component->>GuardRegistry: registerGuard(computedActiveRecordRef)
    GuardRegistry->>RouteRecord: resolve currentRecord from ref
    GuardRegistry->>RouteRecord: attach guard to currentRecord

    Client->>Router: navigate(to Route B, same kept component)
    Router->>KeepAlive: update (component kept alive) <rgba(180,120,200,0.5)>
    KeepAlive->>Component: onDeactivated -> onActivated
    Component->>GuardRegistry: resolve currentRecord from ref
    GuardRegistry->>RouteRecord: detect new record
    GuardRegistry->>RouteRecord: remove guard from old, attach to new

    Client->>Router: route params/query update
    Router->>GuardRegistry: invoke guards for currentRecord
    GuardRegistry-->>Router: guard result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 I hopped through routes both near and far,
Kept alive inside a cozy jar,
ComputedRef pointed which record to meet,
Guards hopped along and found their seat,
Reactivated paths now line up neat. 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing navigation guards triggering on keep-alive component reactivation for different routes, which matches the primary objective.
Linked Issues check ✅ Passed The PR addresses issue #2601 by modifying navigation guard registration to use ComputedRef for active route records, ensuring guards trigger on keep-alive reactivation with route changes [#2601].
Out of Scope Changes check ✅ Passed All changes are within scope: test additions verify the fix works for keep-alive reactivation scenarios, and navigationGuards.ts modifications implement the core fix for guard registration with ComputedRef.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 16, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vue-router@2604

commit: 53a641d

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

❌ Patch coverage is 59.37500% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.54%. Comparing base (a705f4f) to head (53a641d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
packages/router/src/navigationGuards.ts 59.37% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2604      +/-   ##
==========================================
+ Coverage   91.27%   91.54%   +0.27%     
==========================================
  Files          46       46              
  Lines        4181     4188       +7     
  Branches     1114     1116       +2     
==========================================
+ Hits         3816     3834      +18     
+ Misses        358      347      -11     
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/router/__tests__/guards/onBeforeRouteLeave.spec.ts`:
- Around line 77-79: Update the Step 2 comment to accurately describe the
navigation: change the text that currently says navigating to `/a/123` to
reflect that the test actually calls `await router.push('/other')` (e.g., "Step
2: Navigate to /other - SharedComponent is deactivated (kept alive); Leave guard
is called when leaving /a"). This ensures the comment next to the `await
router.push('/other')` line matches the action being performed.

In `@packages/router/__tests__/guards/onBeforeRouteUpdate.spec.ts`:
- Around line 120-123: Update the inline comment that currently reads "// BUG:
The guard was registered with /a's record, not /b's record // So when /b
updates, the guard is not called" to clearly state this is the pre-fix behavior
and contrasts with the test assertion; reference the router.push('/b?page=2')
step and onBeforeRouteUpdate guard so the comment reads that before the fix the
guard was registered to /a (so updating /b did not trigger it), whereas the test
now asserts the guard is called after the fix.

@posva posva merged commit c7735d3 into vuejs:main Jan 20, 2026
4 of 5 checks passed
@babu-ch babu-ch deleted the fix/keepalive branch January 21, 2026 04:44
@babu-ch
Copy link
Contributor Author

babu-ch commented Jan 21, 2026

@posva
Thank you for the fine tuning and review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

onBeforeRouteUpdate is not triggered when keep-alive component is enabled.

2 participants