fix(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

seungineer · 2024-07-30T19:52:22Z

Related issue

#496

원인

하이퍼링크 github 주소가 owner는 githru, repo는 githru-vscode-ext로 하드코딩되어 있습니다.
githru-vscode-ext가 아닌 레포지토리에서 issue number 링크를 열더라도 githru-vscode-ext 레포지토리로 이동합니다.

Result

결과

number의 href 주소가 remote github의 issue number 주소로 올바르게 반영됩니다.
number에 따라 issue 또는 PR이 결정되므로 href 주소가 /issue/235 이더라도 레포지토리에서 '235번'이 PR이라면 github.com에서 /pull/235로 자동 redirection 됩니다.

HTTPS로 clone한 레포지토리에서 이슈 넘버를 클릭한 결과, 올바른 github의 issue로 이동합니다.

SSH로 clone한 레포지토리에서 이슈 넘버를 클릭한 결과, 올바른 github의 issue로 이동합니다.

Work list

1. owner, repo를 parsing 하는 정규식 수정(Commit)

gitconfig가 [email protected]:HandTris/FE-HandTris.git 인 경우 repo를 'F', owner를 'HandTris'로 parsing하여 repo 정보를 사용할 수 없었음
gitconfig가 https://github.com/seungineer/react-toy-projcet.git인 경우 repo를 'r', owner를 'seungineer'로 parsing하여 repo 정보를 사용할 수 없었음
정규식 수정 후 아래와 같이 올바르게 parsing 됩니다.
owner는 'seungineer'이고, repo는 'react-toy-project'입니다.

2. owner와 repo 정보를 Global Data로 관리할 수 있도록 항목 추가(Commit)

owner, repo 정보는 패널이 받는 postMessage에 의해서 결정되므로 postMessage를 listen 하는 곳과 owner, repo 정보를 사용하는 곳(Content 컴포넌트)이 다르기에 Global로 관리하였습니다.

3. owner와 repo를 global로 저장하기 위해 VSCode(Node.js)에서 패널로 메시지를 보내는 기능(Commit)

owner와 repo 정보를 담아서 postMessage 하도록 합니다.

4. 받은 메시지를 처리하며, owner와 repo를 결정하는 EventListener 구현(Commit)

App.tsx에 EventListner를 추가하여 VSCode에서 보내는 메시지를 듣도록 하였습니다.
받은 메시지의 owner, repo 정보로 issue link 하이퍼링크에 사용될 owner, repo 상태를 업데이트합니다.

5. Content 컴포넌트가 렌더링될 때 Global Data의 owner와 repo 정보를 사용하여 Issue number의 하이퍼링크를 만들고, linkedStr을 만들도록 함(Commit)

Content 컴포넌트가 렌더링되면 linkedStr이 단 한 번 생성되어야 하므로 useEffect hook을 사용하였습니다.

EventListner 도입 사유

'VSCode(Node.js)'에서 owner와 repo가 결정되면 'React'에서 이를 가져다 써야 하는 구조입니다. Node.js에서 직접 상태를 업데이트하거나 전달할 수 없습니다.
postMessage 메써드와 EventListner를 사용하면 'React'에서 'VSCode 단'의 정보를 받을 수 있기에 도입하였습니다.
VSCode API를 사용하여 Node.js에서 owner와 repo 정보를 상태 저장소에 저장한 후 'React'에서 저장된 상태를 요청하여 응답받는 방식이 가능합니다. 다만, VSCode API라 VSCode가 아닌 IDE에서는 작동하지 않기에 EventListener로 구현하였습니다.

Discussion

혹 VSCode API를 사용하는 방식이 더 적합한 경우 피드백 주시면 수정해 보겠습니다!

…thru#496

seungineer · 2024-07-30T20:15:33Z

packages/view/src/App.tsx

+    const handleMessage = (event: MessageEvent<RemoteGitHubInfo>) => {
+      const message = event.data;
+      setOwner(message.data.owner);
+      setRepo(message.data.repo);
+    };
+
+    window.addEventListener("message", handleMessage);


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

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

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

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

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

seungineer · 2024-07-30T20:16:56Z

packages/vscode/src/extension.ts

@@ -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";


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

seungineer · 2024-07-30T20:17:06Z

packages/vscode/src/extension.ts

@@ -18,7 +18,7 @@ import {
 import WebviewLoader from "./webview-loader";

 let myStatusBarItem: vscode.StatusBarItem;
-const projectName = 'githru';
+const projectName = "githru";


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

seungineer · 2024-07-30T20:17:16Z

packages/vscode/src/extension.ts

+          branchName = await getCurrentBranchName(gitPath, currentWorkspacePath);
        } catch (error) {
-            console.error(error);
+          console.error(error);


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

seungineer · 2024-07-30T20:19:36Z

packages/vscode/src/extension.ts

+        const { owner, repo: initialRepo } = getRepo(gitConfig);
+        webLoader.setGlobalOwnerAndRepo(owner, initialRepo);
+        const repo = initialRepo[0];


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

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

seungineer · 2024-07-30T20:22:39Z

packages/vscode/src/webview-loader.ts

+  public setGlobalOwnerAndRepo(owner: string, repo: string) {
+    if (this._panel) {
+      this._panel.webview.postMessage({
+        command: "setGlobalOwnerAndRepo",
+        data: { owner, repo },
+      });
+    }
+  }


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

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

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

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

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

seungineer · 2024-07-30T20:34:35Z

packages/view/src/components/VerticalClusterList/Summary/Content/Content.tsx

+      }
+      return acc;
+    }, []);
+    setLinkedStr(newLinkedStr);


AS-IS

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

TO-BE

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

xxxjinn

설명을 정말 자세하게 잘 적어주신 것 같아요!! 👍👍🙂

ytaek

꼼꼼한 수정 감사합니다!!
PR Description은 완벽 그 자체네요! 💯💯💯💯💯

전체 구조 관련해서 리뷰를 남겼습니다.
기존 구조에 맞도록 코드의 구조적인 수정이 필요할 것 같은데요.
우선 해당 PR은 approve 되면 merge 하시고,

현재 리뷰 나온 포인트들을 정리해서 issue로 만들어 주신 다음,
다시 할당하여 진행하시면 좋을 것 같습니다.

그리고, 가능하시다면 PR은 좀 더 끊어서 올려주셔도 좋을 것 같습니다.
Review하는데 시간이 많이 걸리네요 🥲🥲🥲
(지금 같은 경우라면, .. 약간 정석대로 하자면

view쪽 수정 부분
vscode쪽 수정 부분
두 부분으로 올려주셨으면 좀 더 가벼운 마음으로 리뷰했을 것 같습니다 🛩️

아무튼 고생 많으셨습니다! 다음 PR 에서 뵐께요!

ytaek · 2024-08-01T16:58:41Z

packages/view/src/types/RemoteGitHubInfo.ts

@@ -0,0 +1,6 @@
+export interface RemoteGitHubInfo {


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

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

ytaek · 2024-08-01T16:59:45Z

packages/view/src/types/RemoteGitHubInfo.ts

@@ -0,0 +1,6 @@
+export interface RemoteGitHubInfo {
+  data: {


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

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

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

ytaek · 2024-08-01T17:00:51Z

packages/vscode/src/utils/git.util.ts

@@ -208,7 +208,7 @@ export async function getGitConfig(

 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;


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

ytaek · 2024-08-01T17:02:33Z

packages/view/src/hooks/useGlobalData.ts

  setBranchList: Dispatch<SetStateAction<string[]>>;
  selectedBranch: string;
  setSelectedBranch: Dispatch<SetStateAction<string>>;
+  owner: string;
+  setOwner: Dispatch<SetStateAction<string>>;
+  repo: string;
+  setRepo: Dispatch<SetStateAction<string>>;
 } & IDESentEvents;


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

recoil... zustand...👍

recoil... zustand...👍

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

ytaek · 2024-08-01T17:12:31Z

packages/view/src/App.tsx

+    const handleMessage = (event: MessageEvent<RemoteGitHubInfo>) => {
+      const message = event.data;
+      setOwner(message.data.owner);
+      setRepo(message.data.repo);
+    };
+
+    window.addEventListener("message", handleMessage);


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

ytaek · 2024-08-01T17:13:59Z

packages/vscode/src/extension.ts

+        const { owner, repo: initialRepo } = getRepo(gitConfig);
+        webLoader.setGlobalOwnerAndRepo(owner, initialRepo);
+        const repo = initialRepo[0];


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

ytaek · 2024-08-01T17:17:47Z

packages/vscode/src/webview-loader.ts

+  public setGlobalOwnerAndRepo(owner: string, repo: string) {
+    if (this._panel) {
+      this._panel.webview.postMessage({
+        command: "setGlobalOwnerAndRepo",
+        data: { owner, repo },
+      });
+    }
+  }


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

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

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

seungineer · 2024-08-02T16:34:02Z

꼼꼼한 수정 감사합니다!! PR Description은 완벽 그 자체네요! 💯💯💯💯💯

전체 구조 관련해서 리뷰를 남겼습니다. 기존 구조에 맞도록 코드의 구조적인 수정이 필요할 것 같은데요. 우선 해당 PR은 approve 되면 merge 하시고,

현재 리뷰 나온 포인트들을 정리해서 issue로 만들어 주신 다음, 다시 할당하여 진행하시면 좋을 것 같습니다.

그리고, 가능하시다면 PR은 좀 더 끊어서 올려주셔도 좋을 것 같습니다. Review하는데 시간이 많이 걸리네요 🥲🥲🥲 (지금 같은 경우라면, .. 약간 정석대로 하자면

view쪽 수정 부분

vscode쪽 수정 부분
두 부분으로 올려주셨으면 좀 더 가벼운 마음으로 리뷰했을 것 같습니다 🛩️

아무튼 고생 많으셨습니다! 다음 PR 에서 뵐께요!

내용도 많고, 이상한(어색한(?)) 부분도 많은 PR 봐주셔서 감사합니다😅 처음 이렇게 세세한 피드백(코드리뷰)을 받아봤는데 엄청 좋네요.. (감동)(감동)(감동)(감동)(감동)(감동)

이번 PR에서 피드백 받았던 부분은 issue 다시 만들어서 수정 진행할께요!

그리고 다음 PR부터는 명확하게 분리 가능한 수정 부분은 나누어서! PR 올리도록 하겠습니다! 감사합니다 👍👍

seungineer added 6 commits July 30, 2024 22:35

fix(vscode): owner와 repo parsing 정규식 수정 githru#496

ca9b8f3

feat(vscode): owner와 repo 정보를 postMessage하여 Global에 저장할 수 있는 함수 구현 gi…

2895f5c

…thru#496

feat(view): GlobalData에 owner, repo 정보 추가 githru#496

20fea1e

feat(vscode): GlobalData에 owner, repo 저장 기능 구현 githru#496

e408706

feat(view): owner, repo 정보가 담긴 event를 처리할 EventListener 추가 githru#496

606f976

fix(view): linkIssues 및 linkedStr 결정 로직 변경 githru#496

d2fd559

seungineer requested review from a team as code owners July 30, 2024 19:52

fix(vscode): test fail로 인해 lint에 맞도록 수정 githru#496

8f0ebca

seungineer added ✨ view 💻 vscode fix labels Jul 30, 2024

seungineer commented Jul 30, 2024

View reviewed changes

seungineer self-assigned this Jul 30, 2024

xxxjinn approved these changes Aug 1, 2024

View reviewed changes

ytaek approved these changes Aug 1, 2024

View reviewed changes

seungineer merged commit 1354c36 into githru:main Aug 2, 2024
2 checks passed

This was referenced Aug 3, 2024

[view] Remote Git configuration 정보 중 owner와 repo name 정보를 View에서 받는 방식 수정 #588

Closed

[view] dev 모드 실행 시 에러 발생 #603

Closed

ytaek added this to the v0.7.0 milestone Aug 10, 2024

seungineer mentioned this pull request Oct 14, 2024

[view] Remote Git config에서 owner, repo 정보를 View에서 받는 방식 수정 #768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

fix(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

seungineer commented Jul 30, 2024 •

edited

Loading

seungineer Jul 30, 2024

ytaek Aug 1, 2024

seungineer Aug 2, 2024

seungineer Jul 30, 2024

seungineer Jul 30, 2024

seungineer Jul 30, 2024

seungineer Jul 30, 2024

ytaek Aug 1, 2024

seungineer Jul 30, 2024

ytaek Aug 1, 2024

seungineer Aug 2, 2024

seungineer Jul 30, 2024

xxxjinn left a comment

ytaek left a comment

ytaek Aug 1, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

seungineer Aug 2, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

joonwonBaek Aug 2, 2024

seungineer Aug 2, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

seungineer commented Aug 2, 2024 •

edited

Loading

fix(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

fix(view): 커밋 메시지의 PR or Issue number 하이퍼링크 버그 #569

Conversation

seungineer commented Jul 30, 2024 • edited Loading

Related issue

원인

Result

결과

Work list

1. owner, repo를 parsing 하는 정규식 수정(Commit)

2. owner와 repo 정보를 Global Data로 관리할 수 있도록 항목 추가(Commit)

3. owner와 repo를 global로 저장하기 위해 VSCode(Node.js)에서 패널로 메시지를 보내는 기능(Commit)

4. 받은 메시지를 처리하며, owner와 repo를 결정하는 EventListener 구현(Commit)

5. Content 컴포넌트가 렌더링될 때 Global Data의 owner와 repo 정보를 사용하여 Issue number의 하이퍼링크를 만들고, linkedStr을 만들도록 함(Commit)

EventListner 도입 사유

Discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AS-IS

TO-BE

xxxjinn left a comment

Choose a reason for hiding this comment

ytaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungineer commented Aug 2, 2024 • edited Loading

seungineer commented Jul 30, 2024 •

edited

Loading

seungineer commented Aug 2, 2024 •

edited

Loading