-
Notifications
You must be signed in to change notification settings - Fork 0
[fix/#300] BrowsingView에서 네비게이션 pop 시 세션이 정리되지 않는 문제를 개선한다. #301
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
Conversation
YunDaeHyeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📄 PR Code Review Report
🔍 Summary
BrowsingStore에 isMovingToNextStep 플래그를 추가하여 viewWillDisappear 시 브라우저 연결 해제 로직을 조건부로 실행하도록 변경되었습니다. 이 변경은 특정 내비게이션 흐름에서 브라우저 연결을 유지하기 위한 의도로 보이나, 플래그가 한 번 true로 설정된 후 다시 false로 재설정되지 않는 문제가 있어 브라우저 연결이 예상보다 오래 유지되거나 의도치 않게 해제되지 않는 위험을 내포하고 있습니다.
⚠️ Key Issues
isMovingToNextStep플래그의 미재설정:prepareMoveToNextStep인텐트가 전송되면isMovingToNextStep상태는true로 변경됩니다. 하지만 이 플래그를 다시false로 되돌리는 로직이 누락되어 있습니다. 이로 인해 사용자가 내비게이션을 시작한 후 실제 다음 화면으로 이동하지 않고 돌아오거나, 내비게이션이 실패하는 경우에도 해당 플래그는 계속true상태를 유지하게 됩니다. 결과적으로BrowsingView에서 뒤로가기 내비게이션이 발생했을 때browser.disconnect()호출이 불필요하게 건너뛰어질 수 있으며, 이는 브라우저 연결이 의도치 않게 유지되는 리소스 누수 또는 상태 불일치를 유발할 수 있습니다.
🛠 Improvement Suggestions
isMovingToNextStep플래그 재설정 로직 추가:isMovingToNextStep플래그는 특정 내비게이션이 완료되거나 취소되었을 때false로 명시적으로 재설정되어야 합니다. 예를 들어, 다음 화면으로의 전환이 성공적으로 이루어졌음을 알리는 리듀스 결과, 혹은BrowsingView가 다시 화면에 나타날 때false로 재설정하는 인텐트를 추가하는 것을 고려할 수 있습니다. 가장 안전한 방법은 현재 뷰의 라이프사이클이 종료되거나, 이 플래그의 유효 범위가 끝나면false로 초기화하는 것입니다.- 내비게이션 실패 또는 취소 시의 일관성 있는 처리:
router.push작업이 실패하거나 사용자가 내비게이션 도중에 취소하는 상황을 고려하여, 이러한 경우에도isMovingToNextStep플래그가 즉시false로 재설정되도록 처리하여 후속viewWillDisappear로직이 올바르게 동작하게 해야 합니다.
✅ Positive Observations
- 의도 명확화:
viewWillDisappear시 브라우저 연결 해제 로직의 실행 여부를isMovingToNextStep플래그를 통해 제어하려는 시도는 특정 내비게이션 흐름에서 브라우저 연결을 유지해야 하는 비즈니스 로직의 의도를 명확하게 반영하려는 좋은 접근 방식입니다. - Flux/Redux 패턴 유지: 새로운 상태와 인텐트, 결과를 추가하여 일관된 Store 기반의 상태 관리 패턴을 유지하고 있습니다. 이는 코드의 예측 가능성과 테스트 용이성을 높이는 데 기여합니다.
|
AI의 Key Issues 리뷰의 경우 사용자가 갔다가 다시 돌아올 경우 Store가 살아있어서 flag가 true로 남아있는 경우를 말합니다. |
GRJeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
빠른 수정 감사합니다
sangYuLv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 👍🙂
68f6835 to
85405e8
Compare
opficdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- Browser는 계산 프로퍼티로 스트림을 노출 - HeartBeeater는 스트림 매니저의 yield 메서드 사용 - 테스트 코드 모두 통과로 동등성 확인 완료
- BrowsingView에서 뒤로 가기(Navigation Pop) 시 연결을 해제하지 않아 세션이 유지되는 문제 해결 - isMovingToNextStep 상태를 도입하여 화면 이탈 사유(Push vs Pop) 구분 - 정상적인 다음 단계 이동(Push)에는 연결 유지, 뒤로 가기(Pop) 시에는 disconnect 호출
- 이게 왜 살아난거지
21ff8bc to
6f1fb95
Compare
🔗 연관된 이슈
📝 작업 내용
📌 요약
BrowsingView에서 사용자가 Navigation의 뒤로 가기를 하거나스와이프를 통해 홈으로 돌아가도 세션 연결이 종료되지 않아 발생하는 재연결 불가 및 상태 불일치 버그를 수정했습니다.
🔍 상세
문제 상황
사용자가 기기 연결 화면에서 "뒤로 가기" 버튼을 클릭하거나 스와이프로 홈 화면으로 돌아가도,
기존 연결된 세션이 끊어지지 않고 유지되는 문제가 있었습니다.
ConnectedView에 계속 머무름이를 해결하기 위해
onDisappear가 호출되는 시점을다음 단계로 이동(Push)과뒤로 가기(pop)로 구분하여 처리했습니다.내용은 아래와 같습니다.
BrowsingStore에isMovingToNextStep라는 State 플래그 추가true로 설정 (flag가 true라는 것은 정상적인 진행)true면 연결을 유지하고 기존 로직을 그대로 진행합니다.false면 **고의적인 뒤로 가기(취소)**로 판단하여browser.disconnect()호출 및 세션을 정리합니다.💬 리뷰 노트
BrowsingView에서 "뒤로 가기" 행위 자체가 사용자의 의도적인 "연결 취소"라고 생각했습니다.
따라서 촬영 기기에선 별도의 연결 상태 상실 알림 없이 즉시 연결을 해제하고 깔끔하게 초기 상태로 돌아가는 게 맞는 흐름이라고 판단했습니다!
📸 영상 / 이미지 (Optional)
📌 개선 전
default.mov
📌 개선 후
Note
개선 후 타이머 촬영이나 리모트 촬영, 사진 전송, 사진 저장 등의 메인 로직은 정상적으로 동작함을 확인했습니다!
default.mov