Conversation
opficdev
commented
Mar 16, 2026
opficdev
commented
- closed 무지성 로딩 플래그 난사를 줄인다 #278
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! 이 Pull Request는 로딩 상태 관리를 위한 새로운 Highlights
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
이 PR은 여러 ViewModel에 흩어져 있던 로딩 상태 관리 로직을 LoadingState라는 클래스로 추상화하여 코드의 재사용성과 유지보수성을 크게 향상시켰습니다. 특히 immediate와 delayed 모드를 두어 짧은 작업에 대한 불필요한 로딩 표시기 깜빡임을 방지하려는 접근이 매우 좋습니다. 전반적으로 코드 변경 사항은 일관성 있고 깔끔하게 적용되었습니다.
다만, LoadingState의 핵심 로직에서 immediate 로딩과 delayed 로딩이 연달아 발생하는 특정 엣지 케이스에서 로딩 표시기가 깜빡이는 버그를 발견했습니다. 이에 대한 수정 제안을 코드에 직접 댓글로 남겼으니 확인 부탁드립니다. 이 부분을 수정하면 더욱 견고한 코드가 될 것입니다.
| if 0 < delayedCountByTarget[target, default: 0] { | ||
| update(false) | ||
| scheduleDelayedLoadingIfNeeded(for: target, update: update) | ||
| return | ||
| } |
There was a problem hiding this comment.
현재 구현에는 로직 결함이 있어 특정 시나리오에서 로딩 표시기가 깜빡이는 현상(flickering)이 발생할 수 있습니다.
문제 시나리오:
begin(mode: .immediate)가 호출되어 로딩 표시기가 즉시 나타납니다.- 짧은 시간 내에
begin(mode: .delayed)가 호출됩니다.delayed작업을 위한Task가 예약됩니다. end(mode: .immediate)가 호출됩니다.immediateCount가 0이 되므로updateLoadingVisibility가 이 코드 블록으로 진입합니다.
현재 코드는 update(false)를 호출하여 로딩 표시기를 껐다가, 예약된 Task가 지연 시간 후에 완료되면 다시 켜게 됩니다. 이로 인해 불필요한 깜빡임이 발생하여 사용자 경험을 저해합니다.
해결 제안:
delayed 작업을 위한 Task가 이미 실행 중인지 확인하는 조건을 추가하여, immediate 로딩이 끝나더라도 delayed 로딩이 대기 중일 때는 로딩 표시기가 꺼지지 않도록 해야 합니다. 즉, 이미 Task가 있다면 update(false)를 호출하지 않아야 합니다.
| if 0 < delayedCountByTarget[target, default: 0] { | |
| update(false) | |
| scheduleDelayedLoadingIfNeeded(for: target, update: update) | |
| return | |
| } | |
| if 0 < delayedCountByTarget[target, default: 0] { | |
| if delayedTaskByTarget[target] == nil { | |
| update(false) | |
| } | |
| scheduleDelayedLoadingIfNeeded(for: target, update: update) | |
| return | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
이 PR은 여러 ViewModel에 분산되어 있던 로딩 상태 관리를 LoadingState라는 새로운 클래스로 중앙화하여 개선합니다. 이를 통해 코드 중복을 줄이고, 짧은 비동기 작업 시 로딩 인디케이터가 깜빡이는 현상을 방지하기 위해 지연된 로딩 표시 기능을 도입했습니다. 전반적으로 코드의 유지보수성과 사용자 경험을 향상시키는 좋은 변화입니다. 다만, undo 동작 처리 시 에러가 발생했을 때 로딩 상태가 제대로 해제되지 않는 버그가 발견되어 수정을 제안합니다.
| Task { | ||
| // defer을 통해 setLoading을 false로 제어하지 않는 이유 | ||
| // send(.fetchNotifications)를 통해 false로 처리될 것이기 때문 | ||
| send(.setLoading(true)) | ||
|
|
||
| // endLoading(.delayed)를 defer로 두지 않는 이유 | ||
| // send(.fetchNotifications)로 이어지는 즉시 로딩이 같은 isLoading을 이어서 제어해야 하기 때문 | ||
| do { | ||
| try await undoDeleteUseCase.execute(notificationId) | ||
| } catch { | ||
| send(.setAlert(isPresented: true)) | ||
| } | ||
|
|
||
| send(.fetchNotifications) | ||
| endLoading(.delayed) | ||
| } |
There was a problem hiding this comment.
undoDeleteUseCase.execute(notificationId)에서 에러가 발생할 경우 catch 블록으로 이동하지만 endLoading(.delayed)가 호출되지 않아 로딩 상태가 계속 유지되는 버그가 있습니다. defer를 사용하여 Task가 어떤 경로로든 종료될 때 endLoading이 항상 호출되도록 보장하는 것이 좋습니다.
주석에 defer를 사용하지 않는 이유가 언급되어 있지만, LoadingState의 로직을 고려할 때 defer를 사용해도 fetchNotifications의 immediate 로딩이 올바르게 동작하여 문제가 없습니다. defer를 사용하면 코드의 안정성과 가독성이 향상됩니다.
Task {
defer { endLoading(.delayed) }
do {
try await undoDeleteUseCase.execute(notificationId)
send(.fetchNotifications)
} catch {
send(.setAlert(isPresented: true))
}
}| Task { | ||
| // defer을 통해 setLoading을 false로 제어하지 않는 이유 | ||
| // send(.refresh)를 통해 false로 처리될 것이기 때문 | ||
| send(.setLoading(true)) | ||
|
|
||
| // endLoading(.delayed)를 defer로 두지 않는 이유 | ||
| // send(.refresh)로 이어지는 즉시 로딩이 같은 isLoading을 이어서 제어해야 하기 때문 | ||
| do { | ||
| try await undoDeleteTodoUseCase.execute(todoId) | ||
| } catch { | ||
| send(.setAlert(true)) | ||
| } | ||
|
|
||
| send(.refresh) | ||
| endLoading(.delayed) | ||
| } |
There was a problem hiding this comment.
PushNotificationListViewModel에서 발견된 문제와 동일한 버그가 있습니다. undoDeleteTodoUseCase.execute(todoId)에서 에러가 발생하면 endLoading(.delayed)가 호출되지 않아 로딩 인디케이터가 멈출 수 있습니다. defer를 사용하여 endLoading이 항상 호출되도록 수정하는 것이 좋습니다.
Task {
defer { endLoading(.delayed) }
do {
try await undoDeleteTodoUseCase.execute(todoId)
send(.refresh)
} catch {
send(.setAlert(isPresented: true))
}
}1 participant
