Feature/#134 일정 API 연결 및 기능 연동#135
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough이번 변경 사항은 출석 및 일정 관리 기능의 데이터 계층과 UI 계층을 대대적으로 확장하는 내용입니다. 새로운 데이터 모델( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ScheduleScreen
participant VM as ScheduleViewModel
participant Repo as ScheduleRepository
participant API as ScheduleApi
UI->>VM: ScheduleIntent(EnterScheduleScreen)
VM->>Repo: getSchedules(year, month)
Repo->>API: getSchedules(year, month)
API-->>Repo: DateGroupedScheduleResponse
Repo-->>VM: Flow<ScheduleList>
VM-->>UI: Update ScheduleState with schedules
sequenceDiagram
participant UI as AttendHistoryScreen
participant VM as AttendHistoryViewModel
participant UseCase as AttendHistoryUseCase
participant AttendRepo as AttendanceRepository
participant SessionRepo as SessionRepository
participant AttendAPI as AttendanceApi
participant SessionAPI as ScheduleApi
UI->>VM: AttendHistoryIntent.OnEntryScreen
VM->>UseCase: invoke()
UseCase->>AttendRepo: getAttendanceStatistics()
UseCase->>SessionRepo: getSessions()
AttendRepo->>AttendAPI: getAttendanceStatistics()
AttendAPI-->>AttendRepo: AttendStatisticsResponse
SessionRepo->>SessionAPI: getSessions()
SessionAPI-->>SessionRepo: List<SessionResponse>
AttendRepo-->>UseCase: Flow<AttendStatistics>
SessionRepo-->>UseCase: Flow<List<Sessions>>
UseCase-->>VM: Flow<Pair<AttendStatistics, List<Sessions>>>
VM-->>UI: Update AttendHistoryState
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jinukeu
left a comment
jinukeu
left a comment
There was a problem hiding this comment.
Draft 상태가 아니어서 일단 리뷰 드립니다 ... !
(회사에서 작성해서 코멘트를 신경써서 작성하지 못했어요. 이해 안가는 부분이 있다면 말씀해주세요!)
| LaunchedEffect(viewModel.store.sideEffects) { | ||
| viewModel.store.sideEffects.onEach { effect -> | ||
| when(effect) { | ||
| SideEffect.NavigateToBack -> { | ||
| navigateToBack() | ||
| } | ||
| } | ||
| }.launchIn(scope) | ||
| } |
There was a problem hiding this comment.
C: collectWihtLifeCycle 확장함수 만들어놔서 이거 써주시면 좋을거같아요
| data object OnEntryScreen : AttendHistoryIntent | ||
| data object OnClickBackButton : AttendHistoryIntent |
There was a problem hiding this comment.
C: Intent는 'On' 없이 그냥 EntryScreen으로 작성하고 있어서 컨벤션 맞춰주시면 감사하겠습니다.
| val alertMessage: String | ||
| get() { | ||
| return if (absenceCount >= 10) { | ||
| "결석이 10번 이상이 되면 출결에 문제가 생겨 재적처리가 될 수 있어요. 출석 관리에 조금 더 신경 써 주세요!" | ||
| } else { | ||
| "" | ||
| } | ||
| } |
There was a problem hiding this comment.
A: get을 쓰지 않더라도 매번 AttendHistoryState는 copy로 새로 생성되기 때문에 get이 없어도 될거에요
| fun fromApiValue(apiValue: String): ScheduleType { | ||
| return (entries.find { it.name == apiValue }) ?: ETC | ||
| } |
There was a problem hiding this comment.
C: 도메인 레이어에서 Api라는 Data 레이어를 위한 로직 처리가 있는 것 보다는 fromApiValue 로직 자체를 Data 레이어에서 처리하는건 어떠신가요?
There was a problem hiding this comment.
package com.yapp.model
data class NoticeList(
val notices: List<NoticeInfo>,
val lastNoticeId: String,
val hasNext: Boolean,
)
data class NoticeInfo(
val id: String,
val writerName: String,
val writerId: String,
val writerPosition: String,
val writerGeneration: Int,
val createdAt: String,
val title: String,
val content: String,
val noticeType: NoticeType
) {
val writerLabel = "${writerGeneration}기 $writerName"
}
enum class NoticeType(val apiValue: String, val displayValue: String) {
ALL("ALL", "전체"),
OPERATION("OPERATION", "운영"),
SESSION("SESSION", "세션");
companion object {
fun fromApiValue(apiValue: String): NoticeType {
return (entries.find { it.apiValue == apiValue }) ?: ALL
}
}
}기존에 작업되어있던 코드를 봤을 때 fromApiValue 로직이 domain layer에 있어서 우선 그렇게 했습니다
넵 근데 여러 곳에서 쓰일 수 있는 도메인 레이어 특성상 API에 의존적인 로직이 있는게 안 어울리긴 하네요....
There was a problem hiding this comment.
Data Layer쪽에 둔다면 단순히 dto 클래스안에 메소드를 만들지... 아니면 따로 Mapper 클래스를 만들지 고민이네요...
There was a problem hiding this comment.
아 저것도 옮기긴 해야할 것 같습니다 ㅜ
저는 Mapper 클래스는 뭔가 자바 느낌이라, dto 클래스 안에 메서드를 만들 것 같아요 (또는 확장함수로 처리)
| internal class AttendHistoryUseCase @Inject constructor( | ||
| private val attendanceRepository: AttendanceRepository, | ||
| private val sessionRepository: SessionRepository | ||
| ){ | ||
| operator fun invoke() = combine( | ||
| attendanceRepository.getAttendanceStatistics(), | ||
| sessionRepository.getSessions() | ||
| ) { attendStatistics, sessions -> | ||
| attendStatistics to sessions | ||
| } | ||
| } |
There was a problem hiding this comment.
A:
combine 좋네요.
getAttendanceStatistics(), getSessions()가 모두 flow로 되어있던데 일반 suspend 함수로 바꾼 후 UseCase에서 flowOf로 만들어서 처리하는건 어떠세요?
Repository 단에서는 최대한 기본 suspend 함수로 처리하면 좋을 것 같아요.
만약 getAttendanceStatistics()만 별도로 사용하게 되는 경우, flow는 오버헤드가 있을 것 같습니다.
|
|
||
| val uiState by viewModel.store.uiState.collectAsStateWithLifecycle() | ||
| val context = LocalContext.current | ||
| viewModel.store.sideEffects.collectWithLifecycle { effect -> |
There was a problem hiding this comment.
여기는 collectWithLifecycle가 잘 사용되고 있군요!
| if (it == scheduleState.selectedTabIndex) return@ScheduleTabRow | ||
| onIntent(ScheduleIntent.SelectTab(it)) |
There was a problem hiding this comment.
C: it == scheduleState.selectedTabIndex인 경우 아무 동작을 실행하지 않는 것 같은데요. 이 로직은 ViewModel로 위임하는게 더 좋을 것 같아요. UI로직은 아닌 것 같아서요!
| 0 -> ScheduleAllScreen( | ||
| selectedYear = scheduleState.selectedYear, | ||
| selectedMonth = scheduleState.selectedMonth, | ||
| schedules = scheduleState.schedules, | ||
| onIntent = onIntent | ||
| ) | ||
| 1 -> ScheduleSessionScreen() |
There was a problem hiding this comment.
A: 0, 1은 enum으로 처리하면 더 좋을 것 같습니다!
There was a problem hiding this comment.
넵 수정했습니다
ScheduleTab을 enum 으로 관리하니 관련 컴포넌트까지 다 변경하게 되네요
헉 Draft로 표시한다는 걸 깜빡했네요... 리뷰 감사합니다! |
DongChyeon
force-pushed
the
feature/#125-schedule-ui
branch
from
40ad273 to
44de13e
Compare
DongChyeon
force-pushed
the
feature/#134-connect-schedule-api
branch
from
edf5b5d to
5aa54c0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 46
🔭 Outside diff range comments (2)
feature/history/src/main/java/com/yapp/feature/history/navigation/HistoryNavigatoion.kt (1)
1-23:⚠️ Potential issue파일명에 오타가 있습니다.
파일 이름이 "HistoryNavigatoion.kt"로 되어 있는데, 올바른 철자는 "HistoryNavigation.kt"입니다. 코드 일관성 및 가독성을 위해 파일명을 수정하는 것이 좋겠습니다.
feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleScreen.kt (1)
233-237: 🧹 Nitpick (assertive)날짜 문자열 포매팅 시 두 자리 월 표시를 고려해 보세요
"${year}년 ${month}월"표현은 1월일 때2024년 1월로 출력됩니다.
디자인 요구 사항에 따라01월처럼 두 자리로 표시해야 할 수도 있으니"%02d".format(month)사용 여부를 한번 확인해 주세요.
♻️ Duplicate comments (4)
feature/history/src/main/java/com/yapp/feature/history/AttendHistoryUseCase.kt (1)
8-18: 리포지토리 API 설계에 대한 고려 사항이전 리뷰에서 제안된 것처럼, Repository에서 Flow 대신 suspend 함수를 사용하고 UseCase에서 필요에 따라 Flow로 변환하는 방식을 고려해보세요. 이 접근 방식은:
- Repository 계층을 더 단순하게 유지하고
- 데이터를 필요에 따라 한 번만 가져오거나 여러 번 가져올 수 있는 유연성을 제공하며
- Flow가 항상 필요하지 않은 경우 오버헤드를 줄일 수 있습니다
현재 구현에서 Flow를 사용하는 것이 실시간 데이터 업데이트가 필요한 경우에는 적절할 수 있지만, 단순히 데이터를 한 번 가져오는 경우라면 불필요한 복잡성을 추가할 수 있습니다.
feature/history/src/main/java/com/yapp/feature/history/AttendHistoryContract.kt (2)
16-23: get 프로퍼티를 사용하지 않아도 될 것 같습니다.이전 리뷰 코멘트에 언급된 것처럼,
AttendHistoryState는 매번 새로 생성되므로 get 프로퍼티를 사용하지 않아도 됩니다.- val alertMessage: String - get() { - return if (absenceCount >= 10) { - "결석이 10번 이상이 되면 출결에 문제가 생겨 재적처리가 될 수 있어요. 출석 관리에 조금 더 신경 써 주세요!" - } else { - "" - } - } + val alertMessage: String = if (absenceCount >= 10) { + "결석이 10번 이상이 되면 출결에 문제가 생겨 재적처리가 될 수 있어요. 출석 관리에 조금 더 신경 써 주세요!" + } else { + "" + }
27-28: Intent 네이밍 컨벤션을 맞춰주세요.이전 리뷰 코멘트에서 언급된 것처럼, Intent 네이밍에는 'On' 접두사를 사용하지 않는 것이 컨벤션입니다.
sealed interface AttendHistoryIntent { - data object OnEntryScreen : AttendHistoryIntent - data object OnClickBackButton : AttendHistoryIntent + data object EntryScreen : AttendHistoryIntent + data object ClickBackButton : AttendHistoryIntent }feature/history/src/main/java/com/yapp/feature/history/AttendHistoryScreen.kt (1)
55-63: 🧹 Nitpick (assertive)사용자 정의 확장 함수 사용을 고려하세요.
LaunchedEffect와onEach를 사용한 사이드 이펙트 수집 코드를 간결하게 만들 수 있습니다.개발팀에서 이미 만들어 둔
collectWihtLifeCycle확장함수를 사용하는 것이 코드를 더 간결하게 만들 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
core/data-api/src/main/java/com/yapp/dataapi/AttendanceRepository.kt(1 hunks)core/data-api/src/main/java/com/yapp/dataapi/ScheduleRepository.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/data/di/RepositoryModule.kt(2 hunks)core/data/src/main/java/com/yapp/core/data/data/repository/AttendanceRepositoryImpl.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/data/repository/ScheduleRepositoryImpl.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/api/AttendanceApi.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/api/ScheduleApi.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/di/ApiModule.kt(2 hunks)core/data/src/main/java/com/yapp/core/data/remote/model/response/AttendStatisticsResponse.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/model/response/DateGroupedScheduleResponse.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/model/response/SessionResponse.kt(1 hunks)core/model/src/main/java/com/yapp/model/AttendStatistics.kt(1 hunks)core/model/src/main/java/com/yapp/model/ScheduleInfo.kt(1 hunks)core/model/src/main/java/com/yapp/model/Sessions.kt(1 hunks)core/ui/src/main/java/com/yapp/core/ui/component/AttendanceStatusChip.kt(1 hunks)core/ui/src/main/java/com/yapp/core/ui/util/DateTime.kt(1 hunks)core/ui/src/main/res/values/strings.xml(1 hunks)feature/history/.gitignore(1 hunks)feature/history/build.gradle.kts(1 hunks)feature/history/proguard-rules.pro(1 hunks)feature/history/src/main/AndroidManifest.xml(1 hunks)feature/history/src/main/java/com/yapp/feature/history/AttendHistoryContract.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/AttendHistoryScreen.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/AttendHistoryUseCase.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/AttendHistoryViewModel.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/components/AttendanceStatusSession.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/components/SessionAttendanceItem.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/components/StatusItem.kt(1 hunks)feature/history/src/main/java/com/yapp/feature/history/navigation/HistoryNavigatoion.kt(1 hunks)feature/history/src/main/res/values/strings.xml(1 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleContract.kt(1 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleScreen.kt(3 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleViewModel.kt(1 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/component/AssignmentItem.kt(2 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/component/DateGroupedScheduleItem.kt(1 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/component/ScheduleTabRow.kt(5 hunks)feature/schedule/src/main/java/com/yapp/feature/schedule/component/SessionItem.kt(3 hunks)settings.gradle.kts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
feature/history/build.gradle.kts (1)
build-logic/src/main/java/com/yapp/AppNameExtension.kt (1)
setNamespace(6-10)
feature/history/src/main/java/com/yapp/feature/history/components/StatusItem.kt (1)
core/designsystem/src/main/java/com/yapp/core/designsystem/theme/Theme.kt (1)
YappTheme(9-24)
feature/history/src/main/java/com/yapp/feature/history/components/AttendanceStatusSession.kt (2)
core/designsystem/src/main/java/com/yapp/core/designsystem/theme/Theme.kt (1)
YappTheme(9-24)feature/history/src/main/java/com/yapp/feature/history/components/StatusItem.kt (1)
StatusItem(15-29)
feature/schedule/src/main/java/com/yapp/feature/schedule/component/DateGroupedScheduleItem.kt (4)
core/ui/src/main/java/com/yapp/core/ui/util/DateTime.kt (3)
isPastDate(52-60)formatToDay(9-16)formatTimeRange(18-22)feature/schedule/src/main/java/com/yapp/feature/schedule/component/SessionItem.kt (1)
SessionItem(20-66)feature/schedule/src/main/java/com/yapp/feature/schedule/component/AssignmentItem.kt (1)
AssignmentItem(15-43)core/designsystem/src/main/java/com/yapp/core/designsystem/theme/Theme.kt (1)
YappTheme(9-24)
feature/history/src/main/java/com/yapp/feature/history/AttendHistoryViewModel.kt (1)
core/ui/src/main/java/com/yapp/core/ui/mvi/MviIntentStore.kt (1)
mviIntentStore(53-60)
feature/history/src/main/java/com/yapp/feature/history/AttendHistoryScreen.kt (3)
core/designsystem/src/main/java/com/yapp/core/designsystem/theme/Theme.kt (1)
YappTheme(9-24)feature/history/src/main/java/com/yapp/feature/history/components/AttendanceStatusSession.kt (1)
AttendanceStatusSection(24-67)feature/history/src/main/java/com/yapp/feature/history/components/StatusItem.kt (1)
StatusItem(15-29)
feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleViewModel.kt (1)
core/ui/src/main/java/com/yapp/core/ui/mvi/MviIntentStore.kt (1)
mviIntentStore(53-60)
feature/history/src/main/java/com/yapp/feature/history/navigation/HistoryNavigatoion.kt (1)
feature/history/src/main/java/com/yapp/feature/history/AttendHistoryScreen.kt (1)
AttendHistoryRoute(30-64)
feature/schedule/src/main/java/com/yapp/feature/schedule/component/ScheduleTabRow.kt (1)
core/designsystem/src/main/java/com/yapp/core/designsystem/theme/Theme.kt (1)
YappTheme(9-24)
feature/schedule/src/main/java/com/yapp/feature/schedule/component/SessionItem.kt (1)
feature/schedule/src/main/java/com/yapp/feature/schedule/component/IconWithText.kt (1)
IconWithText(14-36)
🪛 detekt (1.23.7)
feature/history/src/main/java/com/yapp/feature/history/components/SessionAttendanceItem.kt
[warning] 9-11: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
core/model/src/main/java/com/yapp/model/Sessions.kt
[warning] 13-15: The class or object AttendType is empty.
(detekt.empty-blocks.EmptyClassBlock)
core/ui/src/main/java/com/yapp/core/ui/util/DateTime.kt
[warning] 13-13: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 47-47: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 57-57: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 13-13: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 47-47: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 57-57: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (27)
core/ui/src/main/res/values/strings.xml (1)
10-16: 새로운 시간 형식 문자열 리소스 추가
시간/분 포맷팅을 위한 문자열 리소스를 깔끔하게 잘 정의하셨습니다. 로컬라이제이션도 적절히 수행되었네요.feature/history/.gitignore (1)
1-1: 빌드 아티팩트 무시 설정
/build디렉토리를 무시하도록 한 것은 적절합니다.feature/history/src/main/AndroidManifest.xml (1)
1-4: 라이브러리 모듈namespace설정 확인 요청
AndroidManifest에package속성이 없는 상태이므로, AGP 7.0+ 환경에서build.gradle.kts의namespace설정을 통해 정상 병합되는지 검증이 필요합니다.
feature/history/build.gradle.kts에namespace = "com.yapp.feature.history"와 같은 설정이 존재하는지 확인해주세요.feature/history/proguard-rules.pro (1)
1-21: 스킵: 프로가드 설정 파일에 활성화된 룰이 없습니다.
현재 모든 내용이 주석 처리된 템플릿으로, 향후 필요 시 프로가드 룰을 추가할 예정이라면 문제 없습니다.feature/history/build.gradle.kts (1)
1-17: 모듈 설정 및 네임스페이스 확인
setNamespace("feature.history")가com.yapp.feature.history로 올바르게 설정되었고, 필요한 Compose 및 Material 의존성을 잘 추가했습니다.core/ui/src/main/java/com/yapp/core/ui/component/AttendanceStatusChip.kt (1)
17-30: 공유 모델 enum 사용으로 일관성 확보
로컬 정의한AttendanceStatus를 제거하고com.yapp.model.AttendanceStatus를 사용하여 도메인 모델과 UI 사이의 중복을 해소한 점이 적절합니다. 문자열 리소스 매핑도 문제 없어 보입니다.core/model/src/main/java/com/yapp/model/AttendStatistics.kt (1)
3-12: 잘 구현된 데이터 모델입니다!출석 통계를 위한 도메인 모델이 명확하게 설계되어 있습니다. 필드명이 직관적이고 모든 속성이 적절한 타입으로 정의되어 있습니다.
core/data/src/main/java/com/yapp/core/data/remote/api/AttendanceApi.kt (1)
11-12: 잘 구현된 API 메서드입니다통계 데이터를 가져오는 GET 요청이 적절하게 정의되어 있습니다.
core/data-api/src/main/java/com/yapp/dataapi/ScheduleRepository.kt (1)
1-11: 인터페이스가 잘 정의되었습니다.
ScheduleRepository인터페이스는 스케줄 관련 데이터를 비동기적으로 가져오는 두 가지 메서드를 명확하게 정의하고 있습니다. Kotlin Flow를 사용하여 반응형 프로그래밍 방식을 적절히 적용했습니다.core/data/src/main/java/com/yapp/core/data/remote/model/response/AttendStatisticsResponse.kt (1)
7-28: 응답 모델과 도메인 변환 로직이 잘 구현되었습니다.
AttendStatisticsResponse클래스와toModel()메서드를 통해 API 응답을 도메인 모델로 변환하는 로직이 깔끔하게 구현되었습니다.@Serializable어노테이션을 사용하여 JSON 직렬화/역직렬화를 적절히 처리하고 있습니다.feature/schedule/src/main/java/com/yapp/feature/schedule/component/AssignmentItem.kt (2)
17-20: ID 타입의 변경이 잘 적용되었습니다.기존 Long 타입에서 String 타입으로 ID 매개변수와 onClick 핸들러를 변경한 것은 백엔드 시스템과의 호환성을 높이는 좋은 접근입니다. UUID나 다른 문자열 기반 식별자를 사용하는 API와 더 잘 맞습니다.
22-41: UI 구성 요소가 적절히 단순화되었습니다.날짜 관련 정보와 과거/미래 상태에 따른 조건부 스타일링을 제거하고 제목과 내용만 표시하도록 변경되었습니다. 이는 관심사 분리에 좋은 접근이며, 날짜 관련 스타일링은 상위 컴포넌트(DateGroupedScheduleItem)에서 처리하는 방식이 코드 유지보수성을 높입니다.
feature/schedule/src/main/java/com/yapp/feature/schedule/component/DateGroupedScheduleItem.kt (2)
46-50: 오늘 날짜의 시각적 구분이 잘 구현되었습니다.오늘 날짜를
isToday플래그를 통해 기본 색상으로 강조하는 방식은 사용자 경험 측면에서 좋습니다. 현재 날짜를 시각적으로 구분해 사용자가 일정을 더 쉽게 파악할 수 있게 합니다.
40-40: 접근성 고려사항과거 날짜에 alpha 값 0.5f를 적용하는 것은 시각적 계층 구조를 만드는 데 효과적이지만, 낮은 대비로 인해 시각적 장애가 있는 사용자에게 접근성 문제를 일으킬 수 있습니다. 최소 대비 비율을 유지하면서 과거 항목을 구분할 수 있는 다른 방법(색상 변경, 레이블 추가 등)도 고려해보세요.
feature/history/src/main/java/com/yapp/feature/history/navigation/HistoryNavigatoion.kt (2)
9-14: 타입 안전 네비게이션 구현이 잘 되었습니다.Serializable data object 대상과 NavController 확장 함수를 사용한 네비게이션 구현은 타입 안전성을 보장하고 네비게이션 로직을 깔끔하게 캡슐화합니다. 모듈식 아키텍처에 잘 맞는 접근 방식입니다.
16-22: 백 네비게이션 처리가 잘 구현되었습니다.NavGraphBuilder 확장 함수는 백 네비게이션을 위한 콜백을 적절히 전달하여 사용자 경험을 잘 고려한 구현입니다. 각 화면의 책임을 명확히 분리하고 있습니다.
feature/history/src/main/java/com/yapp/feature/history/components/AttendanceStatusSession.kt (1)
69-99: 다양한 사용 사례를 보여주는 프리뷰가 잘 구현되었습니다.두 가지 다른 사용 사례(개인 출석 및 세션 상태)를 보여주는 프리뷰 구현이 좋습니다. 개발 및 디자인 검토 과정에서 컴포넌트가 다양한 상황에서 어떻게 보이는지 빠르게 확인할 수 있게 해줍니다.
core/ui/src/main/java/com/yapp/core/ui/util/DateTime.kt (1)
18-22: formatTimeRange 함수의 null 처리가 잘되어 있습니다.null이나 빈 문자열에 대한 검사를 통해 안전하게 함수를 구현했습니다.
feature/schedule/src/main/java/com/yapp/feature/schedule/component/ScheduleTabRow.kt (5)
31-37: Enum 타입 사용이 좋습니다.String 리스트 대신
ScheduleTabenum을 사용하여 타입 안전성을 높였습니다. 이는 오타나 잘못된 값을 방지하는 데 도움이 됩니다.
57-61: 코드가 더 명확하고 유지보수하기 쉬워졌습니다.인덱스 비교 대신 enum의 index 속성을 비교하도록 변경한 것이 좋습니다. 이렇게 하면 코드의 의도가 더 명확해집니다.
68-73: 문자열 리소스 ID 사용은 좋은 방식입니다.하드코딩된 문자열 대신 리소스 ID를 사용하는 것은 다국어 지원과 유지보수성을 향상시킵니다.
84-86: stringResource 함수 사용이 잘 되었습니다.문자열을 하드코딩하는 대신
stringResource를 사용하여 리소스에서 문자열을 가져오는 것이 좋습니다.
107-115: enum 사용과 상태 관리 개선이 잘 되었습니다.ViewModel에서 상태로 관리되는
selectedTab을 사용하고, 전체 enum 항목을tabList로 사용하는 것이 간결하고 유지보수하기 좋습니다.feature/history/src/main/java/com/yapp/feature/history/AttendHistoryScreen.kt (3)
36-38: onIntent를 LaunchedEffect 내부에서 호출하는 것이 좋습니다.화면 진입 시 Intent를 발행하는 패턴이 잘 적용되었습니다.
86-102: UI 구조가 잘 구성되어 있습니다.상태 섹션이 명확하게 나누어져 있고, 각 섹션에 적절한 여백을 사용하여 가독성이 좋습니다.
103-120: 코드 재사용이 잘 되어 있습니다.
AttendanceStatusSection과StatusItem컴포넌트를 재사용하여 코드 중복을 피하고 일관된 UI를 유지하는 좋은 접근 방식입니다.core/data/src/main/java/com/yapp/core/data/remote/model/response/DateGroupedScheduleResponse.kt (1)
120-128: API 신규 상태가 들어오면 ‘SCHEDULED’로 묻혀 버립니다
else -> AttendanceStatus.SCHEDULED로 fallback 하는 것은 안전장치지만,
미지원 값이 들어왔을 때 로그를 남기거나UNKNOWN값을 두어 문제를 조기에 인지하는 편이 좋습니다.
무심코 잘못된 통계를 보게 될 여지가 있습니다.
| ":feature:login", | ||
| ":feature:schedule", | ||
| ":feature:profile", | ||
| ":featrue:history" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
모듈 경로 오타 수정 필요
include에 등록된 모듈명이 "feature"가 아닌 "featrue"로 오타가 있어 프로젝트 동기화 오류가 발생합니다.
아래와 같이 수정해주세요:
- ":featrue:history"
+ ":feature:history"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ":featrue:history" | |
| ":feature:history" |
| <?xml version="1.0" encoding="utf-8"?> | ||
| <resources> | ||
| <string name="attendance_title">출석내역</string> | ||
| <string name="attendance_my_status">내 출석 현황</string> | ||
| <string name="attendance_total_score">총 점수</string> | ||
| <string name="attendance_item">출석</string> | ||
| <string name="late_item">지각</string> | ||
| <string name="absence_item">결석</string> | ||
| <string name="latePass_item">지각 면제권</string> | ||
| <string name="session_title">세션 현황</string> | ||
| <string name="total_session_count_title">전체 세션</string> | ||
| <string name="total_remain_session_count_title">남은 세션</string> | ||
| <string name="total_remain_session_progress_rate_title">진행률</string> | ||
| </resources> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
리소스 키 네이밍 일관성 검토
대부분 snake_case를 사용하시는 중에 "latePass_item"만 camelCase로 혼용되고 있습니다. 일관성을 위해 "late_pass_item"으로 변경을 권장드립니다.
- <string name="latePass_item">지각 면제권</string>
+ <string name="late_pass_item">지각 면제권</string>코드에서 해당 리소스를 참조하는 부분도 함께 업데이트해 주세요.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <?xml version="1.0" encoding="utf-8"?> | |
| <resources> | |
| <string name="attendance_title">출석내역</string> | |
| <string name="attendance_my_status">내 출석 현황</string> | |
| <string name="attendance_total_score">총 점수</string> | |
| <string name="attendance_item">출석</string> | |
| <string name="late_item">지각</string> | |
| <string name="absence_item">결석</string> | |
| <string name="latePass_item">지각 면제권</string> | |
| <string name="session_title">세션 현황</string> | |
| <string name="total_session_count_title">전체 세션</string> | |
| <string name="total_remain_session_count_title">남은 세션</string> | |
| <string name="total_remain_session_progress_rate_title">진행률</string> | |
| </resources> | |
| <?xml version="1.0" encoding="utf-8"?> | |
| <resources> | |
| <string name="attendance_title">출석내역</string> | |
| <string name="attendance_my_status">내 출석 현황</string> | |
| <string name="attendance_total_score">총 점수</string> | |
| <string name="attendance_item">출석</string> | |
| <string name="late_item">지각</string> | |
| <string name="absence_item">결석</string> | |
| <string name="late_pass_item">지각 면제권</string> | |
| <string name="session_title">세션 현황</string> | |
| <string name="total_session_count_title">전체 세션</string> | |
| <string name="total_remain_session_count_title">남은 세션</string> | |
| <string name="total_remain_session_progress_rate_title">진행률</string> | |
| </resources> |
| @Composable | ||
| internal fun SessionAttendanceItem( | ||
| date: String, | ||
| time: String? = null, | ||
| ) { | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
빈 Composable 함수 구현 필요
현재 SessionAttendanceItem이 본문 없이 빈 함수로 정의되어 있습니다. UI 요소가 없으면 화면에 아무것도 그려지지 않으니, 실제 디자인에 맞춘 레이아웃 또는 최소한 // TODO: 구현 주석을 추가하여 향후 작업 계획을 명확히 남겨주세요.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 9-11: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
| interface AttendanceRepository { | ||
| fun getAttendanceStatistics(): Flow<AttendStatistics> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
인터페이스에 KDoc 추가 고려
AttendanceRepository와 getAttendanceStatistics 메서드에 설명을 위한 KDoc을 추가하면, 나중에 IDE에서 문서화를 확인하거나 API 사용 시 이해도를 높일 수 있습니다.
| fun providesSessionsApi(@AuthRetrofit retrofit: Retrofit): ScheduleApi { | ||
| return retrofit.create(ScheduleApi::class.java) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
메서드 이름의 일관성 개선 필요
메서드 이름이 현재 providesSessionsApi로 되어 있는데 다른 모든 메서드들은 provide로 시작합니다. 코드 일관성을 위해 provideSessionsApi 또는 provideScheduleApi로 변경하는 것이 좋겠습니다.
- fun providesSessionsApi(@AuthRetrofit retrofit: Retrofit): ScheduleApi {
+ fun provideScheduleApi(@AuthRetrofit retrofit: Retrofit): ScheduleApi {
return retrofit.create(ScheduleApi::class.java)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun providesSessionsApi(@AuthRetrofit retrofit: Retrofit): ScheduleApi { | |
| return retrofit.create(ScheduleApi::class.java) | |
| } | |
| fun provideScheduleApi(@AuthRetrofit retrofit: Retrofit): ScheduleApi { | |
| return retrofit.create(ScheduleApi::class.java) | |
| } |
| IconWithText( | ||
| iconResId = R.drawable.icon_location, | ||
| text = location, | ||
| contentDescription = null, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
접근성 향상을 위해 contentDescription을 채워 주세요
아이콘의 contentDescription이 null로 전달되어 스크린리더가 해당 정보를 건너뜁니다.
장애인 접근성을 고려해 "시간 정보", "위치 정보" 등 의미 있는 문자열을 지정해 주시면 좋겠습니다.
| duration?.let { | ||
| IconWithText( | ||
| iconResId = R.drawable.icon_time, | ||
| text = duration, | ||
| contentDescription = null, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
duration 가 없을 때 불필요한 여백이 생깁니다
Spacer(4.dp)가 duration 블록 바깥에 있어, duration 과 location 모두 null일 때도 빈 공간이 렌더링됩니다.
조건부 UI 안쪽으로 옮기면 예상치 못한 여백을 방지할 수 있습니다.
- duration?.let {
- IconWithText(
- iconResId = R.drawable.icon_time,
- text = duration,
- contentDescription = null,
- )
- }
-
- Spacer(modifier = Modifier.height(4.dp))
+ duration?.let {
+ IconWithText(
+ iconResId = R.drawable.icon_time,
+ text = it,
+ contentDescription = null,
+ )
+ Spacer(modifier = Modifier.height(4.dp))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| duration?.let { | |
| IconWithText( | |
| iconResId = R.drawable.icon_time, | |
| text = duration, | |
| contentDescription = null, | |
| ) | |
| } | |
| duration?.let { | |
| IconWithText( | |
| iconResId = R.drawable.icon_time, | |
| text = it, | |
| contentDescription = null, | |
| ) | |
| Spacer(modifier = Modifier.height(4.dp)) | |
| } |
| companion object { | ||
| fun toScheduleType(value: String): ScheduleType = | ||
| when (value) { | ||
| "SESSION" -> ScheduleType.SESSION | ||
| "TASK" -> ScheduleType.TASK | ||
| else -> ScheduleType.ETC | ||
| } | ||
|
|
||
| fun toSessionType(value: String?): SessionType? = |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
문자열 → enum 매핑 로직이 중복되고 확장에 약합니다
when 블록을 enum 개수만큼 반복하면 새 값 추가 시 세 곳을 모두 수정해야 합니다.
enum.entries.firstOrNull { it.name == value } ?: DEFAULT 패턴으로 일반화하거나, 각 enum 클래스 안에 fromApiValue 를 확장 함수로 두면 중복을 없앨 수 있습니다.
fun String.toScheduleType() =
ScheduleType.entries.firstOrNull { it.name == this } ?: ScheduleType.ETC이렇게 하면 가독성과 유지보수성이 향상됩니다.
| items(items = schedules.dates) { | ||
| DateGroupedScheduleItem( | ||
| date = it.date, | ||
| dayOfWeek = it.dayOfTheWeek, | ||
| isToday = it.isToday, | ||
| schedules = it.schedules, | ||
| ) { } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LazyColumn items에 key를 지정해 리스트 성능을 높여 주세요
schedules.dates 의 각 원소는 날짜 기준으로 고유하므로 key = { it.date } 정도를 주면
아이템 재구성 시 불필요한 recomposition 및 스크롤 위치 잃어버림을 방지할 수 있습니다.
items(
items = schedules.dates,
key = { it.date }
) { dateGroup -> /* ... */ }| val uiState by viewModel.store.uiState.collectAsStateWithLifecycle() | ||
| val context = LocalContext.current | ||
| viewModel.store.sideEffects.collectWithLifecycle { effect -> | ||
| when (effect) { | ||
| is ScheduleSideEffect.ShowToast -> { | ||
| Toast.makeText(context, effect.message, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
collectWithLifecycle가 재컴포지션마다 중복 구독될 가능성이 있습니다
viewModel.store.sideEffects.collectWithLifecycle { ... } 호출이 ScheduleRoute 컴포저블 본문에 직접 위치해 있습니다.
재컴포지션이 잦은 화면이라면 Flow 구독이 여러 번 생성되어 Toast 가 중복 표시되거나 불필요한 리소스가 낭비될 수 있습니다.
LaunchedEffect(Unit) 블록 안으로 옮겨 한 번만 collect 되도록 하는 편이 안전합니다.
- viewModel.store.sideEffects.collectWithLifecycle { effect ->
+ LaunchedEffect(Unit) {
+ viewModel.store.sideEffects.collectWithLifecycle { effect ->
/* ... */
- }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val uiState by viewModel.store.uiState.collectAsStateWithLifecycle() | |
| val context = LocalContext.current | |
| viewModel.store.sideEffects.collectWithLifecycle { effect -> | |
| when (effect) { | |
| is ScheduleSideEffect.ShowToast -> { | |
| Toast.makeText(context, effect.message, Toast.LENGTH_SHORT).show() | |
| } | |
| } | |
| } | |
| val uiState by viewModel.store.uiState.collectAsStateWithLifecycle() | |
| val context = LocalContext.current | |
| LaunchedEffect(Unit) { | |
| viewModel.store.sideEffects.collectWithLifecycle { effect -> | |
| when (effect) { | |
| is ScheduleSideEffect.ShowToast -> { | |
| Toast.makeText(context, effect.message, Toast.LENGTH_SHORT).show() | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.compose.LocalLifecycleOwner
import androidx.lifecycle.repeatOnLifecycle
import kotlinx.coroutines.flow.Flow
@Composable
inline fun <reified T> Flow<T>.collectWithLifecycle(
minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
noinline action: suspend (T) -> Unit,
) {
val lifecycleOwner = LocalLifecycleOwner.current
LaunchedEffect(this, lifecycleOwner) {
lifecycleOwner.lifecycle.repeatOnLifecycle(minActiveState) {
this@collectWithLifecycle.collect { action(it) }
}
}
}
collectWithLifecycle의 내부 구현을 살펴보면 이미 LaunchedEffect로 관리되고 있어. 다시 한번 검토해줘.
There was a problem hiding this comment.
@jinukeu 네, 맞습니다! 제가 코드를 다시 검토해보니 collectWithLifecycle 확장 함수가 이미 내부적으로 LaunchedEffect를 사용하고 있군요. 따라서 이를 다시 LaunchedEffect로 감싸는 것은 불필요합니다.
@Composable
inline fun <reified T> Flow<T>.collectWithLifecycle(
minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
noinline action: suspend (T) -> Unit,
) {
val lifecycleOwner = LocalLifecycleOwner.current
LaunchedEffect(this, lifecycleOwner) { // 이미 LaunchedEffect 포함
lifecycleOwner.lifecycle.repeatOnLifecycle(minActiveState) {
this@collectWithLifecycle.collect { action(it) }
}
}
}Flow와 lifecycleOwner를 키로 사용하여 재구성 시에도 한 번만 실행되도록 이미 설계되어 있습니다. 제 제안은 중첩된 LaunchedEffect를 만들어 불필요한 복잡성을 추가할 뿐이었네요. 지적해 주셔서 감사합니다!
✏️ Learnings added
Learnt from: jinukeu
PR: YAPP-admin/yappu-world-android#135
File: feature/schedule/src/main/java/com/yapp/feature/schedule/ScheduleScreen.kt:48-56
Timestamp: 2025-04-26T17:07:21.372Z
Learning: `collectWithLifecycle` 확장 함수는 이미 내부적으로 LaunchedEffect를 사용하여 재컴포지션 시에도 Flow가 한 번만 구독되도록 설계되어 있으므로, 이를 추가적인 LaunchedEffect로 감싸는 것은 불필요하다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
오옹 안에 있는 코드까지 제대로 안봤었네요... 확인해주셔서 감사합니다
|
@jinukeu @TaeseongYun 리뷰 가능하신가요?? |
아 review re request가 안되어서 아직 작업 중이신 줄 알았어요 지금 하겠습니당 |
| companion object { | ||
| fun toScheduleType(value: String): ScheduleType = | ||
| when (value) { | ||
| "SESSION" -> ScheduleType.SESSION | ||
| "TASK" -> ScheduleType.TASK | ||
| else -> ScheduleType.ETC | ||
| } | ||
|
|
||
| fun toSessionType(value: String?): SessionType? = |
| ALL(0, R.string.schedule_tab_all), | ||
| SESSION(1, R.string.schedule_tab_session); | ||
|
|
||
| companion object { | ||
| fun fromIndex(index: Int): ScheduleTab { | ||
| return entries.find { it.index == index } ?: ALL | ||
| } | ||
| } |
There was a problem hiding this comment.
A:
index를 없애고 ordinal를 사용해도 좋을 것 같아요
There was a problem hiding this comment.
넵 코드 래빗 리뷰 반영했고... fromIndex 메소드는 현재 사용하지 않아서 제거했습니다.
There was a problem hiding this comment.
감사합니다. 어프로브는 이미 해놔서 머지하셔도 됩니다요!
|
@TaeseongYun 그래서 태성님 PR을 먼저 병합하고 나서 해당 PR은 충돌을 해결해서 병합하는 게 좋다고 생각하는데 |
크게 생각해둔 브랜치 전력이 없긴했는데 제 피알에 코멘트가 달려 수정하는데 좀 걸릴거 같아 해당 피알을 먼저 선 반영해도 될듯합니다. |
TaeseongYun
left a comment
TaeseongYun
left a comment
There was a problem hiding this comment.
코멘트 남긴거 반영하셔도 좋고 처리하지 않으셔도 됩니다 ~
작업 고생하셨습니다 !
| fun isPastDate(dateString: String, pattern: String = "yyyy-MM-dd"): Boolean { | ||
| return try { | ||
| val formatter = DateTimeFormatter.ofPattern(pattern) | ||
| val inputDate = LocalDate.parse(dateString, formatter) | ||
| inputDate.isBefore(LocalDate.now()) | ||
| } catch (e: Exception) { | ||
| false | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
C : 오전 오후를 표시하고 싶으면 a 패턴을 사용하면 간편하게 사용할 수 있습니다 ~
https://developer.android.com/reference/java/time/format/DateTimeFormatter
DongChyeon
force-pushed
the
feature/#134-connect-schedule-api
branch
from
4c70229 to
e3e5a80
Compare
3 participants
💡 Issue
🌱 Key changes
✅ To Reviewers
시간, 날짜 포맷팅 유틸 함수 어디가 넣을지 고민하다가 stringResource를 사용하기도 하고
보여주는 레이어에서만 사용한다고 생각해서 ui:util 모듈에 넣었습니다. 혹시 문제가 되면 말씀해주세요.
📸 스크린샷
2025-04-25.3.14.44.mov
Summary by CodeRabbit
신규 기능
UI/UX 개선
문서 및 리소스
버그 수정 및 리팩터