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]#520 git 연동이 안된 프로젝트에서 무한로딩 증상 개선 #679

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

DaYoung-woo
Copy link
Contributor

@DaYoung-woo DaYoung-woo commented Aug 29, 2024

Related issue

#520

Result

git 연동이 안된 프로젝트에서 무한로딩 증상 개선을 위해 오른쪽 하단 toast메시지를 표기해주었습니다.
362687269-e2af7f98-3693-45a8-afbb-b5c0367f852e

Work list

사용자에게 깃이 연동되지 않은 프로젝트를 보여줄 때 무한로딩화면을 제거하고 하단 오른쪽에 오류 메시지를 보여줄 수 있도록 변경하였습니다.

Discussion

현재 패널이 잠깐 열렸다 닫히며 깜빡임이 있는데, 추후 웹뷰 판넬 자체가 열리지 않도록 개선이 필요합니다.
관련해서 코드 피드백을 주시면 적극 반영해보도록 하겠습니다!

seungineer
seungineer previously approved these changes Aug 31, 2024
Copy link
Member

@seungineer seungineer left a comment

Choose a reason for hiding this comment

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

이제 git 연동되지 않은 레포지토리 처리가 되겠군요! 😈
LGTM ! 👍👍

Comment on lines 34 to 42
if (command === "fetchAnalyzedData" || command === "refresh") {
const baseBranchName = (payload && JSON.parse(payload)) ?? (await fetchCurrentBranch());
// Disable Cache temporarily
// const storedAnalyzedData = context.workspaceState.get<ClusterNode[]>(`${ANALYZE_DATA_KEY}_${baseBranchName}`);

// if (!storedAnalyzedData) {
try {
analyzedData = await fetchClusterNodes(baseBranchName);
context.workspaceState.update(`${ANALYZE_DATA_KEY}_${baseBranchName}`, analyzedData);
Copy link
Member

Choose a reason for hiding this comment

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

(궁금💭)Disable Cache temporarily로 주석 처리되어 있는데,
Cache 기능이 일시적으로 빠진 걸까요?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

엇 이부분은 기존에 있던 주석이었는데 혹시 작업하시던 분이 갑자기 사라지면 당황하실까봐 다시 넣어두었습니다!

packages/vscode/src/webview-loader.ts Outdated Show resolved Hide resolved
Comment on lines +74 to +77
if (!analyzedData) {
this.dispose();
throw new Error("Project not connected to Git.");
}
Copy link
Member

Choose a reason for hiding this comment

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

analyzedData가 존재하지 않으면 Error를 던지는군요!

현재 패널이 잠깐 열렸다 닫히며 깜빡임이 있는데, 추후 웹뷰 판넬 자체가 열리지 않도록 개선이 필요합니다.

analyzedData가 존재하지 않음을 더 빨리 알아채서 판넬 띄우기 전에 Error를 던지는 방식으로 되어야 할까요?.. 그러면 Error를 어디다가 던져야 IDE에서 toast message로 git이 연동되지 않았음을 나타낼 수 있을지 모르겠네요 🤔

해당 PR 방식 외 다른 방법은 떠오르지 않네요...😅 이 방식의 결과도 좋아보이구요! 👍👍👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저희도 Error를 어디서 catch 할 지 여러 방식으로 도전했는데 마땅한 방법을 찾지 못해서 이렇게 처리했습니당!
꼼꼼하게 내용을 확인해 주셔서 감사합니당👍👍👍👍👍👍👍👍

Copy link
Contributor

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

연동되지 않은 프로젝트에 토스트 메시지 보여주는 것 좋네요 👍👍👍
브랜치별 데이터를 캐싱하는 코드가 없어진 것 같아서 코멘트 따로 남겼습니다! 확인 한번 부탁드려요 😊

Comment on lines -20 to -24
//캐시 초기화
console.log("Initialize cache data");
context.workspaceState.keys().forEach(key => {
context.workspaceState.update(key, undefined);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

#661 에서 추가되었던 브랜치별 데이터 캐싱 관련 코드들이 삭제된 것 같아 보이는데 혹시 어떤 이유에서인가요..?!?!
(의도된 삭제가 아니라면 #661 이전 코드로 롤백된 것 같기도 해서 머지 과정에서 뭔가 꼬인 것 같아 보이기도....하네요...🥺)

Comment on lines -40 to -47
const storedAnalyzedData = context.workspaceState.get<ClusterNode[]>(`${ANALYZE_DATA_KEY}_${baseBranchName}`);
let analyzedData = storedAnalyzedData;
if (!storedAnalyzedData) {
console.log("No cache Data");
console.log("baseBranchName : ",baseBranchName);
analyzedData = await fetchClusterNodes(baseBranchName);
context.workspaceState.update(`${ANALYZE_DATA_KEY}_${baseBranchName}`, analyzedData);
}else console.log("Cache data exists");
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분도 브랜치별로 데이터를 캐싱하기 위한 코드입니다..! 여기도 같이 확인 부탁드려요...!

Copy link
Contributor Author

@DaYoung-woo DaYoung-woo Aug 31, 2024

Choose a reason for hiding this comment

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

엇 죄송합니다! 다시 확인해보겠습니다!!

Comment on lines +74 to +77
if (!analyzedData) {
this.dispose();
throw new Error("Project not connected to Git.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

지금 방법처럼 analyzedData 존재 여부를 확인해서 git 연동 여부를 확인하는 방식으로는 저도 지금 PR 외의 방법은 생각나지 않네요... 🥲
이미 생각해보셨을 수도 있지만, 패널 생성 이전에 git 연동 여부를 확인할 수 있는 로직을 별도로 추가할 수 있으면 좋을 것 같긴 합니다........ (어떻게 확인할 수 있을지는 잘 모르겠지만요... 🫠)

Copy link
Member

@seungineer seungineer left a comment

Choose a reason for hiding this comment

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

누락됐던 부분까지 잘 반영된 것 같습니다!
LGTM! 👍👍

Comment on lines +51 to +53
context.workspaceState.update(`${ANALYZE_DATA_KEY}_${baseBranchName}`, analyzedData);
} else console.log("Cache data exists");

Copy link
Member

Choose a reason for hiding this comment

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

} else console.log("... 이 부분 lint도 맞아졌네요 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

확인 감사드립니당 👍👍👍👍

Copy link
Contributor

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

누락된 부분 다시 추가된 것 확인했습니다!! LGTM!!! 👍👍👍

Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

논의 많이 하시던데 잘 해결됐군요 : ) 고생 많으셨습니다!

@@ -121,6 +124,7 @@ export async function activate(context: vscode.ExtensionContext) {
vscode.window.showErrorMessage(error.message);
} else {
vscode.window.showErrorMessage((error as Error).message);
myStatusBarItem.text = `$(diff-review-close) ${projectName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

오홍 요렇게 x를 넣을 수도 있군요 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아이콘이 굉장히 다양하더라고용! 직관적으로 보여주기 위해 x표시로 해주었습니다!

@choisohyun choisohyun modified the milestones: v0.7.1, v0.7.2 Sep 2, 2024
@ytaek
Copy link
Contributor

ytaek commented Sep 10, 2024

리뷰 커멘트들 반영됐다면 merge 부탁드립니다!!
혹시 빠진게 있다면 다음 PR에 올려주세용~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants