fix(nuxt): hook called twice during navigation #29009

buglavecz · 2024-09-15T14:56:43Z

🔗 Linked issue

resolves #26535

📚 Description

This PR: #25932 (custom loading reset/hide delay + force finish()) solved this early fix: #24931 (stop loading indicator if page keys are the same) issue.
But this PR (#25932) generated another error: hook called twice during navigation.

I have removed this code which is no longer needed and also causes an error.

Summary by CodeRabbit

New Features
- Introduced a new navigation link directing users to a page load hook.
- Added a new Vue component for testing hooks.
- Implemented a Nuxt plugin to handle page loading events and log messages based on the route.
Bug Fixes
- Simplified page loading state management by removing unnecessary logic.
Tests
- Added a test case to ensure the page:loading:end event is triggered only once during navigation.

stackblitz · 2024-09-15T14:56:45Z

Run & review this pull request in StackBlitz Codeflow.

danielroe · 2024-09-16T15:57:31Z

Would you be able to add some regression tests for this? 🙏

(And also to confirm that we don't reintroduce the issue that PR was resolving.)

buglavecz · 2024-10-11T05:00:33Z

@danielroe I did a test

buglavecz · 2024-10-13T17:36:29Z

@danielroe thanks your test refactoring

buglavecz · 2024-10-16T13:51:07Z

@danielroe do you need anything else for this PR?

danielroe · 2024-10-16T14:18:43Z

apologies for not commenting!

i was planning to add a regression test to show that this reintroduces the bug where the loading indicator shows permanently after hitting the browser back button

any chance you could have a look?

buglavecz · 2024-10-17T06:25:12Z

@danielroe
unfortunately, it is unlikely that I will have time for it recently... 😞

coderabbitai · 2024-11-05T19:39:25Z

Walkthrough

The pull request introduces several modifications to the NuxtPage component, including the removal of the previousPageKey variable and the hasChildrenRoutes function. These changes simplify the control flow related to page loading states and child route handling. Additionally, a new test case is added to verify that the page:loading:end event is triggered only once during navigation. New components and a plugin are also introduced for testing and handling page loading events.

Changes

File Path	Change Summary
`packages/nuxt/src/pages/runtime/page.ts`	Removed `previousPageKey` variable and `hasChildrenRoutes` function; adjusted loading state logic.
`test/basic.test.ts`	Added test case to check that `page:loading:end` is triggered only once during navigation.
`test/fixtures/basic/pages/index.vue`	Added a new `<NuxtLink>` element directing to `/page-load-hook`.
`test/fixtures/basic/pages/page-load-hook.vue`	Introduced a new Vue component for hook tests with a basic layout.
`test/fixtures/basic/plugins/page-hook-plugin.ts`	Added a Nuxt plugin to handle `page:loading:end` event and log messages based on the route.

Assessment against linked issues

Objective	Addressed	Explanation
`page:loading:end` hook called twice during navigation (26535)	✅

Possibly related PRs

feat(nuxt): add useRuntimeHook composable #29741: The introduction of the useRuntimeHook composable is related to the changes in the main PR as it involves the registration of runtime hooks, which is relevant to the modifications made to the page.ts file regarding the handling of loading states and events.

Suggested labels

enhancement

Suggested reviewers

danielroe

Poem

In the land of Nuxt, changes abound,
With hooks and links, new paths are found.
A loading event, now just once it sings,
Simplified routes, oh what joy it brings!
So hop along, let the testing commence,
For in this code, we've found our sense! 🐇✨

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

test/fixtures/basic/plugins/page-hook-plugin.ts (2)
1-2: Consider adding TypeScript types for better type safety.

Adding explicit types would improve code maintainability and catch potential issues early.
-export default defineNuxtPlugin((nuxtApp) => {
+export default defineNuxtPlugin((nuxtApp: any) => {
1-8: Add JSDoc documentation to explain the plugin's test purpose.

Since this is a test fixture, adding documentation would help other developers understand its role in testing the hook behavior.
+/**
+ * Test plugin for verifying page:loading:end hook behavior.
+ * Logs hook execution only for the /page-load-hook route.
+ * Used in conjunction with basic.test.ts to ensure the hook fires exactly once.
+ */
 export default defineNuxtPlugin((nuxtApp) => {
test/fixtures/basic/pages/index.vue (1)
97-99: LGTM! Consider adding data-testid for better test targeting.

The new NuxtLink addition is well-placed and follows the existing patterns. To improve test reliability, consider adding a data-testid attribute.
-    <NuxtLink to="/page-load-hook">
+    <NuxtLink 
+      data-testid="page-load-hook-link"
+      to="/page-load-hook"
+    >

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2c0994f and 25a21f4.

📒 Files selected for processing (5)

packages/nuxt/src/pages/runtime/page.ts (0 hunks)
test/basic.test.ts (1 hunks)
test/fixtures/basic/pages/index.vue (1 hunks)
test/fixtures/basic/pages/page-load-hook.vue (1 hunks)
test/fixtures/basic/plugins/page-hook-plugin.ts (1 hunks)

💤 Files with no reviewable changes (1)

packages/nuxt/src/pages/runtime/page.ts

✅ Files skipped from review due to trivial changes (1)

test/fixtures/basic/pages/page-load-hook.vue

🔇 Additional comments (3)

test/fixtures/basic/plugins/page-hook-plugin.ts (2)

1-8: LGTM! The plugin implementation looks correct for testing purposes.

The plugin correctly implements the hook listener for verifying the page:loading:end event behavior.

3-7: Consider adding additional test scenarios.

The current implementation only tests for a single route. Consider expanding the test coverage to include:

Different route scenarios
Navigation timing verification
Hook execution order validation

test/basic.test.ts (1)

629-638: LGTM! Well-structured test case for verifying hook behavior.

The test case effectively verifies that the page:loading:end hook is triggered only once during navigation. It follows testing best practices with proper setup, clear assertions, and cleanup.

danielroe · 2024-11-05T21:37:13Z

to be clear, this causes a regression of #25039 which needs to be resolved before it can be merged.

you can reproduce with https://stackblitz.com/edit/nuxt-starter-r5biil?file=pages%2Fuser.vue.

fix(nuxt): hook called twice during navigation

d5cf708

buglavecz requested a review from danielroe as a code owner September 15, 2024 14:56

github-actions bot added 3.x bug labels Sep 15, 2024

Merge branch 'main' into main

c759cf3

buglavecz added 6 commits October 9, 2024 21:13

Merge branch 'nuxt:main' into main

b73ff33

add unit test

b99d566

Merge branch 'nuxt:main' into main

c90f321

eslint fix

390f88b

Merge branch 'nuxt:main' into main

b5ec8f6

fix test

50255cb

danielroe added 3 commits October 11, 2024 14:07

Merge remote-tracking branch 'origin/main' into buglavecz/main

9e83d05

test: only log on /page-load-hook route

2984d3f

test: trigger logging with client-side navigation

1edb8d2

Merge branch 'nuxt:main' into main

25a21f4

coderabbitai bot reviewed Nov 5, 2024

View reviewed changes

danielroe marked this pull request as draft November 5, 2024 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(nuxt): hook called twice during navigation #29009

fix(nuxt): hook called twice during navigation #29009

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

fix(nuxt): hook called twice during navigation #29009

Are you sure you want to change the base?

fix(nuxt): hook called twice during navigation #29009

Conversation

🔗 Linked issue

📚 Description

Summary by CodeRabbit

Walkthrough

Changes

Assessment against linked issues

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Choose a reason for hiding this comment

CodeRabbit Configuration File (`.coderabbit.yaml`)