[view] 새로고침의 무한로딩 문제 해결 #817
Conversation
- handleChangeBranchList에서 로딩 상태 해제 로직 추가 - 즉, 브랜치 리스트 응답 시에도 로딩 상태가 해제되도록 수정
데이터처리 속도차이로 인해 일정시간의 타이머를 걸어두었습니다.
| setBranchList: (branches) => set({ branchList: branches }), | ||
| setSelectedBranch: (branch) => set({ selectedBranch: branch }), | ||
| handleChangeBranchList: (branches) => | ||
| handleChangeBranchList: (branches) => { |
There was a problem hiding this comment.
(다음 PR을 위한 제안) 코드가 길지 않지만, set state를 할 때 immutability를 신경써서 작업을 하긴 해야하네요.
추후 작업에서는 immer를 사용해보면 어떨까요?
There was a problem hiding this comment.
(다음 PR을 위한 제안) 코드가 길지 않지만, set state를 할 때 immutability를 신경써서 작업을 하긴 해야하네요. 추후 작업에서는
immer를 사용해보면 어떨까요?
괜찮으시면 issue로 새로 등록하시고,
전체 코드에서 immer 적용 작업해보셔도 좋을 것 같습니다!
There was a problem hiding this comment.
넵! 다음 작업으로 immer 적용 한번 해보겠습니다
There was a problem hiding this comment.
넵! 다음 작업으로 immer 적용 한번 해보겠습니다
👍👍👍
@chae-dahee 님, 해당 건 이슈로 등록하고 할당하신 다음에 진행해주셔도 좋을 것 같습니다!!
| })); | ||
| setTimeout(() => { | ||
| useLoadingStore.getState().setLoading(false); | ||
| }, 1500); | ||
| }, |
There was a problem hiding this comment.
(아리까리) 앗. 현재 store의 setstate에서 다른 store의 값을 건드리게 되면, 뭔가 로직의 의존성이 생길 것 같은데요.
handleChangeBranchList 를 호출한 부분에서 사용해야하지 않을까요?
There was a problem hiding this comment.
맞습니다. 리뷰해주신 대로 스토어 간의 직접적인 의존성으로 인해 위 문서의 🚨 문제상황이 발생한 것 같아요..
따라서 handleChangeBranchList 와 handleChangeAnalyzedData 의 로딩상태를 개별적으로 관리하도록 useLoadingStore를 수정해 보았습니다!
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
packages/view/src/store/branch.ts
Outdated
| selectedBranch: !state.selectedBranch && branches.head ? branches.head : state.selectedBranch, | ||
| })), | ||
| })); | ||
| useLoadingStore.getState().setIsBranchLoading(false); |
There was a problem hiding this comment.
요부분은 저도 좀 아리까리해서 논의해보면 좋을 것 같은데요.
이 부분이 하나의 store에서 다른 store의 값들에 의존성을 가지는 건데
pmndrs/zustand#630
요런 글들을 보면 이럴꺼면 하나로 합치는게 낫다 이런 의견도 있구요.
pmndrs/zustand#1586
Zustand is unopinionated, and you can do what you can do.
요런 의견들도 있고.. 다양하군요 ㅎㅎ
요 건은 이슈를 만들어서 한번 discussion 해보면 어떨까요?
There was a problem hiding this comment.
의존성에 대해 짚어주셔서 구조를 더 깊이 고민하고 바로잡을 수 있었습니다. 감사합니다!
결론부터 말씀드리면 기존 패턴으로 돌아가는 것이 맞다고 생각합니다.
처음에 제가 isBranchLoading 상태를 분리했던 것은 무한 로딩 문제에 대한 잘못된 접근이었습니다. 다시 분석해보니, 문제의 근본적인 해결책은 로딩 상태의 구조가 아니라 '2️⃣ 브랜치 목록을 재요청하는 로직 처리' 이었습니다. (파악이 부족해서 발생한 오해입니다.. 스스로도 조금 아쉽네요🥲)
즉, 2️⃣ 로직만으로도 문제는 해결되므로, 1️⃣ 작업처럼 불필요한 의존성을 만들면서까지 로딩 상태를 분리할 필요가 없었습니다. 꼼꼼히 봐주신 덕분에 실수를 명확히 인지하고 수정할 수 있게 되었습니다.
Zustand의 Slices Pattern 설계에 따르면, 강한 결합이 있을 경우 하나의 스토어에서 관리하라고 권장합니다. 분리된 두개의 로딩상태는 사실상 하나의 원자적 트랜잭션으로, 상위 컴포넌트에서 하나로 관리하는 것이 가장 적합한 구조라고 생각합니다. 각 스토어에 로딩 상태를 결합시켜 불필요한 의존성을 만드는 것보다 단순하고 예측 가능한 방법입니다.
따라서 현재 PR에 적용된 isBranchLoading과 같은 분리된 로딩 상태를 다시 제거하고, 기존처럼 하나의 전역 로딩 상태를 사용하는 패턴으로 코드를 되돌리는 것이 맞다고 생각합니다. 이 방식이 스토어 간의 사이드 이펙트를 줄이고, 상태 변화의 흐름을 더 예측 가능하게 만들어 테스트와 유지보수 측면에서 더 유리할 것 같습니다. 말씀해주신 고혀래야하는 문제상황도 방지하고요!
이 방향으로 코드를 수정하도록 하겠습니다. 혹시 Issue discussion이 필요하다고 생각되시면 말씀해주세요!
There was a problem hiding this comment.
구현해주신 내용 잘 봤습니다! 무한로딩될 때마다 불편했는데 수정해주셔서 다음부터는 사용하기 편해질 것 같아요ㅎㅎ
한가지 궁금증이자 의견하나 드리자면, 로딩 여부를 각 도메인에서 관리하는 건 어떻게 생각하시는지 궁금합니다!
사용자 인터렉션이 있을 때마다 여러 요소들이 각각 다른 시점에 로딩이 완료될 것 같은데, 그 때마다 loading.ts 에서 접두사를 바꾸면서 관리하게 되면 추후 각각의 로딩에 대한 매칭을 하기 힘들 수도 있겠다는 생각이 들었습니다ㅎㅎ
analyzdedData, branchList 를 컨트롤 하는 각각의 모듈에서 isLoading을 따로 관리하게되면 코드 응집성도 높아지고 의미가 명확해질 수 있을 것 같습니다!
좋은 의견 정말 감사합니다! 말씀해주신 "각 도메인에서 로딩을 관리하는 방식"은 확장성을 고려한다면 짚고 넘어가야 할 포인트라고 생각합니다. 의견 주신 대로 여러 요소가 다른 시점에 로딩이 완료되는 상황이라면, 각 도메인 스토어가 자체 로딩 상태를 갖는 것이 응집성과 명확성 측면에서 필요한 설계입니다. 다만, 현재 동작을 자세히 살펴보면, 로딩이 필요한 장시간 작업은 analyzedData과 연관이 있습니다
따라서 과거에 로딩상태를 Context API 로 관리하고, 현재 Zustand 로 넘어온 이유이지 않을까 싶습니다. 아무래도 analyzedData 로딩 작업이 도메인에 종속된 로딩상태라고 판단하기 보다, 서비스 전체의 로딩상태에 가깝기 때문입니다. 제가 위의 코멘트에서 상태분리 로직에 대해 다시 생각해보고 revert를 결심한 이유도, 하나의 로딩상태 시나리오를 사용해 단순한 구조를 효과적으로 관리할 방법이라고 생각했기 때문입니다. 좋은 질문 덕분에 현재 githru 의 로딩과 상태관리에 대해 분석하고 다시 생각해 볼 수 있었던 것 같습니다. 감사합니다😊 |
- 무한로딩의 근본적인 문제가 아님 - zustand 상태끼리의 의존성와 결합도를 낮추기 위해 isBranchLoading 을 제거했습니다.
…ahee/githru-vscode-ext into fix/#813_infinite-loading
|
답변이 늦어져서 죄송합니다! 먼저 자세한 의견을 공유해 주셔서 정말 감사드려요😊 다희님 덕분에 스토어 간 의존성에 대해 깊이 생각해볼 좋은 기회가 되었습니다! 몇 가지 궁금한 점들이 있어서 질문을 드리고 싶어요.
=> 현재 만약 현재 구조에서의 Zustand 넘어온 이유에 대한 다희님의 생각을 들어보고 싶습니다!
=> 현재 로딩 상태가 다른 스토어에서 관리된다면, |
|
@SingTheCode 안녕하세요! 먼저 질문에 대한 대답을 드려보자면 1. useState or context api → zustand 사용하게 된 의견
이 두 문제를 해결하기 위해 zustand 를 사용한 것 같습니다. 또한 zustand 는 selector 로 불필요한 리렌더링을 방지한다고 알고 있습니다. 현재의 단일 로딩상태일때는 크게 체감되지 않을 수 있지만, 위 상황을 고려한 설계 결정으로 보입니다. 저도 더 다방면으로 스토어를 활용하면 좋을 것 같습니다! 2. 로딩 상태 분리 전략
말씀해주신대로 이 방식은 한계점이 보이네요! 아마 가장 중요한 데이터인 analyzedData 가 준비되면, 일단 화면을 보여줘서 속도를 높이려는 전략의 결과로 보입니다. branchList나 githubInfo는 부가적인 정보이므로, 로딩 여부와 관계없이 핵심 컨텐츠를 먼저 보여주는 것입니다. 따라서 제안해주신 isLoading 상태 관리에 대한 개선이 필요해 보입니다. 도메인 스토어가 자신의 데이터와 로딩 상태를 함께 책임지는 구조로 발전하면 좋을 것 같습니다. 괜찮으시다면 제가 이슈 파서 개선작업을 진행해 보겠습니다 👍 또는 재현님께서 관심있거나 하고싶은 작업이시라면 말씀해주세요! 상태관리에 대해 더 논의사항이 필요하다면 discussion 이슈를 새로 파는것도 필요할 것 같습니다. 새로고침 무한로딩은 처음에는 상태관리의 문제인줄 파악했으나, 사실 응답처리 로직의 생략이 문제였습니다. 이번 PR 에서는 먼저 무한로딩 해결(응답처리 로직)하는데에 집중하고 싶습니다 😊 |
|
로딩 상태관리에 대한 이슈가 아니였는데 얘기가 많이 길어졌네요ㅎㅎ |
3 participants
Related issue
Closes #813
Result
2025-08-06.7.18.29.mov
✅ 새로고침 버튼 클릭 시 무한로딩 문제 해결
Work list
Loading 상태, IsBranchLoading 상태를 구분하여 의존성 해제Discussion
1️⃣
로딩상태를 분리하고, 해제하는 로직 추가두개의 메시지가 전송됩니다.
로딩상태 해제 로직
해당 함수에 로딩 상태를 해제하는 로직을 추가하고, 두 기능을 각각의 상태로 분리하여 해결했습니다.2️⃣ IDEAdapter 의 refresh 일 경우 추가
view/src/service/index.ts 의 sendRefreshDataCommand 함수는 새로고침 버튼을 클릭할때 돌아갑니다.
sendFetchAnalyzedDataMessage 함수가 아닌, sendRefreshDataMessage 함수를 호출합니다.
이때 백엔드 응답의 command 는 refresh 로 오게 됩니다. (백엔드 로직은 fetch, refresh 동일합니다.)
응답을 처리하는 view/src/ide/VSCodeIDEAdapter.ts 에서 command 가 refresh 로 올 경우에 대한 처리가 빠져있어서
Unknown Message가 출력되는 것을 확인했습니다.해당 switch 문에 refresh case 를 추가하여 해결했습니다!
🚨 문제상황branchList 가 analyzedData 보다 로딩 해제되는 속도가 빨라, 1초 정도 아래와 같은 상태가 발생합니다. 따라서 현재 setTimeout 을 걸어두었습니다만.. 아쉬운 해결책이라는 생각이 듭니다.. 혹시 좋은 의견 있으시다면 코멘트 부탁드립니다!해결 완료, #822 추후 immer 적용으로 상태관리를 직관적이고 안전하게 보장할 예정입니다.