[Refactor/#323] 홈화면을 MVVM -> MVI 로 마이그레이션합니다. #327
Conversation
…> DailyCloverType로 이름을 변경합니다.
- 홈화면에서 연/월이 변경될 때 변경된 월의 1일을 기준으로 캘린더+데일리 일기 정보를 불러오는데 그것만을 위해 함수가 하나 더 있어서 1일을 넘기는 식으로 변경하여 통일했습니다.
- 인앱리뷰는 테스트 앱이라 플레이스토어 요청이 불가하여 테스트가 어렵습니다. 콘솔에 테스트앱을 따로 업로드 해야함.
WalkthroughMigrates Home from MVVM to MVI (Mavericks): adds HomeContract, intents/side-effects, and Mavericks HomeViewModel; introduces new calendar and diary UI components and domain models; removes legacy calendar composables and UI state types; adds deep-link activity and tweaks imports/type package locations and timezone utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as HomeScreen (Compose)
participant VM as HomeViewModel (Mavericks)
participant Repo as Repository/UseCases
participant SE as SideEffects
User->>UI: Launch Home
UI->>VM: postIntent(InitializeInfo(year,month,day))
VM->>Repo: load calendar & daily info
Repo-->>VM: CalendarMonthlyInfo, DailyDiaryInfo
VM->>VM: setState -> update HomeState
VM->>UI: state update
UI-->>User: Render MonthlyCalendar & DailyDiary
User->>UI: Tap day
UI->>VM: postIntent(OnClickDay(year,month,day))
VM->>Repo: fetch day data
Repo-->>VM: DailyDiaryInfo
VM->>VM: setState(selectedDate, dailyInfo)
VM->>UI: state update
UI-->>User: Update UI
sequenceDiagram
autonumber
actor OS as Android
participant AM as AndroidManifest
participant DL as ClodyDeeplinkActivity
participant Air as Airbridge SDK
participant Nav as App Navigator
OS->>AM: Resolve deep link (clody:// or https://clody.abr.ge / clody.airbridge.io)
AM-->>OS: Launch ClodyDeeplinkActivity
OS->>DL: onCreate/onResume
DL->>Air: handleDeferredDeeplink(callback)
Air-->>DL: callback(uri?)
alt uri != null
DL->>Nav: navigate based on uri
else
DL-->>OS: finish/no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
SYAAINN
changed the base branch from
develop
to
refactor/#317-notification-request
MoonsuKang
left a comment
MoonsuKang
left a comment
There was a problem hiding this comment.
대규모 공사 수고하셨습니다...
리뷰는 궁금한거 위주로 남겼어요
| package com.sopt.clody.data.remote.dto.response | ||
|
|
||
| import com.sopt.clody.domain.model.ReplyStatus | ||
| import com.sopt.clody.domain.type.ReplyStatus |
There was a problem hiding this comment.
API 변경 시 Domain 영향 최소화하려면 ReplyStatusDto - Mapper로 나중에 바꾸는 것도 좋아보입니다.
클린 아키텍처에 따르면 말이죠
| * */ | ||
| data class CalendarMonthlyInfo( | ||
| val totalCloverCount: Int = 0, | ||
| val calendarDailyInfoList: List<CalendarDailyInfo> = listOf(), |
There was a problem hiding this comment.
불변성을 보장한다면 emptyList()로 바꾸는 게 좋습니다
| onShowDiaryDeleteStateChange: (Boolean) -> Unit, | ||
| fun DailyDiary( | ||
| selectedDate: LocalDate, | ||
| selectedDailyInfo: DailyDiaryInfo, |
There was a problem hiding this comment.
컴포저블에서 domain layer의 model을 참조하고 있는데, 사실 이러면 아키텍처와 디자인 패턴 두가지 관점에서 좋지 않습니다.
UI State만으로 해도 되고, 정말 clean architecture 원칙을 지킬 것 이라면 uimodel또한 필요합니다.
| fun DailyStateButton( | ||
| calendarDailyInfo: CalendarMonthlyInfo.CalendarDailyInfo, | ||
| selectedDailyInfo: DailyDiaryInfo, | ||
| onClickWriteDiary: () -> Unit, | ||
| onClickContinueDraft: () -> Unit, |
There was a problem hiding this comment.
여기도 위와 비슷한 케이스 입니다.
Domain Model이 UI Layer에 직접 노출하는 구조로 되어있는거죠.
| Column( | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) { |
| fun getCalendarDailyInfo(): CalendarMonthlyInfo.CalendarDailyInfo? { | ||
| return if (isCalendarDataLoaded()) { | ||
| calendarMonthlyInfo.calendarDailyInfoList[dayOfMonth - 1] | ||
| } else { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
State내부에서 비즈니스 로직을 처리한 이유가 있을까요? 궁금해서유
SYAAINN
changed the base branch from
refactor/#317-notification-request
to
develop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
app/src/main/AndroidManifest.xml (1)
68-99: Add lifecycle attributes for better deep link handling UX.The deep link handler activity lacks attributes to prevent it from cluttering the back stack. Consider adding
android:noHistory="true"to ensure the activity is removed after the user navigates away, and potentiallyandroid:launchMode="singleTask"to prevent multiple instances.Apply this diff:
<activity android:name=".ClodyDeeplinkActivity" - android:exported="true"> + android:exported="true" + android:noHistory="true" + android:launchMode="singleTask">app/src/main/java/com/sopt/clody/ClodyDeeplinkActivity.kt (1)
9-11: Add setContentView or document the transparent Activity pattern.The Activity has no content view set, which means it will be invisible to the user. If this is intentional for a transparent deep link handler, consider adding a comment to clarify the design. Otherwise, this could lead to confusion during maintenance.
app/src/main/java/com/sopt/clody/presentation/ui/type/DailyStateButtonType.kt (1)
19-21: Simplify using the existingisUnreadOrNotReadproperty.The condition checking
READY_NOT_READandUNREADYcan leverage theisUnreadOrNotReadproperty already defined inReplyStatusfor better maintainability.Apply this diff to simplify the logic:
- calendarDailyInfo.replyStatus == ReplyStatus.READY_READ || - calendarDailyInfo.replyStatus == ReplyStatus.READY_NOT_READ || - (calendarDailyInfo.replyStatus == ReplyStatus.UNREADY && calendarDailyInfo.diaryCount > 0) -> REPLY_ENABLED + calendarDailyInfo.replyStatus == ReplyStatus.READY_READ || + (calendarDailyInfo.replyStatus.isUnreadOrNotRead && calendarDailyInfo.diaryCount > 0) -> REPLY_ENABLED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/sopt/clody/ClodyDeeplinkActivity.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyCalendarResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyDiaryResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/model/CalendarMonthlyInfo.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/model/DailyDiaryInfo.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/model/ExampleModel.kt(0 hunks)app/src/main/java/com/sopt/clody/domain/type/Notification.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/type/ReplyStatus.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/di/ViewModelsModule.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/MonthlyDiaryList.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/ClodyCalendar.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/DayItem.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/HorizontalDivider.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/MonthlyItem.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/WeekHeader.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/CalendarDateData.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/DiaryDateData.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/CloverCount.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/DailyDiary.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/DailyStateButton.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/HomeTopAppBar.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendarAndDailyDiary.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/CalendarState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/DailyDiariesState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/DeleteDiaryState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/ScrollableCalendar.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/type/DailyCloverType.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/type/DailyStateButtonType.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/type/DiaryCloverType.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/utils/base/UiLoadState.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt(1 hunks)
💤 Files with no reviewable changes (14)
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/WeekHeader.kt
- app/src/main/java/com/sopt/clody/domain/model/ExampleModel.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/DailyDiariesState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/DeleteDiaryState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/CalendarState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/component/CloverCount.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/HorizontalDivider.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/DayItem.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/DiaryDateData.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/CalendarDateData.kt
- app/src/main/java/com/sopt/clody/presentation/ui/type/DiaryCloverType.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/ClodyCalendar.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/ScrollableCalendar.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/MonthlyItem.kt
🧰 Additional context used
🧬 Code graph analysis (4)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/DailyStateButton.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt (1)
ClodyButton(15-44)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendarAndDailyDiary.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt (1)
MonthlyCalendar(37-138)app/src/main/java/com/sopt/clody/presentation/ui/home/component/DailyDiary.kt (1)
DailyDiary(31-125)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (1)
sendNotification(36-72)app/src/main/java/com/sopt/clody/data/repositoryimpl/NotificationRepositoryImpl.kt (1)
sendNotification(19-22)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (14)
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt (1)
navigateToDiaryList(33-38)app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt (1)
navigateToSetting(46-50)app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigation.kt (1)
navigateToWriteDiary(30-37)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt (1)
navigateToReplyLoading(33-45)app/src/main/java/com/sopt/clody/presentation/ui/component/FailureScreen.kt (1)
FailureScreen(27-69)app/src/main/java/com/sopt/clody/presentation/ui/home/component/HomeTopAppBar.kt (1)
HomeTopAppBar(24-89)app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendarAndDailyDiary.kt (1)
MonthlyCalendarAndDailyDiary(26-86)app/src/main/java/com/sopt/clody/presentation/ui/home/component/DailyStateButton.kt (1)
DailyStateButton(14-73)app/src/main/java/com/sopt/clody/presentation/ui/component/popup/ClodyPopupBottomSheet.kt (1)
ClodyPopupBottomSheet(28-77)app/src/main/java/com/sopt/clody/presentation/ui/component/timepicker/YearMonthPicker.kt (1)
YearMonthPicker(53-185)app/src/main/java/com/sopt/clody/presentation/ui/component/bottomsheet/DiaryDeleteSheet.kt (1)
DiaryDeleteSheet(26-40)app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/ClodyDialog.kt (1)
ClodyDialog(36-149)app/src/main/java/com/sopt/clody/presentation/ui/component/toast/ClodyToastMessage.kt (1)
ClodyToastMessage(27-66)app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/FailureDialog.kt (1)
FailureDialog(32-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (28)
app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
8-8: LGTM – package reorganization complete. AllReplyStatusimports now referencecom.sopt.clody.domain.type.ReplyStatus; no olddomain.modelimports remain.app/src/main/java/com/sopt/clody/domain/type/Notification.kt (1)
1-5: LGTM! Clean package reorganization.The package move to
domain.typeis appropriate for consolidating domain type definitions. The enum remains unchanged.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (1)
25-25: LGTM! Import correctly updated.The import path aligns with the package reorganization. All usages of the Notification enum remain unchanged and correct.
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (1)
10-10: No stale imports found
No occurrences ofcom.sopt.clody.domain.Notificationremain; import update is complete.app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt (2)
110-114: LGTM! Material3 HorizontalDivider properly configured.The migration from a local component to Material3's
HorizontalDivideris correct, and thefillMaxWidth()modifier ensures the divider spans the full width as intended.
16-16: Local HorizontalDivider component removal confirmed. No matching imports or definitions remain.app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (1)
169-173: LGTM! Consistent with the Material3 migration.The
HorizontalDividerimplementation matches the pattern used inTermsOfServicePage.kt, ensuring consistency across the auth flow screens.app/src/main/AndroidManifest.xml (1)
80-88: Digital Asset Links are correctly configured
Both assetlinks.json files on clody.abr.ge and clody.airbridge.io include thecom.sopt.clodypackage name and required SHA-256 fingerprints, soandroid:autoVerify="true"will work as intended.app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
69-83: Ensure diary timestamp doesn’t shift the target date
Combining the input date with the user’s current time then converting zones can cross midnight, e.g. Jan 1 23:30 PST → Jan 2 KST. Either always use the diary’s date at 00:00 KST (e.g. LocalDate.atStartOfDay(kstZone)) or confirm the server groups entries by the date portion of this timestamp.app/src/main/java/com/sopt/clody/domain/type/ReplyStatus.kt (1)
1-11: LGTM! Package relocation aligns with domain organization.The move from
domain.modeltodomain.typeproperly categorizesReplyStatusas a domain-level type. The enum logic remains unchanged.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/MonthlyDiaryList.kt (1)
12-12: LGTM! Import path updated correctly.The import now references the relocated
ReplyStatustype.app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyCalendarResponseDto.kt (1)
3-3: LGTM! Import path updated correctly.The import now references the relocated
ReplyStatustype.app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyDiaryResponseDto.kt (1)
3-3: LGTM! Import path updated correctly.The import now references the relocated
ReplyStatustype. Note that a previous reviewer suggested introducing a mapper pattern (ReplyStatusDto → Domain) to minimize domain impact from API changes, which could be considered in a future refactor.app/src/main/java/com/sopt/clody/presentation/ui/type/DailyStateButtonType.kt (1)
15-24: Verify the condition precedence for edge cases.The
whenexpression evaluates conditions sequentially. If a diary is both deleted (isDeleted = true) and has a draft (isDraft = true), the current logic returnsREPLY_DISABLED(Line 16) without considering the draft state. Confirm this precedence is intentional for the UX flow.app/src/main/java/com/sopt/clody/presentation/ui/type/DailyCloverType.kt (2)
38-40: Verify ifREADY_NOT_READshould also display tiered clovers.The tiered clover logic (BOTTOM/MID/TOP) only checks for
READY_READstatus. If a user has an unread reply (READY_NOT_READ) with multiple diaries, the current logic falls through toUNGIVEN_CLOVER(Line 41). Confirm whether unread replies should also display tiered clovers based ondiaryCount.
35-35: LGTM! Deleted diary handling aligns with PR objectives.The logic correctly returns
DISABLED_REPLYwhenisDeleted && diaryCount > 0, implementing the requirement to show a darker gray clover for deleted diaries as stated in the PR description ("일기 삭제 후 재작성 시 클로버 모양을 진한 회색 클로버로 설정").app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt (1)
8-8: LGTM! Import path updated correctly.The import now references the relocated
ReplyStatustype.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt (1)
15-15: LGTM! Import path updated correctly.The import now references the relocated
ReplyStatustype.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt (1)
7-7: LGTM! Import path update aligns with package refactor.The
ReplyStatustype has been relocated fromdomain.modeltodomain.typeacross the codebase. This import update maintains compatibility without affecting logic.app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt (1)
40-40: LGTM! Import path update aligns with package refactor.The
ReplyStatusimport path change is consistent with the project-wide relocation todomain.type.app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt (1)
8-8: LGTM! Import path update aligns with package refactor.Consistent with the project-wide
ReplyStatusrelocation todomain.type.app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt (1)
38-38: LGTM! Import path update aligns with package refactor.The
ReplyStatusimport change is part of the consistent package relocation todomain.type.app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt (1)
3-3: LGTM! Import path update aligns with package refactor.The
ReplyStatusimport change supports the package relocation while preserving all route definitions and default values.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt (2)
31-31: LGTM! Import path update aligns with package refactor.The
ReplyStatusimport change is consistent with the project-wide relocation todomain.type.
50-50: LGTM! Drawable update aligns with PR objectives.The drawable resource change from
ic_home_expired_written_clovertoic_home_disabled_reply_cloverfor theINVALID_DRAFTcase implements the stated PR objective: changing the clover appearance to a darker gray after diary deletion and rewriting.app/src/main/java/com/sopt/clody/domain/model/DailyDiaryInfo.kt (1)
1-13: LGTM! Clean domain model for daily diary data.The
DailyDiaryInfodata class provides a clear structure for representing daily diary information in the home screen. Default values (emptyList()andfalse) ensure safe initialization.app/src/main/java/com/sopt/clody/presentation/di/ViewModelsModule.kt (2)
7-7: LGTM! HomeViewModel import supports new binding.Import added to support the new Hilt binding for
HomeViewModel.Factory.
40-45: LGTM! Hilt binding correctly integrates Mavericks HomeViewModel.The
bindHomeViewModelFactorymethod follows the established pattern for Mavericks-based ViewModels, enabling Hilt to provideHomeViewModelinstances via the assisted injection mechanism.
| class ClodyDeeplinkActivity : AppCompatActivity() { | ||
|
|
||
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Set up proper activity behavior for a deep link handler.
This activity serves as a deep link entry point but doesn't set a content view or configure launch mode. Deep link handler activities typically:
- Don't need a UI (no
setContentView) - Should not remain in the back stack (consider
android:noHistory="true"in manifest) - Should finish immediately after processing the link
Consider adding finish() after handling the deep link to prevent an empty activity from remaining in the back stack:
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
+ // No content view needed for deep link handler
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/ClodyDeeplinkActivity.kt around lines 7–11,
the activity currently does nothing and can remain as an empty entry on the back
stack; update it to act as a no-UI deep link handler by: (1) not calling
setContentView, (2) immediately reading and handling the incoming intent/data in
onCreate (e.g., parse intent.data or extras and route to the correct place), (3)
calling finish() after processing to close the activity, and (4) set
android:noHistory="true" for this activity in AndroidManifest.xml so it is not
kept in the back stack (optionally add a transition override if you want no
animation).
| override fun onResume() { | ||
| super.onResume() | ||
|
|
||
| val isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> | ||
| // when handleDeferredDeeplink is called firstly after install | ||
| if (uri != null) { | ||
| // show proper content using uri (YOUR_SCHEME://...) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Complete the deep link URI handling implementation.
The URI callback is incomplete with only a placeholder comment. This leaves the deep link functionality non-functional.
The current implementation:
- Doesn't handle the URI when received
- Doesn't navigate to any destination
- Doesn't call
finish()to remove the activity from the stack
Apply this diff to add navigation and cleanup:
override fun onResume() {
super.onResume()
val isFirstCalled = Airbridge.handleDeferredDeeplink { uri ->
// when handleDeferredDeeplink is called firstly after install
if (uri != null) {
- // show proper content using uri (YOUR_SCHEME://...)
+ // TODO: Parse uri and navigate to appropriate screen
+ // Example: NavigationUtil.handleDeepLink(this, uri)
}
+ // Finish this activity after handling the deep link
+ finish()
}
+
+ // If not first call, also finish immediately
+ if (!isFirstCalled) {
+ finish()
+ }
}Would you like me to help implement the URI parsing and navigation logic?
📝 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.
| override fun onResume() { | |
| super.onResume() | |
| val isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> | |
| // when handleDeferredDeeplink is called firstly after install | |
| if (uri != null) { | |
| // show proper content using uri (YOUR_SCHEME://...) | |
| } | |
| } | |
| } | |
| override fun onResume() { | |
| super.onResume() | |
| val isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> | |
| // when handleDeferredDeeplink is called firstly after install | |
| if (uri != null) { | |
| // TODO: Parse uri and navigate to appropriate screen | |
| // Example: NavigationUtil.handleDeepLink(this, uri) | |
| } | |
| // Finish this activity after handling the deep link | |
| finish() | |
| } | |
| // If not first call, also finish immediately | |
| if (!isFirstCalled) { | |
| finish() | |
| } | |
| } |
| val isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> | ||
| // when handleDeferredDeeplink is called firstly after install | ||
| if (uri != null) { | ||
| // show proper content using uri (YOUR_SCHEME://...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement URI handling and finish the Activity.
The callback contains only a placeholder comment. Without actual URI processing and navigation logic, this Activity cannot fulfill its deep link handling purpose. Additionally, the Activity does not call finish(), which means it will remain in the back stack as an invisible screen after handling the deep link.
Consider implementing:
- Parse the URI and extract relevant parameters (e.g., screen destination, query params)
- Navigate to the appropriate screen using an Intent (e.g., MainActivity with extras)
- Call
finish()to remove this Activity from the back stack
Example:
Airbridge.handleDeferredDeeplink { uri ->
- // when handleDeferredDeeplink is called firstly after install
if (uri != null) {
- // show proper content using uri (YOUR_SCHEME://...)
+ // Parse URI and navigate to the appropriate screen
+ val intent = Intent(this, MainActivity::class.java).apply {
+ data = uri
+ // Add any necessary extras based on URI
+ }
+ startActivity(intent)
+ } else {
+ // No deferred deep link, navigate to main screen
+ startActivity(Intent(this, MainActivity::class.java))
}
+ finish()
}🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/ClodyDeeplinkActivity.kt around lines 16 to
21, the Airbridge.handleDeferredDeeplink callback only contains a placeholder
and never finishes the Activity; implement URI processing: check for uri !=
null, parse path and query parameters (e.g., destination and any ids), build an
Intent targeting the appropriate Activity (e.g., MainActivity) with the parsed
parameters added as extras, call startActivity(intent) and then finish() to
remove this Activity from the back stack; also handle the null case by either
directing to a default screen or simply calling finish().
| override fun onResume() { | ||
| super.onResume() | ||
|
|
||
| val isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> |
There was a problem hiding this comment.
Unused variable: isFirstCalled is never referenced.
The isFirstCalled boolean indicates whether this is the first deferred deep link call after app install, which could be useful for analytics or conditional logic. Either use this variable or remove it to reduce code noise.
Apply this diff if the variable is not needed:
- val isFirstCalled = Airbridge.handleDeferredDeeplink { uri ->
+ Airbridge.handleDeferredDeeplink { uri ->
// when handleDeferredDeeplink is called firstly after install
if (uri != null) {
// show proper content using uri (YOUR_SCHEME://...)📝 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 isFirstCalled = Airbridge.handleDeferredDeeplink { uri -> | |
| Airbridge.handleDeferredDeeplink { uri -> | |
| // when handleDeferredDeeplink is called firstly after install | |
| if (uri != null) { | |
| // show proper content using uri (YOUR_SCHEME://...) |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/ClodyDeeplinkActivity.kt around line 16, the
local boolean isFirstCalled returned from Airbridge.handleDeferredDeeplink is
assigned but never used; either remove the assignment to eliminate the unused
variable or use isFirstCalled where appropriate (e.g., pass it to analytics or
conditional logic). Update the code by deleting the "val isFirstCalled =" part
if you don't need the flag, or by referencing isFirstCalled in subsequent logic
to record or act on first-call behavior.
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt
Show resolved
Hide resolved
| enum class UiLoadState { | ||
| Idle, | ||
| Loading, | ||
| Success, | ||
| Error, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM! Consider sealed class for more flexibility.
The enum is well-defined and the trailing comma after Error follows Kotlin best practices. However, consider whether a sealed class hierarchy might better serve your MVI architecture if you need to associate data with states (e.g., error details with Error, or loading progress with Loading).
Example sealed class alternative:
sealed interface UiLoadState {
data object Idle : UiLoadState
data object Loading : UiLoadState
data object Success : UiLoadState
data class Error(val message: String? = null, val throwable: Throwable? = null) : UiLoadState
}Additionally, verify that this doesn't duplicate functionality already provided by Mavericks or existing state management utilities in the codebase.
🏁 Script executed:
#!/bin/bash
# Description: Check for similar load state patterns in the codebase
# Search for existing load/loading state enums or sealed classes
rg -n --type kotlin -C3 'sealed (class|interface).*State|enum class.*(Load|Loading|Ui)State'
# Check if Mavericks provides similar functionality
rg -n --type kotlin 'import.*mavericks.*State'Length of output: 13182
Replace UiLoadState enum with existing UiState sealed interface
The generic UiState<T> in presentation/utils/base/UiState.kt already models idle/loading/success. Remove this enum and use UiState<Nothing> for idle/loading, extend UiState with an Error(val message: String?, val throwable: Throwable?) variant instead.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/base/UiLoadState.kt
around lines 3-8, remove this enum and switch callers to use the existing
UiState sealed interface instead; update presentation/utils/base/UiState.kt to
add an Error(val message: String?, val throwable: Throwable?) variant to the
sealed hierarchy, and use UiState<Nothing> for Idle and Loading states (and
UiState<T> for Success with data); then replace all imports/usages of
UiLoadState in the codebase to the corresponding UiState<Nothing>.Idle,
UiState<Nothing>.Loading, UiState.Success(data) and UiState.Error(message,
throwable) variants and remove the deleted file from the project.
- 에어브릿지 scheme 설정 수정 - 에러스크린 메시지 초기화 추가
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt (1)
33-33: Import issue from previous review has been resolved.The file now correctly imports
java.time.DayOfWeek, which provides theplus()method (line 47) andgetDisplayName()method (line 77) required by this composable. This addresses the compilation issue flagged in the previous review.
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt (1)
77-77: Consider safer locale access.Direct indexing with
locales[0]could theoretically throwIndexOutOfBoundsExceptionif the locales list is empty, though this is unlikely in practice on Android.Apply this diff for defensive programming:
- text = dayOfWeek.getDisplayName(TextStyle.NARROW, LocalConfiguration.current.locales[0]), + text = dayOfWeek.getDisplayName(TextStyle.NARROW, LocalConfiguration.current.locales.getOrElse(0) { java.util.Locale.getDefault() }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/AndroidManifest.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (1)
sendNotification(36-72)app/src/main/java/com/sopt/clody/data/repositoryimpl/NotificationRepositoryImpl.kt (1)
sendNotification(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/MonthlyCalendar.kt (2)
56-56: Efficient lookup optimization.Using
associateByto create a date-to-info map is a good optimization that reduces the lookup complexity from O(n) to O(1) for each day cell.
108-120: Clarify blank-cell behavior for dates without diary entries
calendarDailyInfoListis built fromdata.diariesonly, so dates with no diary entry render as blank cells.- For the PR’s “gray→dark gray” clover logic, confirm whether:
• the backend will provide an entry for every date (including empty ones), or
• this component should render a default gray clover for missing dates.
| is HomeContract.HomeIntent.ConfirmYearMonthPicker -> loadCalendarMonthlyInfo(intent.newYear, intent.newMonth) | ||
| is HomeContract.HomeIntent.DismissYearMonthPicker -> setState { copy(showYearMonthPicker = false) } |
There was a problem hiding this comment.
Close the year/month picker after confirm.
When ConfirmYearMonthPicker fires we still leave showYearMonthPicker true, so the picker never dismisses unless the user manually cancels. Reset the flag before reloading the calendar.
- is HomeContract.HomeIntent.ConfirmYearMonthPicker -> loadCalendarMonthlyInfo(intent.newYear, intent.newMonth)
+ is HomeContract.HomeIntent.ConfirmYearMonthPicker -> {
+ setState { copy(showYearMonthPicker = false) }
+ loadCalendarMonthlyInfo(intent.newYear, intent.newMonth)
+ }📝 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.
| is HomeContract.HomeIntent.ConfirmYearMonthPicker -> loadCalendarMonthlyInfo(intent.newYear, intent.newMonth) | |
| is HomeContract.HomeIntent.DismissYearMonthPicker -> setState { copy(showYearMonthPicker = false) } | |
| is HomeContract.HomeIntent.ConfirmYearMonthPicker -> { | |
| setState { copy(showYearMonthPicker = false) } | |
| loadCalendarMonthlyInfo(intent.newYear, intent.newMonth) | |
| } | |
| is HomeContract.HomeIntent.DismissYearMonthPicker -> setState { copy(showYearMonthPicker = false) } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt
around lines 66 to 67, the ConfirmYearMonthPicker intent handler doesn't close
the year/month picker before reloading the calendar; update the handler to first
setState { copy(showYearMonthPicker = false) } to dismiss the picker, then call
loadCalendarMonthlyInfo(intent.newYear, intent.newMonth) so the UI is updated
and the picker is closed before the reload.
📌 ISSUE
closed #323
📄 Work Description
✨ PR Point
정말.. 울고싶었던 순간이 한두번이 아닌데요.
1. Mavericks 써보자고 승낙한 제 자신이 미웠습니다.
기존에 제가 사용하던 MVI 뷰모델과는 차이가 크더군요.. 아직 제가 Mavericks를 잘 몰라서 그런지 모르겠지만, withState를 사용하려다가 coroutine, suspend fun 장벽에 가로막혀 뷰모델에서 state를 참조하여 쓰고 싶었던 로직들도 전부 Screen에서 state를 참조하여 파라미터로 넘겨서 사용하는 수 밖에 없었습니다. CS 공부를 해야겠다고 느꼈고(특히 코루틴...) 뷰모델을 왜 이렇게 만들었을까 하는 궁금증이 크게 들었습니다. 그래서 그런지 아마 보시면 MVI 형태를 했지만 어딘가 좀 이상한 무언가가 되지 않았을까 싶어요.. 확인 부탁드립니다.
2. 다른 뷰들 마이그레이션이 두려워졌습니다. 하지만 기뻤습니다.
홈화면이 기능이 많긴했지만 다른 화면들도 마이그레이션 하려니 가슴이 턱턱 막히는 기분이더군요.. 하지만 홈화면 마이그레이션 하면서 컴포넌트도 좀 줄이고 계층 구조도 간소화해서 개인적으로는 코드가 상당히 가독성이 좋아졌다고 생각합니다. 컴포넌트 개선과 마이그레이션.. 참 독이 든 사과 같아요
3. 에러대응을 아직 못했습니다.
MVI 마이그레이션 하면서 기존 에러대응 로직을 좀 많이 깎아냈는데요, 에러대응(특히 스크린 대응)을 하려다가도 뭔가 Route에서? Screen에서? 위치를 어디로 잡는게 좋을지 뭔가 같이 보고 정하고 싶어서 일단 에러대응을 배제하고 작업했습니다.. 이 부분은 좀 코드리뷰 부탁드려요
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores