Fix and improve `split-issue-pr-search-results` by dertieran · Pull Request #3837 · refined-github/refined-github

dertieran · 2020-12-22T18:31:25Z

Steps to test as mentioned in the issue:

Visit https://github.com/search?q=refined+github&type=Issues
Click Pull requests (and wait for load)
Click Issues

The problem was that createUrl always appended the next is:<type> query part.
This is also used by Github for the filter and every previous is:<type> is ignored.
But the detection did just check for the first occurence of is:<type> so the ui wasn't updated correctly.

I changed the code to first clear all is:<type> and then add the new filter.
I also improved the detection if multiple is:<types> are in the search.
As a bonus I also added that the language and state search parameters are also copied for each url

fregante · 2020-12-22T19:41:22Z

Good catch. The code is a little verbose but I haven’t looked exactly at how it can be improved

source/features/split-issue-pr-search-results.tsx

This cleans up the initial search query string

The search type is only capitalized in GHE

fregante

Untested but it looks right!

Can you add some tests to ensure that the query is cleaned correctly? Also add some tricky queries to ensure that they're not broken by mistake.

dertieran · 2021-01-08T09:17:33Z

@fregante Sure, just not sure what a "tricky" query would be 😅
Right now I added a test that uses the query from the issue ?q=refined+github and the .add('is:issue').
I guess that's fairly simple, would tricky be something with multiple is:pr/issue?

Also I noticed that before the passed URLSearchParams where just assigned, I changed it that it now creates a new copy, should I change that back?

Edit: Just testing something for another PR

// https://github.com/fregante

fregante · 2021-01-08T09:59:50Z

I just realized that the regex is acting on the raw string like it did at the beginning of the PR. I’d rather use getQueryParts as previously suggested

SearchQuery was added specifically to avoid having to reparse the query (…)

This way you don’t also have drop duplicate spaces because it already does that.

dertieran · 2021-01-08T10:19:08Z

I just realized that the regex is acting on the raw string like it did at the beginning of the PR

So you mean something like this?

set(query) {
	const parts = splitQueryParts(query);
	this.searchParams.set('q', deduplicate(parts).join(' '));
}

dertieran · 2021-01-08T14:05:25Z

Okay this now are changes then I initially intended 😅
But I now also removed to TODO for supporting spaces in quoted query strings.

(Not sure if this is the cleanest code for deduplicating the query parts.)

fregante · 2021-01-08T23:12:13Z

test/search-query.ts


 test('.getQueryParts', t => {
-	const query = new SearchQuery(new URLSearchParams('?q=cool+is%3Aissue'));
+	const query = new SearchQuery('?q=cool+is%3Aissue');


I'd rather see a more readable

Suggested change

const query = new SearchQuery('?q=cool+is%3Aissue');

const query = new SearchQuery(['q', 'cool is:issue']);

Sure, so just accept all URLSearchParams options?

URLSearchParams(init?: string | string[][] | Record<string, string> | URLSearchParams)

Would be this then

new SearchQuery([['q', 'cool is:issue']]);

But it would also work to just pass the not escaped string if it is just for readability in the tests

new SearchQuery('q=cool is:issue');

I just changed SearchQuery to accept all URLSearchParams options.
And then used the {q: 'query'} parameter (thought it was more readable than the entries option).

Hope that is okay otherwise I can also change it back or to something else.

fregante · 2021-01-12T07:59:28Z

source/github-helpers/search-query.ts

+	let indexToKeep = -1;
+	for (const value of values) {
+		const lastIndex = array.lastIndexOf(value);
+		if (lastIndex > indexToKeep) {
+			indexToKeep = lastIndex;
+		}
+	}
+
+	if (indexToKeep === -1) {
+		return array;
+	}
+
+	return array.filter((value, index) => index >= indexToKeep || !values.includes(value));


This uses only a loop:

Suggested change

let indexToKeep = -1;

for (const value of values) {

const lastIndex = array.lastIndexOf(value);

if (lastIndex > indexToKeep) {

indexToKeep = lastIndex;

}

}

if (indexToKeep === -1) {

return array;

}

return array.filter((value, index) => index >= indexToKeep || !values.includes(value));

let wasKeywordFound = false;

const deduplicated = []

for (const current of array.reverse()) {

const isKeyword = keywords.includes(current);

if (!isKeyword || !wasKeywordFound) {

deduplicated.unshift(current);

wasKeywordFound = wasKeywordFound || isKeyword;

}

}

return deduplicated;

I think this might be a bit "slower" in some edgcases (e.g. if the keywords are not in the array at all) but it's more readable 👍

fregante · 2021-01-17T10:14:37Z

Thanks for fixing this!

dertieran added 3 commits December 22, 2020 19:03

Clear search query before adding filter

64f727a

Improve search query detection for multiple filters

84e86b4

Copy the language and state search params

3c3cf7c

fregante reviewed Dec 22, 2020

View reviewed changes

source/features/split-issue-pr-search-results.tsx Outdated Show resolved Hide resolved

fregante reviewed Dec 22, 2020

View reviewed changes

source/features/split-issue-pr-search-results.tsx Outdated Show resolved Hide resolved

fregante reviewed Dec 22, 2020

View reviewed changes

source/features/split-issue-pr-search-results.tsx Outdated Show resolved Hide resolved

fregante reviewed Dec 22, 2020

View reviewed changes

source/features/split-issue-pr-search-results.tsx Outdated Show resolved Hide resolved

yakov116 added bug enhancement labels Dec 22, 2020

fregante marked this pull request as draft December 27, 2020 08:47

dertieran added 7 commits December 31, 2020 13:22

Update the link element instead of creating a url

907dec6

Deduplicate the search query when set

98c61c0

Stop accepting raw location in search query

ce0ccbd

Set the search query initially

27a8163

This cleans up the initial search query string

Use includes for detecting pr or issue queries

fd16069

Check search type with lowercase

1431291

The search type is only capitalized in GHE

Improve search query deduplication regexp

4e5fb96

dertieran marked this pull request as ready for review January 7, 2021 11:56

fregante approved these changes Jan 8, 2021

View reviewed changes

dertieran added 2 commits January 8, 2021 10:18

Add test for deduplicating is:pr/issue queries

aa286aa

Improve search query deduplication regexp even more

730510b

dertieran added 4 commits January 8, 2021 14:12

Use query parts to clean query string

e2d6990

Add test for removing additional spaces

f9fd7b1

Just use query strings in tests

19ec87c

Add support for query string values with spaces

79c1e6f

fregante reviewed Jan 8, 2021

View reviewed changes

Geeyun-JY3 mentioned this pull request Jan 10, 2021

Some border issues StylishThemes/GitHub-Dark#1287

Open

dertieran added 2 commits January 11, 2021 15:44

Allow all URLSearchParams options in SearchQuery

7d11c35

Use query object in search query tests

2d629cd

fregante reviewed Jan 12, 2021

View reviewed changes

Make deduplicate function more readable

70305e6

fregante merged commit 413a461 into refined-github:master Jan 17, 2021

dertieran deleted the fix-split-issue-pr-search-results branch January 17, 2021 10:20

fregante mentioned this pull request Jan 18, 2021

bugs-tab: Cannot read property 'pathname' of undefined #3897

Closed

fregante mentioned this pull request Jul 27, 2021

Include other bug labels in bugs-tab #4592

Merged

	const query = new SearchQuery('?q=cool+is%3Aissue');
	const query = new SearchQuery(['q', 'cool is:issue']);

Uh oh!

Conversation

dertieran commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Dec 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

dertieran commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dertieran commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dertieran commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

dertieran Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

dertieran Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

fregante Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dertieran Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante commented Jan 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dertieran commented Dec 22, 2020 •

edited

Loading

dertieran commented Jan 8, 2021 •

edited

Loading

fregante commented Jan 8, 2021 •

edited

Loading

dertieran commented Jan 8, 2021 •

edited

Loading

dertieran commented Jan 8, 2021 •

edited

Loading

fregante Jan 12, 2021 •

edited

Loading

dertieran Jan 12, 2021 •

edited

Loading