[REFACTOR/#280] 4차 스프린트 QA를 반영합니다.#281
Conversation
…te 및 isFromDraft 필드 추가
|
""" WalkthroughThis update introduces new properties to several data classes, enhances UI components to handle draft and invalid draft diary states, and updates ViewModel and screen logic to track and reflect draft validity. Button components now support customized colors and disabled states based on diary reply status. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant HomeViewModel
participant DiaryStateButton
participant WriteDiaryViewModel
User->>HomeScreen: Open Home Screen
HomeScreen->>HomeViewModel: Collect hasDraft, isInvalidDraft
HomeScreen->>DiaryStateButton: Pass hasDraft, canReply, isInvalidDraft, etc.
DiaryStateButton->>User: Render button (disabled if isInvalidDraft)
User->>WriteDiaryViewModel: Attempt to write diary
WriteDiaryViewModel->>WriteDiaryViewModel: Check isDiaryExpired
alt Diary expired
WriteDiaryViewModel->>HomeScreen: Set state to NoReply
else Diary not expired
WriteDiaryViewModel->>HomeScreen: Set state based on replyType
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/DiaryStateButton.kt (1)
28-48:⚠️ Potential issue
isInvalidDraftbranch is unreachable – reorder thewhenclauses
hasDraftistrueeven for an invalid draft, so the first branch short-circuits thewhen, causing"이어쓰기"to be shown instead of the intended disabled"답장확인".-when { - hasDraft -> { … } - isInvalidDraft -> { … } +when { + isInvalidDraft -> { // 1️⃣ highest priority + … + } + hasDraft -> { // 2️⃣ + … + }Without this change, users with an expired draft will be able to tap Continue Writing, contradicting business rules.
🧹 Nitpick comments (5)
app/src/main/java/com/sopt/clody/data/remote/dto/response/WriteDiaryResponseDto.kt (1)
9-10: Annotate the new field or document expected semantics
isFromDraftis added without a@SerialNameannotation or KDoc.
If the backend field name differs (e.g.is_from_draft) deserialization will silently fail.
Add an explicit annotation or short comment to avoid future regressions and improve readability.app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (1)
149-153: Nice: explicit variable name improves clarityRenaming the lambda parameter to
dailyResponsemakes the success block self-describing.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt (1)
127-151: Consolidate disabled-state styling to reduce duplicationThe button builds custom
containerColor/contentColoryet still passesdisabled*Colorvalues.
You can delegate everything toButtonDefaults.buttonColorsby supplying onlydisabled*Color, avoiding manualifblocks:- val isDisabled = dailyDiary.isDeleted || dailyDiary.replyStatus == ReplyStatus.INVALID_DRAFT - val containerColor = if (dailyDiary.replyStatus == ReplyStatus.INVALID_DRAFT) { - ClodyTheme.colors.gray08 - } else { - ClodyTheme.colors.lightBlue - } - val contentColor = if (dailyDiary.replyStatus == ReplyStatus.INVALID_DRAFT) { - ClodyTheme.colors.gray06 - } else { - ClodyTheme.colors.blue - } + val isDisabled = dailyDiary.isDeleted || dailyDiary.replyStatus == ReplyStatus.INVALID_DRAFTcolors = ButtonDefaults.buttonColors( containerColor = ClodyTheme.colors.lightBlue, contentColor = ClodyTheme.colors.blue, disabledContainerColor = ClodyTheme.colors.gray08, disabledContentColor = ClodyTheme.colors.gray06, )Cleaner and avoids divergent colours if the theme changes.
app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt (1)
21-24: LeverageLocalContentColorinstead of manual text-color switching
ButtonDefaults.buttonColors()already puts thecontentColor/disabledContentColorinLocalContentColor.
Manually overriding theTextcolor duplicates that logic and makes it easy for the two sources to drift.-Text( - text = text, - color = if (enabled) contentColor else disabledContentColor, - style = ClodyTheme.typography.body2SemiBold, -) +Text( + text = text, + style = ClodyTheme.typography.body2SemiBold, +)This keeps a single source of truth and lets downstream themes override content colours through
ButtonDefaults.
No functional change, but eliminates an unnecessary branch.Also applies to: 40-41
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
291-296: Misleading parameter name —replyStatusis actually aRoute.ReplyLoading.ReplyLoadingFromonClickReplyDiary: ( … replyStatus: Route.ReplyLoading.ReplyLoadingFrom ) -> Unit
replyStatussuggests a domain enum but the type is the origin enum.
Rename tofrom(or similar) to avoid confusion when the realReplyStatusis also in scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/WriteDiaryResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/component/DiaryStateButton.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(7 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/component/DiaryStateButton.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt (1)
ClodyButton(15-44)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyCalendarResponseDto.kt (1)
17-18: Verify date–format alignment across DTOs
date: Stringis introduced but its expected format (ISO-8601,yyyy-MM-dd, etc.) is not stated.
Down-stream parsing (e.g. inDiaryDeleteButton,isDiaryExpired) will break if the server changes the pattern.Document or enforce the format (e.g. with
LocalDate+ custom serializer).app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (1)
96-98: StateFlow for draft presence looks goodNew
_hasDraft/hasDraftflow is correctly initialised and exposed as read-only.
No issues spotted.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt (1)
50-56: Icon mapping covers INVALID_DRAFT stateThe extra case keeps UI feedback consistent. 👍
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
126-130: Logic duplication withDiaryStateButton
hasDraft && !isValidDraftDate()opens the “continue draft” dialog, while the bottom bar still shows the disabled “답장확인” button (after the previous bug is fixed).
Double-check that these two UX paths are consistent; otherwise users may see contradictory actions.
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt
Show resolved
Hide resolved
| LaunchedEffect(Unit) { | ||
| val year = selectedDiaryDate.year | ||
| val month = selectedDiaryDate.month | ||
| val day = selectedDate.dayOfMonth | ||
|
|
||
| if (selectedDate.dayOfMonth != 0) { | ||
| homeViewModel.updateSelectedDate( | ||
| LocalDate.of(selectedDiaryDate.year, selectedDiaryDate.month, selectedDate.dayOfMonth), | ||
| ) | ||
| } else { | ||
| homeViewModel.updateSelectedDate(LocalDate.now()) | ||
| } | ||
| homeViewModel.loadCalendarData(year, month) | ||
| delay(500) | ||
| homeViewModel.loadDailyDiariesData(year, month, day) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Two LaunchedEffect(Unit) blocks fire concurrently – consider merging
Both blocks use the same key (Unit) and run on first composition, starting two coroutines that hit the network:
- in-app review & analytics
- calendar + diaries loading
The 500 ms delay is a brittle coupling between the two loads and may break on slow devices or change of dispatcher.
Combine them or chain the repository calls inside a single suspend function to guarantee ordering without magic delays.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt
around lines 92 to 100, two LaunchedEffect(Unit) blocks run concurrently causing
network calls to overlap and rely on a fragile 500ms delay for sequencing. To
fix this, merge these two LaunchedEffect blocks into one and chain the
repository calls sequentially within a single suspend function or coroutine,
ensuring the first completes before the second starts without using arbitrary
delays.
- 추후 flavor이나 build variant로 debug release 분리 할 것
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/sopt/clody/data/remote/dto/response/DailyDiariesResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(7 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt
[warning] 106-106: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
84-91: 👍 DuplicateLaunchedEffectblocks merged – removes race & magic delayThe new single block eliminates the fragile 500 ms delay noted in the previous review and makes the sequence explicit. Nice clean-up.
| @SerialName("diaries") val diaries: List<Diary>, | ||
| @SerialName("isDeleted") val isDeleted: Boolean, | ||
| @SerialName("isDraft") val isDraft: Boolean, | ||
| ) { |
There was a problem hiding this comment.
isDeleted should be nullable or have a default value to avoid MissingFieldException.
Adding a non-nullable field to a DTO breaks backward compatibility with older back-end payloads that don’t yet include the field. kotlinx.serialization will throw at runtime if "isDeleted" is absent.
- @SerialName("isDeleted") val isDeleted: Boolean,
+ @SerialName("isDeleted") val isDeleted: Boolean = false,Either supply a sensible default (false) or make it Boolean? and handle null downstream.
📝 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.
| @SerialName("diaries") val diaries: List<Diary>, | |
| @SerialName("isDeleted") val isDeleted: Boolean, | |
| @SerialName("isDraft") val isDraft: Boolean, | |
| ) { | |
| @SerialName("diaries") val diaries: List<Diary>, | |
| - @SerialName("isDeleted") val isDeleted: Boolean, | |
| + @SerialName("isDeleted") val isDeleted: Boolean = false, | |
| @SerialName("isDraft") val isDraft: Boolean, | |
| ) { |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/dto/response/DailyDiariesResponseDto.kt
around lines 8 to 11, the non-nullable Boolean field isDeleted can cause a
MissingFieldException if the backend payload does not include it. To fix this,
either make isDeleted nullable by changing its type to Boolean? or provide a
default value such as false to ensure backward compatibility and prevent runtime
exceptions.
| LaunchedEffect(Unit) { | ||
| val year = selectedDiaryDate.year | ||
| val month = selectedDiaryDate.month | ||
| val day = selectedDate.dayOfMonth | ||
|
|
||
| if (selectedDate.dayOfMonth != 0) { | ||
| homeViewModel.updateSelectedDate( | ||
| LocalDate.of(selectedDiaryDate.year, selectedDiaryDate.month, selectedDate.dayOfMonth), | ||
| ) | ||
| } else { | ||
| homeViewModel.updateSelectedDate(LocalDate.now()) | ||
| try { | ||
| coroutineScope { | ||
| val calendarDeferred = async { homeViewModel.loadCalendarData(year, month) } | ||
| val dailyDeferred = async { homeViewModel.loadDailyDiariesData(year, month, day) } | ||
|
|
||
| calendarDeferred.await() | ||
| dailyDeferred.await() | ||
| } | ||
| } catch (e: Exception) { | ||
| homeViewModel.setErrorState(true, "데이터를 불러오는데 실패했습니다.") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch-all Exception is swallowed; use supervisorScope + logging to preserve stack-trace.
coroutineScope { async … }cancels both coroutines when one fails. If you want both results but tolerate failure in either, wrap withsupervisorScopeinstead.- The generic
catch (e: Exception)silently drops the throwable after setting UI state, making debugging impossible and triggering detekt’sSwallowedExceptionwarning.
- try {
- coroutineScope {
- val calendarDeferred = async { homeViewModel.loadCalendarData(year, month) }
- val dailyDeferred = async { homeViewModel.loadDailyDiariesData(year, month, day) }
-
- calendarDeferred.await()
- dailyDeferred.await()
- }
- } catch (e: Exception) {
- homeViewModel.setErrorState(true, "데이터를 불러오는데 실패했습니다.")
- }
+ try {
+ supervisorScope {
+ val calendar = async { homeViewModel.loadCalendarData(year, month) }
+ val daily = async { homeViewModel.loadDailyDiariesData(year, month, day) }
+ calendar.await()
+ daily.await()
+ }
+ } catch (e: Exception) {
+ Timber.e(e, "HomeScreen data load failed") // or Log.e(...)
+ homeViewModel.setErrorState(true, "데이터를 불러오는데 실패했습니다.")
+ }This keeps the stack-trace, satisfies static analysis, and ensures one failed call doesn’t cancel the other unnecessarily.
📝 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.
| LaunchedEffect(Unit) { | |
| val year = selectedDiaryDate.year | |
| val month = selectedDiaryDate.month | |
| val day = selectedDate.dayOfMonth | |
| if (selectedDate.dayOfMonth != 0) { | |
| homeViewModel.updateSelectedDate( | |
| LocalDate.of(selectedDiaryDate.year, selectedDiaryDate.month, selectedDate.dayOfMonth), | |
| ) | |
| } else { | |
| homeViewModel.updateSelectedDate(LocalDate.now()) | |
| try { | |
| coroutineScope { | |
| val calendarDeferred = async { homeViewModel.loadCalendarData(year, month) } | |
| val dailyDeferred = async { homeViewModel.loadDailyDiariesData(year, month, day) } | |
| calendarDeferred.await() | |
| dailyDeferred.await() | |
| } | |
| } catch (e: Exception) { | |
| homeViewModel.setErrorState(true, "데이터를 불러오는데 실패했습니다.") | |
| } | |
| } | |
| LaunchedEffect(Unit) { | |
| val year = selectedDiaryDate.year | |
| val month = selectedDiaryDate.month | |
| val day = selectedDate.dayOfMonth | |
| try { | |
| supervisorScope { | |
| val calendar = async { homeViewModel.loadCalendarData(year, month) } | |
| val daily = async { homeViewModel.loadDailyDiariesData(year, month, day) } | |
| calendar.await() | |
| daily.await() | |
| } | |
| } catch (e: Exception) { | |
| Timber.e(e, "HomeScreen data load failed") // or Log.e(...) | |
| homeViewModel.setErrorState(true, "데이터를 불러오는데 실패했습니다.") | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 106-106: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt
around lines 93 to 109, replace the coroutineScope with supervisorScope to
prevent cancellation of both coroutines if one fails, and add proper logging
inside the catch block to preserve the stack trace. This involves changing
coroutineScope to supervisorScope and logging the caught exception before
setting the error state, ensuring failures in one async call do not cancel the
other and that exceptions are not silently swallowed.
2 participants
📌 ISSUE
closed #280
📄 Work Description
✨ PR Point
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Bug Fixes
Improvements