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

Prevent empty query parameter being set on dashboard #11561

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 22, 2020

Prevent the dashboard from setting an empty query parameter

Fix #11543

Signed-off-by: Andrew Thornton [email protected]

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 22, 2020
if (queryString) {
window.history.replaceState({}, '', `?${queryString}`);
} else {
window.history.replaceState({}, '', window.location.pathname);
Copy link
Member

Choose a reason for hiding this comment

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

Is this branch even necessary? This would kill any #hash present on the URL (thought the branch above would also)

Copy link
Member

Choose a reason for hiding this comment

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

Here's a helper function you can put in utils.js that preserves pathname, search and hash:

function setUrl(opts) {
  const url = new URL(window.location.href);
  if ('pathname' in opts) url.pathname = opts.pathname || '/';
  if ('search' in opts) url.search = opts.search || '';
  if ('hash' in opts) url.hash = opts.hash || '';
  const newLocation = String(url).replace(url.origin, '');
  window.history[opts.push ? 'pushState' : 'replaceState'](null, null, newLocation);
}

You can then call it like this to set/unset only the search part:

setUrl({search: params.toString() || ''});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash is will be out of date and irrelevant if you are pressing these buttons - if anything we should be setting the hash to reference the repo-list table.

Imagine we finally get round to sticking ids on the newsfeed items and you have been sent the newsfeed via an anchor then have scrolled back up to the repo-list - keeping the anchor would mean that on going back you will scroll back to the anchor even though the actual proper context for those query elements is the repo-list.

As the repo-list is the top of the page it's not necessarily needed to set the anchor hash - but - keeping it could be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the hash too with it. This should be equal to your version:

setUrl({search: params.toString() || '', hash: ''});

Your first case would set a wrong path if gitea is running on a subdirectory as it would set path to parent root directory. I think my function is safer because it only sets the parts that you want to change, not all at once.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it seems a replaceState with ?something as URL does not kill the path, at least in Firefox. But I would probably not want rely that this behaviour is the same in all browsers.

Copy link
Member

Choose a reason for hiding this comment

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

So I think your way is actually fine. I did not realize browsers were smart enough to do the right thing for ?search and #hash pseudo urls, but it appears to be working fine because the url standard requires to apply the base url so fragments like this will work.

> new URL('?search', "http:https://www.example.com/dir/?search").href
'http:https://www.example.com/dir/?search'
> new URL((new URL("http:https://www.example.com/dir/")).pathname, "http:https://www.example.com/dir/?search").href
'http:https://www.example.com/dir/'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe! one of the things that I've recently realised is that it is helpful to run your testing instance through a proxy mounted on a suburl (http:https://localhost/gitea) as it catches a lot of simple problems.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 23, 2020
@zeripath
Copy link
Contributor Author

make lg-tm work

@zeripath zeripath merged commit 788b8b1 into go-gitea:master May 24, 2020
@zeripath zeripath deleted the only-show-query-on-dashboard-if-query-empty branch May 24, 2020 13:02
zeripath added a commit to zeripath/gitea that referenced this pull request May 24, 2020
Prevent the dashboard from setting an empty query parameter

Fix go-gitea#11543

Signed-off-by: Andrew Thornton [email protected]
@zeripath zeripath added the backport/done All backports for this PR have been created label May 24, 2020
lafriks pushed a commit that referenced this pull request May 24, 2020
Prevent the dashboard from setting an empty query parameter

Fix #11543

Signed-off-by: Andrew Thornton [email protected]
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
Prevent the dashboard from setting an empty query parameter

Fix go-gitea#11543

Signed-off-by: Andrew Thornton [email protected]
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

? being appended on the frontpage URL
5 participants