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.

Sign up for GitHub

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

Make issue suggestion work for all editors #33340

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Make issue suggestion work for all editors #33340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a310c88
fix
wxiaoguang Jan 21, 2025
e4c8791
fix pathname usage
wxiaoguang Jan 21, 2025
590e794
fix text-expander popup
wxiaoguang Jan 21, 2025
c26dc3f
Merge branch 'main' into fix-suggestion-issue
wxiaoguang Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions web_src/js/features/comp/TextExpander.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import {matchEmoji, matchMention, matchIssue} from '../../utils/match.ts';
import {emojiString} from '../emoji.ts';
import {svg} from '../../svg.ts';
import {parseIssueHref, parseIssueNewHref} from '../../utils.ts';
import {parseIssueHref, parseRepoOwnerPathInfo} from '../../utils.ts';
import {createElementFromAttrs, createElementFromHTML} from '../../utils/dom.ts';
import {getIssueColor, getIssueIcon} from '../issue.ts';
import {debounce} from 'perfect-debounce';

const debouncedSuggestIssues = debounce((key: string, text: string) => new Promise<{matched:boolean; fragment?: HTMLElement}>(async (resolve) => {
let issuePathInfo = parseIssueHref(window.location.href);
if (!issuePathInfo.ownerName) issuePathInfo = parseIssueNewHref(window.location.href);
const issuePathInfo = parseIssueHref(window.location.href);
if (!issuePathInfo.ownerName) {
const repoOwnerPathInfo = parseRepoOwnerPathInfo(window.location.pathname);
issuePathInfo.ownerName = repoOwnerPathInfo.ownerName;
issuePathInfo.repoName = repoOwnerPathInfo.repoName;
// then no issuePathInfo.indexString here, it is only used to exclude the current issue when "matchIssue"
}
if (!issuePathInfo.ownerName) return resolve({matched: false});

const matches = await matchIssue(issuePathInfo.ownerName, issuePathInfo.repoName, issuePathInfo.indexString, text);
Expand Down
5 changes: 5 additions & 0 deletions web_src/js/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ export type RequestOpts = {
data?: RequestData,
} & RequestInit;

export type RepoOwnerPathInfo = {
ownerName: string,
repoName: string,
}

export type IssuePathInfo = {
ownerName: string,
repoName: string,
Expand Down
17 changes: 10 additions & 7 deletions web_src/js/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
basename, extname, isObject, stripTags, parseIssueHref,
parseUrl, translateMonth, translateDay, blobToDataURI,
toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64, isImageFile, isVideoFile, parseIssueNewHref,
toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64, isImageFile, isVideoFile, parseRepoOwnerPathInfo,
} from './utils.ts';

test('basename', () => {
Expand Down Expand Up @@ -45,12 +45,15 @@ test('parseIssueHref', () => {
expect(parseIssueHref('')).toEqual({ownerName: undefined, repoName: undefined, type: undefined, index: undefined});
});

test('parseIssueNewHref', () => {
expect(parseIssueNewHref('/owner/repo/issues/new')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues'});
expect(parseIssueNewHref('/owner/repo/issues/new?query')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues'});
expect(parseIssueNewHref('/sub/owner/repo/issues/new#hash')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues'});
expect(parseIssueNewHref('/sub/owner/repo/compare/feature/branch-1...fix/branch-2')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'pulls'});
expect(parseIssueNewHref('/other')).toEqual({});
test('parseRepoOwnerPathInfo', () => {
expect(parseRepoOwnerPathInfo('/owner/repo/issues/new')).toEqual({ownerName: 'owner', repoName: 'repo'});
expect(parseRepoOwnerPathInfo('/owner/repo/issues/new?query')).toEqual({ownerName: 'owner', repoName: 'repo'});
expect(parseRepoOwnerPathInfo('/owner/repo/releases')).toEqual({ownerName: 'owner', repoName: 'repo'});
expect(parseRepoOwnerPathInfo('/other')).toEqual({});
window.config.appSubUrl = '/sub';
expect(parseRepoOwnerPathInfo('/sub/owner/repo/issues/new#hash')).toEqual({ownerName: 'owner', repoName: 'repo'});
expect(parseRepoOwnerPathInfo('/sub/owner/repo/compare/feature/branch-1...fix/branch-2')).toEqual({ownerName: 'owner', repoName: 'repo'});
window.config.appSubUrl = '';
});

test('parseUrl', () => {
Expand Down
14 changes: 8 additions & 6 deletions web_src/js/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {decode, encode} from 'uint8-to-base64';
import type {IssuePageInfo, IssuePathInfo} from './types.ts';
import type {IssuePageInfo, IssuePathInfo, RepoOwnerPathInfo} from './types.ts';

// transform /path/to/file.ext to file.ext
export function basename(path: string): string {
Expand Down Expand Up @@ -32,16 +32,18 @@ export function stripTags(text: string): string {
}

export function parseIssueHref(href: string): IssuePathInfo {
// FIXME: it should use pathname and trim the appSubUrl ahead
const path = (href || '').replace(/[#?].*$/, '');
const [_, ownerName, repoName, pathType, indexString] = /([^/]+)\/([^/]+)\/(issues|pulls)\/([0-9]+)/.exec(path) || [];
return {ownerName, repoName, pathType, indexString};
}

export function parseIssueNewHref(href: string): IssuePathInfo {
const path = (href || '').replace(/[#?].*$/, '');
const [_, ownerName, repoName, pathTypeField] = /([^/]+)\/([^/]+)\/(issues\/new|compare\/.+\.\.\.)/.exec(path) || [];
const pathType = pathTypeField ? (pathTypeField.startsWith('issues/new') ? 'issues' : 'pulls') : undefined;
return {ownerName, repoName, pathType};
export function parseRepoOwnerPathInfo(pathname: string): RepoOwnerPathInfo {
const appSubUrl = window.config.appSubUrl;
if (appSubUrl && pathname.startsWith(appSubUrl)) pathname = pathname.substring(appSubUrl.length);
const path = (pathname || '').replace(/[#?].*$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

two small problem to point out

  • may we should trust types and not null check pathname. to use empty string can hide callers issue
  • when it is not a repo path, we returns empty object and check if ownerName is exists even though it's type does not say it can be nullable. may we can return null instead null ownerName?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't do that (use null) for 2 reasons:

  1. There was a long argument about returning null or not: Downscale pasted PNG images based on metadata #29123 (review)
  2. The parseIssueHref was already written that way: Fix context popup error #17398 (comment)

To avoid unnecessary arguments and changes, I didn't use null here.

The pathname usage has been improved in e4c8791

const [_, ownerName, repoName] = /([^/]+)\/([^/]+)/.exec(path) || [];
return {ownerName, repoName};
}

export function parseIssuePageInfo(): IssuePageInfo {
Expand Down
Loading