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(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

Merged
merged 7 commits into from
Aug 2, 2024
13 changes: 13 additions & 0 deletions packages/view/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type IDEPort from "ide/IDEPort";
import { useGlobalData } from "hooks";
import { RefreshButton } from "components/RefreshButton";
import type { IDESentEvents } from "types/IDESentEvents";
import type { RemoteGitHubInfo } from "types/RemoteGitHubInfo";

const App = () => {
const initRef = useRef<boolean>(false);
Expand All @@ -39,6 +40,18 @@ const App = () => {
}
}, [handleChangeAnalyzedData, handleChangeBranchList, ideAdapter, setLoading]);

const { setOwner, setRepo } = useGlobalData();
useEffect(() => {
const handleMessage = (event: MessageEvent<RemoteGitHubInfo>) => {
const message = event.data;
setOwner(message.data.owner);
setRepo(message.data.repo);
};

window.addEventListener("message", handleMessage);
Comment on lines +45 to +51
Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분에서 VSCode(Node.js)로부터 message를 받게 되면 message 내의 owner, repo 값으로 useGlobalData hook의 ownerrepo가 update됩니다.

  • update되는 ownerrepo는 이슈의 하이퍼링크를 만드는 데 사용됩니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub정보를 받아오는 방식에 대해서는 좀 논의가 필요할 것 같습니다.
현재 처음 loading할때 engine에서 전체 데이터에 대한 분석을 해서 던지는데,
해당 github 정보도 engine에서 넘어오는 데이터와 생명주기를 같이 하는 것 같으므로,
이렇게 별도로 message를 주고 받는 방식보다는
기존방식으로 동일하게 진행하는게 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

engine에서 넘어오는 데이터와 생명주기를 같이 해야하는 것에 대한 고려를 하지 못했습니다. ㅠㅠ

별도 issue를 만들어서
isPRSuccess, csmDict과 같은 정보를 React 단으로 넘길 때, github 정보도 함께 넘기는 방향으로 수정해보겠습니다!

return () => window.removeEventListener("message", handleMessage);
}, []);

if (loading) {
return (
<BounceLoader
Expand Down
Original file line number Diff line number Diff line change
@@ -1,33 +1,41 @@
import React from "react";
import React, { useEffect, useState } from "react";
import { IoIosArrowDropdownCircle, IoIosArrowDropupCircle } from "react-icons/io";

import { useGlobalData } from "hooks";

import type { ContentProps } from "../Summary.type";

const Content = ({ content, clusterId, selectedClusterId }: ContentProps) => {
const str: string = content.message;
const regex = /^(\(#[0-9]+\)|#[0-9]+)/g;
const tobeStr: string[] = str.split(" ");
const { owner, repo } = useGlobalData();
const [linkedStr, setLinkedStr] = useState<React.ReactNode[]>([]);

useEffect(() => {
const str: string = content.message;
const regex = /^(\(#[0-9]+\)|#[0-9]+)/g;
const tobeStr: string[] = str.split(" ");

const linkedStr = tobeStr.reduce((acc: React.ReactNode[], tokenStr: string) => {
const matches = tokenStr.match(regex); // #num 으로 결과가 나옴 ()가 결과에 포함되지 않음
if (matches) {
const matchedStr = matches[0];
const matchedStrNum: string = matchedStr.substring(1);
const linkIssues = `https://github.com/githru/githru-vscode-ext/issues/${matchedStrNum}`;
acc.push(
<a
href={linkIssues}
key={`issues-${matchedStr}`}
>
{matchedStr}
</a>
);
acc.push(" ");
} else {
acc.push(tokenStr);
acc.push(" ");
}
return acc;
const newLinkedStr = tobeStr.reduce((acc: React.ReactNode[], tokenStr: string) => {
const matches = tokenStr.match(regex); // #num 으로 결과가 나옴 ()가 결과에 포함되지 않음
if (matches) {
const matchedStr = matches[0];
const matchedStrNum: string = matchedStr.substring(1);
const linkIssues = `https://github.com/${owner}/${repo}/issues/${matchedStrNum}`;
acc.push(
<a
href={linkIssues}
key={`issues-${matchedStr}`}
>
{matchedStr}
</a>
);
acc.push(" ");
} else {
acc.push(tokenStr);
acc.push(" ");
}
return acc;
}, []);
setLinkedStr(newLinkedStr);
}, []);
Copy link
Member Author

Choose a reason for hiding this comment

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

AS-IS

linkedStr constant 변수가 커멧 메시지로 DIV 태그에 입력됩니다.

TO-BE

linkedStr 계산 로직을 useEffect 훅 내에서 처리하며, 재렌더링 되더라도 linkedStr은 한 번만 계산됩니다.


return (
Expand Down
8 changes: 7 additions & 1 deletion packages/view/src/context/GlobalDataProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export const GlobalDataProvider = ({ children }: PropsWithChildren) => {

const [branchList, setBranchList] = useState<string[]>([]);
const [selectedBranch, setSelectedBranch] = useState<string>(branchList?.[0]);
const [owner, setOwner] = useState<string>("");
const [repo, setRepo] = useState<string>("");

const handleChangeBranchList = (branches: { branchList: string[]; head: string | null }) => {
setSelectedBranch((prev) => (!prev && branches.head ? branches.head : prev));
Expand Down Expand Up @@ -44,8 +46,12 @@ export const GlobalDataProvider = ({ children }: PropsWithChildren) => {
setSelectedBranch,
handleChangeAnalyzedData,
handleChangeBranchList,
owner,
setOwner,
repo,
setRepo,
}),
[data, filteredRange, filteredData, selectedData, branchList, selectedBranch, loading]
[data, filteredRange, filteredData, selectedData, branchList, selectedBranch, loading, owner, repo]
);

return <GlobalDataContext.Provider value={value}>{children}</GlobalDataContext.Provider>;
Expand Down
4 changes: 4 additions & 0 deletions packages/view/src/hooks/useGlobalData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ type GlobalDataState = {
setBranchList: Dispatch<SetStateAction<string[]>>;
selectedBranch: string;
setSelectedBranch: Dispatch<SetStateAction<string>>;
owner: string;
setOwner: Dispatch<SetStateAction<string>>;
repo: string;
setRepo: Dispatch<SetStateAction<string>>;
} & IDESentEvents;
Comment on lines 24 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

점점 context api 관련 쪽이 번잡해 지는 것으로 보아... 아무래도 상태 관리 외부 lib를 쓸 때가 온 것 같습니다!!! 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

recoil... zustand...👍

Copy link
Member Author

Choose a reason for hiding this comment

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

recoil... zustand...👍

reocoil 사용해봤는데 편하더라구요 ㅎ ㅎ(편하긴 했는데 어떤 사이드 이펙트가 있는지는 잘 모릅니다 !..)


export const GlobalDataContext = createContext<GlobalDataState | undefined>(undefined);
Expand Down
6 changes: 6 additions & 0 deletions packages/view/src/types/RemoteGitHubInfo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface RemoteGitHubInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

RemoteGitHub이라고 하니 Remote가 아닌 GitHub 관련 기능이 있을 것으로 보이기도 하는 것 같습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

아래쪽에서는 GitRemoteConfig 라는 함수를 쓰고 있네요. 기존 것과 통일성이 있으면서도 구분되는 이름으로 하면 좋을 것 같네요.

data: {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, Info 내에 data가 있고, 그 안에 owner, repo가 있는데
property들이 다수 존재해서 뭔가 grouping을 해야 하는 상황이 아니라면,
다소 모호한 의미의 data라는 group name?은 없어도 될 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

메시지를 웹뷰에 날릴 때 data.owner, data.repo 로 그룹핑하여 날려서 type 인터페이스도 동일하게 되었어요 😅

engine에서 데이터 함께 날릴 때, 현재 data로 네이밍되어 있는 부분도 수정하겠습니다! 🫡

owner: string;
repo: string;
};
}
13 changes: 7 additions & 6 deletions packages/vscode/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as vscode from "vscode";
import { COMMAND_LAUNCH, COMMAND_LOGIN_WITH_GITHUB, COMMAND_RESET_GITHUB_AUTH } from "./commands";
import { Credentials } from "./credentials";
import { GithubTokenUndefinedError, WorkspacePathUndefinedError } from "./errors/ExtensionError";
import { deleteGithubToken, getGithubToken, setGithubToken, } from "./setting-repository";
import { deleteGithubToken, getGithubToken, setGithubToken } from "./setting-repository";
Copy link
Member Author

Choose a reason for hiding this comment

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

lint에 맞춰서 자동으로 수정되었습니다.

import { mapClusterNodesFrom } from "./utils/csm.mapper";
import {
findGit,
Expand All @@ -18,7 +18,7 @@ import {
import WebviewLoader from "./webview-loader";

let myStatusBarItem: vscode.StatusBarItem;
const projectName = 'githru';
const projectName = "githru";
Copy link
Member Author

Choose a reason for hiding this comment

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

lint에 맞춰서 자동으로 수정되었습니다.


function normalizeFsPath(fsPath: string) {
return fsPath.replace(/\\/g, "/");
Expand Down Expand Up @@ -58,12 +58,11 @@ export async function activate(context: vscode.ExtensionContext) {

const fetchBranches = async () => await getBranches(gitPath, currentWorkspacePath);
const fetchCurrentBranch = async () => {

let branchName;
try {
branchName = await getCurrentBranchName(gitPath, currentWorkspacePath)
branchName = await getCurrentBranchName(gitPath, currentWorkspacePath);
} catch (error) {
console.error(error);
console.error(error);
Comment on lines +63 to +65
Copy link
Member Author

Choose a reason for hiding this comment

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

lint에 맞춰서 자동으로 수정되었습니다.

}

if (!branchName) {
Expand All @@ -77,7 +76,9 @@ export async function activate(context: vscode.ExtensionContext) {
const fetchClusterNodes = async (baseBranchName = initialBaseBranchName) => {
const gitLog = await getGitLog(gitPath, currentWorkspacePath);
const gitConfig = await getGitConfig(gitPath, currentWorkspacePath, "origin");
const { owner, repo } = getRepo(gitConfig);
const { owner, repo: initialRepo } = getRepo(gitConfig);
webLoader.setGlobalOwnerAndRepo(owner, initialRepo);
const repo = initialRepo[0];
Comment on lines +79 to +81
Copy link
Member Author

Choose a reason for hiding this comment

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

webLoader.setGlobalOwnerAndRepo(owner, initialRepo) 를 통해서 VSCode는 React 패널로 owner와 initialRepo를 postMessage 합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

위 App.tsx에 적었던 것 처럼,
굳이 여기서 webloader에 event날릴 필요없이,
아래쪽의 isPRSuccess 같은 애들과 같이 보내주면 될 거 같습니다.

const engine = new AnalysisEngine({
isDebugMode: true,
gitLog,
Expand Down
2 changes: 1 addition & 1 deletion packages/vscode/src/utils/git.util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
if (await isExecutable(file)) {
try {
return await getGitExecutable(file);
} catch (_) {}

Check warning on line 114 in packages/vscode/src/utils/git.util.ts

View workflow job for this annotation

GitHub Actions / build (16.x)

Empty block statement
}
}
return Promise.reject<GitExecutable>();
Expand Down Expand Up @@ -147,7 +147,7 @@
for (let i = 0; i < paths.length; i++) {
try {
return await getGitExecutable(paths[i]);
} catch (_) {}

Check warning on line 150 in packages/vscode/src/utils/git.util.ts

View workflow job for this annotation

GitHub Actions / build (16.x)

Empty block statement
}
throw new Error("None of the provided paths are a Git executable");
}
Expand Down Expand Up @@ -208,7 +208,7 @@

export const getRepo = (gitRemoteConfig: string) => {
const gitRemoteConfigPattern =
/(?:https?|git)(?::\/\/(?:\w+@)?|@)(?:github\.com)(?:\/|:)(?:(?<owner>.+?)\/(?<repo>.+?))(?:\.git|\/)?(\S*)$/m;
/(?:https?|git)(?::\/\/(?:\w+@)?|@)(?:github\.com)(?:\/|:)(?:(?<owner>[^/]+?)\/(?<repo>[^/.]+))(?:\.git|\/)?(\S*)$/m;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 정규식 디버깅하기 쉽지 않았을텐데 최고입니다!!

const gitRemote = gitRemoteConfig.match(gitRemoteConfigPattern)?.groups;
if (!gitRemote) {
throw new Error("git remote config should be: [https?://|git@]${domain}/${owner}/${repo}.git");
Expand Down
8 changes: 8 additions & 0 deletions packages/vscode/src/webview-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ export default class WebviewLoader implements vscode.Disposable {
`;
return returnString;
}
public setGlobalOwnerAndRepo(owner: string, repo: string) {
if (this._panel) {
this._panel.webview.postMessage({
command: "setGlobalOwnerAndRepo",
data: { owner, repo },
});
}
}
Comment on lines +114 to +121
Copy link
Member Author

Choose a reason for hiding this comment

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

VSCode에서 React 패널의 EventListner로 postMessage하는 함수입니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

위에서 말했듯이, 해당 인터페이스는 엔진에서 데이터 넘길 때 같이 하는 걸로 진행하면 될 것 같구요.

그리고 설사 setGlobalOwnerAndRepo 라는 함수가 필요하다고 가정하더라도,
필요한 command 가 있다면 기존과 같은 방식으로 통일성 있게 진행되어야 할 것 같습니다.
(이렇게 따로 public set 함수로 나오는게 맞는지에 대한 고민 필요?)

그러고보니 extension.ts와 webviewloader.ts는 리팩토링이 좀 심각하게 필요하긴 하겠네요 👿👿👿

Copy link
Member Author

Choose a reason for hiding this comment

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

engine에서 데이터 넘길 때 함께 처리하는 방식으로 수정한다면 웹뷰에 해당 메시지를 날리는 함수는 필요하지 않게 될 것 같습니다.
위에서 피드백 주신대로 새로운 issue에서 수정해보겠습니다 🚀

}

type GithruFetcher<D = unknown, P extends unknown[] = []> = (...params: P) => Promise<D>;
Expand Down
Loading