Merged
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and centralized error handling framework to improve the application's stability and user experience. It standardizes how errors are captured, logged, and presented to the user, particularly for network and server-side issues. The changes involve creating new error-related components, refactoring existing network layers, and integrating the new error handling capabilities into key ViewModels and UI elements throughout the application. Additionally, it includes minor UI adjustments for image rendering and article display, along with refinements to the authentication process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a comprehensive error handling and logging system across the app, improves UI on several screens, and refactors some features, aiming to significantly enhance code stability and maintainability. While the overall structure is sound, two security concerns were identified: unauthorized PII exposure via automatic clipboard copying and potential sensitive data leakage in application logs due to overly verbose error mapping. Additionally, some screens lack retry logic for errors, and the new error handling patterns are not consistently applied throughout the codebase. Addressing these points will improve user privacy, prevent accidental data exposure, and ensure a more robust and consistent error handling implementation.
Comment on lines
+114
to
+115
| onGoBack: { router.pop() }, | ||
| onRetry: { Task { await viewModel.loadPopularKeywords() } } |
There was a problem hiding this comment.
서버 오류 발생 시 재시도 로직이 loadPopularKeywords()만 호출하도록 고정되어 있습니다. 만약 사용자가 검색어를 입력한 후 검색 과정에서 오류가 발생했다면, 재시도 시 인기 검색어를 다시 불러오는 것이 아니라 현재 입력된 검색어로 검색을 재시도해야 합니다. viewModel.searchText의 상태에 따라 분기하여 적절한 재시도 동작을 수행하도록 수정해야 합니다.
onGoBack: { router.pop() },
onRetry: {
Task {
if viewModel.searchText.isEmpty {
await viewModel.loadPopularKeywords()
} else {
await viewModel.searchNewsletters()
}
}
}
Comment on lines
28
to
29
| let message = try? response.mapString() | ||
| return .serverError(statusCode: response.statusCode, message: message) |
There was a problem hiding this comment.
The ErrorMapper.map(response:) function maps the entire response body to a string and includes it in the NetworkError.serverError message if the response does not match the expected ServerErrorBody format. This error message is subsequently logged by the ErrorLogger. If the server response contains sensitive information (such as PII, tokens, or internal system details) in the body during an error condition, this information will be written to the application logs.
Comment on lines
+81
to
+84
| UserDefaults.standard.set(false, forKey: "isLoggedIn") | ||
| UserDefaults.standard.set(false, forKey: "isGuest") | ||
| UserDefaults.standard.removeObject(forKey: "nickname") | ||
| UserDefaults.standard.removeObject(forKey: "email") |
There was a problem hiding this comment.
UserDefaults에 사용되는 키를 문자열 리터럴로 직접 사용하는 것은 오타에 취약하고 유지보수를 어렵게 만듭니다. 이러한 키들을 별도의 상수나 열거형으로 관리하여 타입 안정성을 높이고 재사용성을 개선하는 것이 좋습니다.
private enum UserDefaultsKeys {
static let isLoggedIn = "isLoggedIn"
static let isGuest = "isGuest"
static let nickname = "nickname"
static let email = "email"
}
// ...
UserDefaults.standard.set(false, forKey: UserDefaultsKeys.isLoggedIn)
UserDefaults.standard.set(false, forKey: UserDefaultsKeys.isGuest)
UserDefaults.standard.removeObject(forKey: UserDefaultsKeys.nickname)
UserDefaults.standard.removeObject(forKey: UserDefaultsKeys.email)| .serverErrorPopup( | ||
| error: $viewModel.currentError, | ||
| onGoBack: { router.pop() }, | ||
| onRetry: {} |
There was a problem hiding this comment.
서버 오류 발생 시 사용자가 재시도할 수 있도록 onRetry 클로저를 구현해야 합니다. 현재 비어있어 오류 발생 시 사용자가 아무런 조치를 취할 수 없습니다. 로그인 로직을 다시 호출하도록 수정하는 것을 제안합니다.
onRetry: {
viewModel.login {
isLoggedIn = true
isGuest = false
router.resetTo(.tabbar(selectedTab: .home))
}
}| .serverErrorPopup( | ||
| error: $viewModel.currentError, | ||
| onGoBack: { router.pop() }, | ||
| onRetry: {} |
There was a problem hiding this comment.
서버 오류 팝업의 onRetry 핸들러가 비어있습니다. 회원가입 과정에서 네트워크 오류 등이 발생했을 때 사용자가 재시도를 할 수 없어 불편을 겪을 수 있습니다. 현재 단계에 맞는 재시도 로직을 구현해야 합니다. 예를 들어, viewModel에 마지막으로 시도한 작업을 재실행하는 메서드를 추가하는 것을 고려해볼 수 있습니다.
Comment on lines
70
to
77
| loadTask = Task { @MainActor in | ||
| isLoading = true | ||
| async let interests = useCase.fetchBookmarkedInterests() | ||
| async let articles = useCase.fetchBookmarkedArticles(interest: interest, sortBy: convertSortOrderToOption(sortOrder)) | ||
| do { | ||
| self.interests = try await interests | ||
| self.bookmarks = try await articles | ||
| } catch { | ||
| await performAsync(feature: "bookmark", operation: "loadInitial", loadingBinding: \.isLoading) { | ||
| async let fetchedInterests = useCase.fetchBookmarkedInterests() | ||
| async let fetchedArticles = useCase.fetchBookmarkedArticles(interest: interest, sortBy: convertSortOrderToOption(sortOrder)) | ||
| self.interests = try await fetchedInterests | ||
| self.bookmarks = try await fetchedArticles | ||
| } | ||
| isLoading = false | ||
| } |
There was a problem hiding this comment.
performAsync 헬퍼를 사용하여 비동기 작업과 오류 처리를 일관되고 간결하게 잘 리팩터링했습니다. 코드 가독성과 유지보수성이 크게 향상되었습니다.
Comment on lines
+168
to
170
| .onChange(of: viewModel.detail) { detail in | ||
| if detail != nil { isViewReady = true } | ||
| } |
There was a problem hiding this comment.
데이터가 로드된 후 뷰를 표시하는 로직을 onAppear의 asyncAfter 대신 onChange(of: viewModel.detail)로 변경한 점이 좋습니다. 이 방식은 데이터의 상태 변화에 직접 반응하므로 더 안정적이고 선언적입니다.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.