Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the redundant navigation error #2286

Merged
merged 5 commits into from
Jun 21, 2022

Conversation

absidue
Copy link
Member

@absidue absidue commented Jun 1, 2022


Fix the redundant navigation error

Pull Request Type

  • Bugfix

Description

Fixes the issue where spamming the nav buttons throws vue errors about redundant navigations.

Screenshots (if appropriate)
duplicate-navigation-error

Testing (for code that is not small enough to be easily understandable)
I spammed the nav buttons and the error didn't occur anymore.

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 7287e00

Additional context
Add any other context about the problem here.

@PrestonN PrestonN enabled auto-merge (squash) June 1, 2022 21:40
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 1, 2022
@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jun 1, 2022

Error still appears when clicking the profile select's setting icon
image

@absidue
Copy link
Member Author

absidue commented Jun 2, 2022

@ChunkyProgrammer Thanks for noticing that, should be fixed now.

@ChunkyProgrammer
Copy link
Member

Was able to have this error happen again when searching (search the same thing twice)

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 4, 2022
@vallode
Copy link
Contributor

vallode commented Jun 5, 2022

Potentially we need to hijack the prototype of the vue-router push function here? I mean is it feasible to expect every single next router interaction to always remember to put this if statement in place?

@absidue
Copy link
Member Author

absidue commented Jun 5, 2022

I thought about that but I haven't looked into the source code for that yet (if you want to take a look remember to look at the source code for the version that FreeTube uses). Although I'll need to expand the checks to also check the query parameters etc.

@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jun 9, 2022
@absidue
Copy link
Member Author

absidue commented Jun 9, 2022

I've now removed the individual checks and wrapped the router push function in one that checks the path and query before navigating.

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Jun 10, 2022

Can we have a summary of what to be tested?

  • spamming the nav buttons
  • clicking the profile select's setting icon
  • search the same thing twice
  • Paste a YT Video URL to navbar, press arrow button multiple times
  • Anything else?

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested

  • spamming the nav buttons
  • clicking the profile select's setting icon
  • search the same thing twice
  • Paste a YT Video URL to navbar, press arrow button multiple times

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Cleanup duplicate statements (especially conditional)

isQueryEmpty can also be removed

src/renderer/router/index.js Outdated Show resolved Hide resolved
@absidue absidue requested a review from PikachuEXE June 11, 2022 21:48
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@efb4f5ff-1298-471a-8973-3d47447115dc

@ChunkyProgrammer sorry for the spam :D

@PikachuEXE
Copy link
Collaborator

@efb4f5ff-1298-471a-8973-3d47447115dc
Why Is It Called Spam? - https://youtu.be/Wo9QkKxTT1w

@PrestonN PrestonN merged commit 5643419 into FreeTubeApp:development Jun 21, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 21, 2022
@absidue absidue deleted the fix-duplicate-navigation branch June 30, 2022 20:26
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.

6 participants