feat: implementing fetchBranchList #477

ss-won · 2023-09-11T09:50:51Z

Related issue

[view] 기준 Branch 선택 UI #276, [vscode] 기준 Branch 선택 #278

Result

현 git repo의 branch list를 가져와 select box에 렌더링합니다.

Work list

초기 렌더링시 sendGetBranchListMessage 호출을 통해, BranchSelector options 값 렌더링
FakeIDEAdaptor (view dev mode)에 필요한 branchList mock data 생성
fetchBranchList에도 workspaceStore 적용
기타 vscode 리팩토링
- storedAnalyzedData가 덮어쓰기 되는 부분 수정
- 모든 parsing 함수는 특정 값을 return 하도록 하고, respondToMessage 메소드에서 일괄적으로 JSON.stringify를 적용하도록 수정

Discussion

base branch(처음 선택할 브랜치) 기준 -> default branch or HEAD?
select box css...?

ss-won · 2023-09-11T09:52:54Z

@all-contributors please add @ss-won for code

allcontributors · 2023-09-11T09:53:03Z

@ss-won

I've put up a pull request to add @ss-won! 🎉

ytaek

매우 중요한 기능인데, 작업 감사합니다!!

ytaek · 2023-09-12T01:18:55Z

packages/view/src/App.tsx

@@ -20,22 +20,24 @@ import type { IDESentEvents } from "types/IDESentEvents";
 const App = () => {
  const initRef = useRef<boolean>(false);

-  const { filteredData, fetchAnalyzedData, loading, setLoading } = useGlobalData();
+  const { filteredData, fetchAnalyzedData, fetchBranchList, loading, setLoading } = useGlobalData();


지금보니, fetch해서 가져온 거니, fetchedBranchList라고 하면 더 명확할 것 같습니다!
혹은 그냥 branchList라고 해도 될 듯?
(앞의 것도 fetchedAnalyzedData 로 해야 할것 같긴한데..)

안그래도 이미 fetch해온 response data만 전달받아서 상태 업데이트를 하는 함수에 fetch라는 이름이 좀 어색하다고 생각하긴 했었는데 🤔..마땅히 좋은 이름은 떠오르지 않네요. branchList는 또 state 명칭으로 쓰고 있어서 변수명 중복 이슈가 있고 아래 3안 중에 고민해서 일단 적용해보겠습니다.

fetchedBranchList, fetchedAnalyzedData

handleChangeBranchList, handleChangeAnalyzedData

updateBranchList, updateAnalyzedData

packages/view/src/components/BranchSelector/BranchSelector.tsx

ytaek · 2023-09-12T01:31:57Z

packages/view/src/context/GlobalDataProvider.tsx

+  // TODO 초기에 base branch를 fetch해서 적용
+  const [selectedBranch, setSelectedBranch] = useState<string>("main");


넵 이 부분은 빠르게 적용되면 좋겠습니다.
main이 아닌 master가 될 수도 있어서...

혹은 engine에서 넘겨줄 때의 list의 index 0을 쓴다던지? 등의 방법이 있을 것 같네요.

가장 명확한 것은 default는 현재 git이 사용하고 있는 branch가 될 것 같습니다.

ytaek · 2023-09-12T01:32:44Z

packages/view/src/hooks/useGlobalData.ts

  filteredRange: DateFilterRange;
  filteredData: ClusterNode[];
  selectedData: ClusterNode[];
-  setFilteredData: Dispatch<React.SetStateAction<ClusterNode[]>>;
-  setSelectedData: Dispatch<React.SetStateAction<ClusterNode[]>>;
-  setFilteredRange: Dispatch<React.SetStateAction<DateFilterRange>>;
-  fetchAnalyzedData: (analyzedData: ClusterNode[]) => void;
+  setFilteredData: Dispatch<SetStateAction<ClusterNode[]>>;
+  setSelectedData: Dispatch<SetStateAction<ClusterNode[]>>;
+  setFilteredRange: Dispatch<SetStateAction<DateFilterRange>>;
  loading: boolean;
-  setLoading: Dispatch<React.SetStateAction<boolean>>;
-};
+  setLoading: Dispatch<SetStateAction<boolean>>;
+  branchList: string[];
+  setBranchList: Dispatch<SetStateAction<string[]>>;
+  selectedBranch: string;
+  setSelectedBranch: Dispatch<SetStateAction<string>>;
+} & IDESentEvents;


슬슬 요 부분을 정리할 때가 온 것 같습니다 ㅎㅎ
상태관리용 미들웨어가 필요하지 않을까용

넵 아무래도 state가 계속 늘어날 가능성이 있고 비동기 작업이 많고, 타입도 좀 더 깔끔하게 관리하기 위해서 상태관리 라이브러리 사용하면 좋을 것 같네요.

가벼운 용량의 zustand나 jotai 사용하면 좋지 않을까 싶은데, 해당 내용 관련 이슈 올려보겠습니다.

ytaek · 2023-09-12T01:33:46Z

packages/view/src/ide/FakeIDEAdapter.ts

 import type { IDESentEvents } from "types/IDESentEvents";

 import fakeData from "../fake-assets/cluster-nodes.json";
+import fakeBranchList from "../fake-assets/branch-list.json";


ytaek · 2023-09-12T02:05:00Z

#222 도 참고 부탁드립니다!

newgardener · 2023-09-12T12:02:28Z

packages/vscode/src/extension.ts

+        const storedBranchList = context.workspaceState.get<string[]>("branchList") ?? [];
+        context.workspaceState.update(
+          "branchList",
+          (await getBranchNames(gitPath, currentWorkspacePath)) ?? storedBranchList
+        );


여기 로직 관련해서 질문이 있는데요~
caching되어 있는 branch 관련 데이터가 없다면 getBranchNames 함수를 호출하는 방식으로 해야 하지 않을까요??

Suggested change

const storedBranchList = context.workspaceState.get<string[]>("branchList") ?? [];

context.workspaceState.update(

"branchList",

(await getBranchNames(gitPath, currentWorkspacePath)) ?? storedBranchList

);

const branchListData = context.workspaceState.get<string[]>("branchList") ?? (await getBranchNames(gitPath, currentWorkspacePath));

context.workspaceState.upsate("branchList", branchListData);

이런 식으로요!

네네 맞습니다! 짜다보니 caching이 아니라 오류 대체 fallback store 처럼 되어버리긴 했습니다. 🥲

전 일단 기존에 저장된 부분을 previous, 새로 fetching하는 부분을 latest라고 생각했습니다.
현재는 refresh랑 기본 fetch랑 구분이 없다보니..branch list의 상태가 바뀌는 것을 업데이트 하기 위해 매번 fetching 하는 방식 + fetch에 혹시 오류가 있을 경우에는 기존 저장된 previous 값이 있다면 대체하는 방향으로 로직을 작성하였습니다.

어제 잠시 이야기 나눈 대로, refresh랑 fetch part 로직을 분리하는게 좋겠네요! 일단은 단순 fetch 함수에는 말씀하신대로 caching을 적용해 놓겠습니다 :) refresh part 작성 나눠서 같이 진행해봐요~

cache를 하려면, 기존 데이터가 바뀌지 않았다는 전제가 깔려있어야 하는데,
아직 그걸(브랜치 상태) 모니터링하는 부분이 없어서, 조금 아리까리하네요.
(실제 브랜치 리스트의 변경이 있어도 계속 처음 caching된 데이터만 가져올테니)

일단 브랜치 리스트 가져오는 것이 cost가 큰 작업이 아닌 것 같으니, cache는 추후에 고민해봐도 좋을 것 같습니다!

@ss-won
네네! refresh, fetch 로직 분리하는 PR 얼른 올려보도록 할게용

newgardener

👍🏻👍🏻👍🏻

ss-won · 2023-09-13T23:07:55Z

일단 merge 후 해당 리뷰 내용 반영하겠습니다~

ss-won requested review from a team as code owners September 11, 2023 09:50

ss-won added the new feature label Sep 11, 2023

allcontributors bot mentioned this pull request Sep 11, 2023

docs: add ss-won as a contributor for code #478

Merged

feat: implementing fetchBranchList

90f0f09

ss-won force-pushed the feat/issue#276 branch from 7fd043f to 90f0f09 Compare September 11, 2023 09:57

ss-won added ✨ view 💻 vscode labels Sep 11, 2023

ytaek approved these changes Sep 12, 2023

View reviewed changes

newgardener reviewed Sep 12, 2023

View reviewed changes

newgardener approved these changes Sep 13, 2023

View reviewed changes

ss-won merged commit 8646e82 into githru:main Sep 13, 2023
2 checks passed

ss-won mentioned this pull request Sep 19, 2023

feat(view, vscode): implement branchSelector onChange handler #486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implementing fetchBranchList #477

feat: implementing fetchBranchList #477

ss-won commented Sep 11, 2023 •

edited

Loading

ss-won commented Sep 11, 2023

allcontributors bot commented Sep 11, 2023

ytaek left a comment

ytaek Sep 12, 2023

ss-won Sep 13, 2023 •

edited

Loading

ytaek Sep 12, 2023

ytaek Sep 12, 2023

ss-won Sep 12, 2023

ytaek Sep 12, 2023

ytaek commented Sep 12, 2023

newgardener Sep 12, 2023

ss-won Sep 13, 2023 •

edited

Loading

ytaek Sep 13, 2023

newgardener Sep 13, 2023

newgardener left a comment

ss-won commented Sep 13, 2023

		// TODO 초기에 base branch를 fetch해서 적용
		const [selectedBranch, setSelectedBranch] = useState<string>("main");

feat: implementing fetchBranchList #477

feat: implementing fetchBranchList #477

Conversation

ss-won commented Sep 11, 2023 • edited Loading

Related issue

Result

Work list

Discussion

ss-won commented Sep 11, 2023

allcontributors bot commented Sep 11, 2023

ytaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss-won Sep 13, 2023 • edited Loading

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

ytaek commented Sep 12, 2023

Choose a reason for hiding this comment

ss-won Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newgardener left a comment

Choose a reason for hiding this comment

ss-won commented Sep 13, 2023

ss-won commented Sep 11, 2023 •

edited

Loading

ss-won Sep 13, 2023 •

edited

Loading

ss-won Sep 13, 2023 •

edited

Loading