Conversation
WalkthroughThis update integrates the draft diary (임시저장) API into the application. It introduces new DTOs, repository methods, use cases, and ViewModel logic for fetching and saving draft diaries. The UI is updated to handle draft diary state and user interactions, while new tests and test doubles are added to verify the draft diary functionality. Build and test configurations are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant UI (WriteDiaryScreen)
participant VM (WriteDiaryViewModel)
participant UseCaseFetch
participant UseCaseSave
participant Repo
participant RemoteDS
participant API
UI->>VM: On screen start, fetchDraftDiary(year, month, day)
VM->>UseCaseFetch: invoke(year, month, day)
UseCaseFetch->>Repo: fetchDraftDiary(year, month, day)
Repo->>RemoteDS: fetchDraftDiary(year, month, day)
RemoteDS->>API: GET /draft-diary
API-->>RemoteDS: DraftDiariesResponseDto
RemoteDS-->>Repo: ApiResponse<DraftDiariesResponseDto>
Repo-->>UseCaseFetch: Result<DraftDiaryContents>
UseCaseFetch-->>VM: Result<DraftDiaryContents>
VM-->>UI: Update entries with fetched drafts
UI->>VM: On exit/dismiss, saveDraftDiary(year, month, day)
VM->>UseCaseSave: invoke(date, contents)
UseCaseSave->>Repo: saveDraftDiary(date, contents)
Repo->>RemoteDS: saveDraftDiary(request)
RemoteDS->>API: POST /draft-diary
API-->>RemoteDS: ApiResponse<Unit>
RemoteDS-->>Repo: ApiResponse<Unit>
Repo-->>UseCaseSave: Result<Unit>
UseCaseSave-->>VM: Result<Unit>
VM-->>UI: Handle save result (success/failure)
Assessment against linked issues
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 detekt (1.23.8)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt[warning] 209-209: String.format("%04d-%02d-%02d", year, month, day) uses implicitly default locale for string formatting. (detekt.potential-bugs.ImplicitDefaultLocale) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ 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: 3
🧹 Nitpick comments (6)
app/src/main/java/com/sopt/clody/domain/model/CreatedDraftDiaryInfo.kt (1)
3-5: Consider using a more type-safe timestamp representation.While using
StringforcreatedAtworks functionally, it lacks type safety and validation. Consider usingLocalDateTime,Instant, or a custom timestamp type to prevent parsing errors and ensure consistent formatting.+import java.time.LocalDateTime + data class CreatedDraftDiaryInfo( - val createdAt: String, + val createdAt: LocalDateTime, )app/src/test/java/com/sopt/clody/repositoryimpl/DiaryRepositoryImplTest.kt (2)
26-27: Remove debugging println statements.These debugging statements should be removed before merging to production.
- println("fetchDraftDiary result: $result") - println("fetchDraftDiary error: ${result.exceptionOrNull()?.message}")
51-52: Remove debugging println statements.These debugging statements should be removed before merging to production.
- println("saveDraftDiary result: $result") - println("saveDraftDiary error: ${result.exceptionOrNull()?.message}")app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (1)
202-214: Remove debug println statement.The implementation is solid, but the println statement should be removed for production code.
Apply this diff to remove the debug statement:
result.onSuccess { _failureMessage.value = "" _showFailureDialog.value = false - println("성공함") }.onFailure { e ->app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt (2)
48-63: Consider more descriptive error messages.The implementation logic is correct, but the error messages could be more helpful for debugging test failures.
- ?: throw IllegalStateException("draftDiariesResponse not set") + ?: throw IllegalStateException("draftDiariesResponse not set. Call setDraftDiariesResponse() before testing fetchDraftDiary.")- ?: throw IllegalStateException("saveDraftResponse not set") + ?: throw IllegalStateException("saveDraftResponse not set. Call setSaveDraftDiaryResponse() before testing saveDraftDiary.")
64-79: Consider consistency for hardcoded messages.The helper methods work well for test setup, but the hardcoded Korean message "성공" might be inconsistent with other parts of the codebase.
Consider extracting the success message to a constant or making it configurable:
+ companion object { + private const val SUCCESS_MESSAGE = "성공" + } + fun setDraftDiariesResponse(list: List<String>) { draftDiariesResponse = ApiResponse( status = 200, - message = "성공", + message = SUCCESS_MESSAGE, data = DraftDiariesResponseDto(draftDiaries = list), ) } fun setSaveDraftDiaryResponse(createdAt: String) { saveDraftResponse = ApiResponse( status = 201, - message = "성공", + message = SUCCESS_MESSAGE, data = DraftDiaryCreatedResponseDto(createdAt), ) }Alternatively, you could reference a shared constant if one exists elsewhere in the codebase for API success messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
app/build.gradle.kts(2 hunks)app/src/main/java/com/sopt/clody/data/remote/api/DiaryService.kt(2 hunks)app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt(2 hunks)app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt(2 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/request/SaveDraftDiaryRequestDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/DraftDiariesResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/DraftDiaryCreatedResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt(2 hunks)app/src/main/java/com/sopt/clody/domain/model/CreatedDraftDiaryInfo.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/model/DraftDiaryContents.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt(2 hunks)app/src/main/java/com/sopt/clody/domain/usecase/FetchDraftDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/utils/extension/LifeCycle.kt(2 hunks)app/src/test/java/com/sopt/clody/FakeDiaryRepository.kt(1 hunks)app/src/test/java/com/sopt/clody/WriteDiaryViewModelTest.kt(1 hunks)app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt(1 hunks)app/src/test/java/com/sopt/clody/repositoryimpl/DiaryRepositoryImplTest.kt(1 hunks)gradle/libs.versions.toml(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/extension/LifeCycle.kt (1)
LaunchedEffectWhenStarted(42-55)
app/src/test/java/com/sopt/clody/WriteDiaryViewModelTest.kt (1)
app/src/test/java/com/sopt/clody/repositoryimpl/DiaryRepositoryImplTest.kt (1)
Given(8-59)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (33)
app/src/main/java/com/sopt/clody/presentation/utils/extension/LifeCycle.kt (2)
3-4: LGTM: Clean import additions for Compose integration.The new imports are properly organized and necessary for the Composable function implementation.
Also applies to: 7-7
32-55: Excellent lifecycle-aware Composable utility function.The implementation follows Compose best practices:
- Proper use of
LocalLifecycleOwner.current- Correct
LaunchedEffectpattern with lifecycle dependency- Flexible parameters with sensible defaults
- Comprehensive Korean documentation
- Clean separation of concerns
This utility will be valuable for lifecycle-aware side effects throughout the app.
gradle/libs.versions.toml (2)
126-131: Well-structured test dependency definitions.The library entries follow the existing conventions and properly reference the version variables.
204-208: Clean bundle organization for Kotest modules.The bundle groups the related Kotest modules logically, following the existing pattern in the file.
app/build.gradle.kts (2)
80-84: Correct JUnit Platform configuration for Kotest.The
useJUnitPlatform()configuration is required for Kotest tests to run properly. Well implemented.
131-134: Proper test dependency additions.The test dependencies are correctly placed and reference the appropriate library aliases from the version catalog.
app/src/main/java/com/sopt/clody/domain/model/DraftDiaryContents.kt (1)
1-5: Clean and well-designed domain model.The data class follows Kotlin conventions with:
- Proper package structure
- Descriptive property naming
- Appropriate type for the use case
- Simple, focused design
Perfect for representing draft diary contents in the domain layer.
app/src/main/java/com/sopt/clody/data/remote/dto/request/SaveDraftDiaryRequestDto.kt (1)
6-9: LGTM! Clean DTO implementation.The DTO follows good practices with proper serialization annotations and clear field mapping. The structure is appropriate for handling draft diary requests.
app/src/main/java/com/sopt/clody/domain/usecase/FetchDraftDiaryUseCase.kt (1)
7-13: LGTM! Clean use case implementation following clean architecture.The use case correctly implements the single responsibility principle with proper dependency injection and error handling through Result type. The delegation pattern to the repository is appropriate.
app/src/main/java/com/sopt/clody/data/remote/dto/response/DraftDiaryCreatedResponseDto.kt (1)
8-16: LGTM: Clean DTO implementation following established patterns.The DTO correctly handles the API response structure with proper serialization annotations and domain conversion. The
createdAtfield as String is acceptable if it matches the API contract, though consider using a more specific timestamp type if the API allows.app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt (1)
7-13: LGTM: Use case follows clean architecture principles.The implementation correctly encapsulates the repository call with proper dependency injection and error handling via Result type. While it's a thin abstraction layer, it maintains architectural consistency and provides a clean interface for the presentation layer.
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (4)
51-51: Good import addition for lifecycle-aware functionality.The import of
LaunchedEffectWhenStartedsupports the lifecycle-aware draft fetching implementation.
78-80: Excellent use of lifecycle-aware effect for draft fetching.The
LaunchedEffectWhenStartedutility correctly ensures that draft diary fetching occurs when the screen starts, following proper lifecycle management. This will automatically fetch any existing draft when the user enters the write diary screen.
103-103: Smart change detection for improved UX.Using
hasChangedFromInitial()to determine whether to show the exit dialog is a good UX improvement. Users will only be prompted about unsaved changes when they actually have changes to save.
146-150: Excellent UX: Auto-save on exit dialog dismiss.The implementation to save the draft diary when dismissing the exit dialog provides excellent user experience. Users can safely dismiss the dialog knowing their work will be preserved, while still having the option to discard changes via the confirm action.
app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt (2)
9-11: Good domain model imports for draft functionality.The imports correctly bring in the necessary domain models for the new draft diary methods.
20-21: Well-designed repository interface extensions.The new methods follow established patterns with appropriate parameter types and Result return types for consistent error handling. The
fetchDraftDiarymethod correctly takes date parameters, whilesaveDraftDiaryaccepts content for persistence.app/src/test/java/com/sopt/clody/repositoryimpl/DiaryRepositoryImplTest.kt (1)
8-59: Well-structured BDD tests with good coverage.The test structure follows good BDD practices with clear Given/When/Then scenarios. The use of
FakeDiaryRemoteDataSourcefor dependency injection is appropriate for unit testing, and the assertions properly validate both success states and data integrity.app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt (1)
4-4: Clean interface extensions following existing patterns.The new method signatures are well-designed and consistent with the existing interface conventions. The parameter naming and return types appropriately match the draft diary functionality requirements.
Also applies to: 7-8, 22-23
app/src/main/java/com/sopt/clody/data/remote/api/DiaryService.kt (1)
4-4: Well-designed API endpoints following REST conventions.The new endpoints correctly use appropriate HTTP methods (GET for fetching, POST for saving) and follow the existing parameter naming conventions. The Retrofit annotations are properly applied.
Also applies to: 8-9, 66-76
app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt (1)
6-6: Appropriate data source implementations maintaining clean architecture.The implementations correctly delegate to the service layer without adding business logic, maintaining proper separation of concerns. The parameter passing follows the established patterns in the class.
Also applies to: 10-11, 42-46
app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt (2)
87-94: LGTM! Consistent implementation pattern.The
fetchDraftDiarymethod follows the established pattern used by other methods in this repository implementation, properly handling exceptions and API responses.
96-103: LGTM! Proper request DTO usage and error handling.The
saveDraftDiarymethod correctly wraps the contents in aSaveDraftDiaryRequestDtoand follows the same error handling pattern as other methods.app/src/test/java/com/sopt/clody/FakeDiaryRepository.kt (1)
18-44: LGTM! Appropriate stub implementation for focused testing.The NotImplementedError approach for unused methods is appropriate for a focused fake repository that only needs to support draft diary operations.
app/src/test/java/com/sopt/clody/WriteDiaryViewModelTest.kt (3)
20-32: LGTM! Proper coroutine test setup.The test dispatcher setup and teardown correctly handles the main dispatcher for coroutine testing.
34-56: Test depends on FakeDiaryRepository fix.This test sets
draftDiaryResultbut currently theFakeDiaryRepository.fetchDraftDiarymethod doesn't use this property. The test will work after the FakeDiaryRepository is fixed to use the result properties.
59-84: LGTM! Comprehensive save operation test.The test properly sets up the ViewModel state, performs operations, and verifies the expected outcome using async verification.
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (4)
12-13: LGTM! Proper dependency injection of use cases.The new use case dependencies are correctly injected and follow the established pattern in the ViewModel.
Also applies to: 26-27
64-64: LGTM! Good approach for change tracking.The
initialEntriesproperty provides a clean way to track the original state for change detection.
178-180: LGTM! Simple and effective change detection.The comparison logic correctly identifies when the current entries differ from the initial state.
182-200: LGTM! Comprehensive fetch implementation with proper state management.The method properly clears existing state, populates new data, updates initial entries for change tracking, and handles both success and failure cases appropriately.
app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt (2)
15-19: Well-structured test double implementation.The fake data source follows good testing practices by focusing on specific functionality (draft diaries) and using a fail-fast approach with stored responses.
20-47: Appropriate approach for focused test double.Using
NotImplementedErrorfor unused methods clearly indicates the scope of this fake implementation and helps catch any unintended usage during testing.
| kotestVersion = "5.9.0" | ||
| mockk = "1.13.10" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify dependency versions for latest stable releases.
The dependency versions should be checked to ensure you're using the latest stable releases for optimal security and features.
🌐 Web query:
What are the latest stable versions of Kotest and MockK for Android development?
💡 Result:
As of June 4, 2025, the latest stable versions of Kotest and MockK for Android development are:
-
Kotest: Version 5.7.0, released on May 15, 2025.
-
MockK: Version 1.13.16, released on January 10, 2025.
These versions are compatible with Android development and can be integrated into your project using the following dependencies:
For Kotest:
testImplementation 'io.kotest:kotest-runner-junit5:5.7.0'
testImplementation 'io.kotest:kotest-assertions-core:5.7.0'For MockK:
testImplementation 'io.mockk:mockk:1.13.16'Ensure that your project is configured to use JUnit 5, as Kotest integrates with it for running tests.
For more details on integrating these libraries into your Android project, refer to their official documentation:
Citations:
Update Kotest and MockK to latest stable releases
The PR bumps Kotest to 5.9.0 and MockK to 1.13.10, but the latest stable versions (as of June 4, 2025) are:
- Kotest 5.7.0
- MockK 1.13.16
Please update gradle/libs.versions.toml accordingly:
kotestVersion = "5.9.0"
-mockk = "1.13.10"
+kotestVersion = "5.7.0"
+mockk = "1.13.16"And update any Kotest module entries or bundles to use 5.7.0 instead of 5.9.0.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In gradle/libs.versions.toml at lines 51 to 52, the Kotest and MockK versions
are set to 5.9.0 and 1.13.10 respectively, but the latest stable versions are
Kotest 5.7.0 and MockK 1.13.16. Update the kotestVersion to "5.7.0" and mockk to
"1.13.16". Also, ensure any other Kotest module entries or bundles in the file
use version 5.7.0 to maintain consistency.
| data class DraftDiariesResponseDto( | ||
| @SerialName("draftDiaries") val draftDiaries: List<String>, | ||
| ) { | ||
| fun toDomain() = DraftDiaryContents( | ||
| draftDiaries = draftDiaries ?: emptyList(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix inconsistency between property nullability and null check.
There's a logical inconsistency: draftDiaries is declared as non-null List<String> on line 9, but line 12 performs a null check draftDiaries ?: emptyList(). This null check is unnecessary since the property cannot be null.
Option 1: If the API can return null, make the property nullable:
-@SerialName("draftDiaries") val draftDiaries: List<String>,
+@SerialName("draftDiaries") val draftDiaries: List<String>?,Option 2: If the API always returns a list, remove the null check:
fun toDomain() = DraftDiaryContents(
- draftDiaries = draftDiaries ?: emptyList(),
+ draftDiaries = draftDiaries,
)Please verify the API contract to determine which approach is correct.
📝 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.
| data class DraftDiariesResponseDto( | |
| @SerialName("draftDiaries") val draftDiaries: List<String>, | |
| ) { | |
| fun toDomain() = DraftDiaryContents( | |
| draftDiaries = draftDiaries ?: emptyList(), | |
| ) | |
| } | |
| data class DraftDiariesResponseDto( | |
| @SerialName("draftDiaries") val draftDiaries: List<String>?, // now nullable | |
| ) { | |
| fun toDomain() = DraftDiaryContents( | |
| draftDiaries = draftDiaries ?: emptyList(), // safe fallback | |
| ) | |
| } |
| data class DraftDiariesResponseDto( | |
| @SerialName("draftDiaries") val draftDiaries: List<String>, | |
| ) { | |
| fun toDomain() = DraftDiaryContents( | |
| draftDiaries = draftDiaries ?: emptyList(), | |
| ) | |
| } | |
| data class DraftDiariesResponseDto( | |
| @SerialName("draftDiaries") val draftDiaries: List<String>, | |
| ) { | |
| fun toDomain() = DraftDiaryContents( | |
| draftDiaries = draftDiaries, // no need for `?:` | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/dto/response/DraftDiariesResponseDto.kt
between lines 8 and 14, the property draftDiaries is declared as a non-null
List<String> but the toDomain function performs a redundant null check on it. To
fix this, either make draftDiaries nullable if the API can return null, or if
the API guarantees a non-null list, remove the null check in toDomain and
directly use draftDiaries. Confirm the API contract to decide which fix to
apply.
| var draftDiaryResult: Result<DraftDiaryContents>? = null | ||
| var saveDraftResult: Result<CreatedDraftDiaryInfo>? = null | ||
| private var draftDiaryContents: DraftDiaryContents? = null |
There was a problem hiding this comment.
Fix inconsistent property usage in test fake.
The draftDiaryResult property is declared but never used in the fetchDraftDiary implementation. This creates confusion as tests may set draftDiaryResult expecting it to control the behavior.
Apply this diff to fix the inconsistency:
override suspend fun fetchDraftDiary(year: Int, month: Int, date: Int): Result<DraftDiaryContents> {
- return draftDiaryContents?.let { Result.success(it) }
- ?: Result.failure(IllegalStateException("No draft found"))
+ return draftDiaryResult ?: Result.failure(IllegalStateException("No draft found"))
}
override suspend fun saveDraftDiary(contents: List<String>): Result<CreatedDraftDiaryInfo> {
draftDiaryContents = DraftDiaryContents(contents)
- return Result.success(CreatedDraftDiaryInfo(createdAt = "2024-06-01T00:00:00"))
+ return saveDraftResult ?: Result.success(CreatedDraftDiaryInfo(createdAt = "2024-06-01T00:00:00"))
}📝 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.
| var draftDiaryResult: Result<DraftDiaryContents>? = null | |
| var saveDraftResult: Result<CreatedDraftDiaryInfo>? = null | |
| private var draftDiaryContents: DraftDiaryContents? = null | |
| override suspend fun fetchDraftDiary(year: Int, month: Int, date: Int): Result<DraftDiaryContents> { | |
| return draftDiaryResult ?: Result.failure(IllegalStateException("No draft found")) | |
| } | |
| override suspend fun saveDraftDiary(contents: List<String>): Result<CreatedDraftDiaryInfo> { | |
| draftDiaryContents = DraftDiaryContents(contents) | |
| return saveDraftResult ?: Result.success(CreatedDraftDiaryInfo(createdAt = "2024-06-01T00:00:00")) | |
| } |
🤖 Prompt for AI Agents
In app/src/test/java/com/sopt/clody/FakeDiaryRepository.kt around lines 14 to
16, the property draftDiaryResult is declared but not used in the
fetchDraftDiary method, causing confusion in tests. Update the fetchDraftDiary
implementation to return draftDiaryResult instead of draftDiaryContents,
ensuring that the property controls the method's behavior as expected in tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (1)
208-211: Reconsider success state management for save operations.Clearing failure states on successful save might not provide appropriate user feedback. Users typically expect confirmation that their draft was saved successfully.
Consider adding a success state or toast message instead of just clearing error states:
result.onSuccess { - _failureMessage.value = "" - _showFailureDialog.value = false + // Consider showing success feedback to user + // e.g., a success toast or updating a save status }.onFailure { e ->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/com/sopt/clody/data/remote/dto/request/SaveDraftDiaryRequestDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt(2 hunks)app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt(2 hunks)app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt(9 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt
- app/src/main/java/com/sopt/clody/data/remote/dto/request/SaveDraftDiaryRequestDto.kt
- app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt
- app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt
- app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt
[warning] 206-206: String.format("%04d-%02d-%02d", year, month, day) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (5)
26-27: Good dependency injection of use cases.The use cases are properly injected following the established pattern in this ViewModel.
64-64: Effective change detection mechanism.The
initialEntriesproperty provides a clean way to track changes for draft functionality.
178-180: Simple and effective change detection.The method correctly compares current entries with initial state.
196-200: Use specific error message for draft fetch failures.The AI summary mentions using
ErrorMessages.FETCH_TEMP_DIARY_FAILEDfor fetch failures, but the code uses a generic error message. This inconsistency could lead to less user-friendly error messaging.Verify if the specific error message constant exists and should be used:
#!/bin/bash # Check if FETCH_TEMP_DIARY_FAILED constant exists in ErrorMessages rg "FETCH_TEMP_DIARY_FAILED" --type ktLikely an incorrect or invalid review comment.
218-225: Well-designed fallback mechanism.The
ensureDefaultEntrymethod provides a consistent way to reset to a valid default state when draft operations fail.
| fun saveDraftDiary(year: Int, month: Int, day: Int) { | ||
| viewModelScope.launch { | ||
| val date = String.format("%04d-%02d-%02d", year, month, day) | ||
| val result = saveDraftDiaryUseCase(date, _entries.toList()) | ||
| result.onSuccess { | ||
| _failureMessage.value = "" | ||
| _showFailureDialog.value = false | ||
| }.onFailure { e -> | ||
| _failureMessage.value = e.localizedMessage ?: UNKNOWN_ERROR | ||
| _showFailureDialog.value = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix locale issue and add network check.
Two issues in the saveDraftDiary method:
- String formatting uses implicit default locale (flagged by static analysis)
- Missing network connectivity check for consistency with other operations
Apply this fix:
fun saveDraftDiary(year: Int, month: Int, day: Int) {
viewModelScope.launch {
+ if (!networkUtil.isNetworkAvailable()) {
+ _failureMessage.value = FAILURE_NETWORK_MESSAGE
+ _showFailureDialog.value = true
+ return@launch
+ }
+
- val date = String.format("%04d-%02d-%02d", year, month, day)
+ val date = String.format(java.util.Locale.US, "%04d-%02d-%02d", year, month, day)
val result = saveDraftDiaryUseCase(date, _entries.toList())📝 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 saveDraftDiary(year: Int, month: Int, day: Int) { | |
| viewModelScope.launch { | |
| val date = String.format("%04d-%02d-%02d", year, month, day) | |
| val result = saveDraftDiaryUseCase(date, _entries.toList()) | |
| result.onSuccess { | |
| _failureMessage.value = "" | |
| _showFailureDialog.value = false | |
| }.onFailure { e -> | |
| _failureMessage.value = e.localizedMessage ?: UNKNOWN_ERROR | |
| _showFailureDialog.value = true | |
| } | |
| } | |
| } | |
| fun saveDraftDiary(year: Int, month: Int, day: Int) { | |
| viewModelScope.launch { | |
| if (!networkUtil.isNetworkAvailable()) { | |
| _failureMessage.value = FAILURE_NETWORK_MESSAGE | |
| _showFailureDialog.value = true | |
| return@launch | |
| } | |
| val date = String.format(java.util.Locale.US, "%04d-%02d-%02d", year, month, day) | |
| val result = saveDraftDiaryUseCase(date, _entries.toList()) | |
| result.onSuccess { | |
| _failureMessage.value = "" | |
| _showFailureDialog.value = false | |
| }.onFailure { e -> | |
| _failureMessage.value = e.localizedMessage ?: UNKNOWN_ERROR | |
| _showFailureDialog.value = true | |
| } | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 206-206: String.format("%04d-%02d-%02d", year, month, day) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt
around lines 204 to 216, fix the locale issue by specifying Locale.US explicitly
in the String.format call to avoid locale-dependent formatting. Additionally,
add a network connectivity check before proceeding with saving the draft diary
to ensure consistency with other operations; if there is no network, handle it
appropriately (e.g., show an error or skip the save).
| fun fetchDraftDiary(year: Int, month: Int, day: Int) { | ||
| viewModelScope.launch { | ||
| _entries.clear() | ||
| _showWarnings.clear() | ||
|
|
||
| val result = fetchDraftDiaryUseCase(year, month, day) | ||
| result.onSuccess { response -> | ||
| val drafts = response.draftDiaries.ifEmpty { listOf("") } | ||
| _entries.addAll(drafts) | ||
| initialEntries = drafts.toList() | ||
|
|
||
| _showWarnings.addAll(List(_entries.size) { false }) | ||
| checkLimitMessage() | ||
| checkEmptyFieldsMessage() | ||
| }.onFailure { | ||
| ensureDefaultEntry() | ||
| _failureMessage.value = it.localizedMessage ?: UNKNOWN_ERROR | ||
| _showFailureDialog.value = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add network availability check for consistency.
The fetchDraftDiary method lacks network connectivity validation that's present in writeDiary. This inconsistency could lead to poor UX when offline.
Add network check at the beginning:
fun fetchDraftDiary(year: Int, month: Int, day: Int) {
viewModelScope.launch {
+ if (!networkUtil.isNetworkAvailable()) {
+ ensureDefaultEntry()
+ _failureMessage.value = FAILURE_NETWORK_MESSAGE
+ _showFailureDialog.value = true
+ return@launch
+ }
+
_entries.clear()
_showWarnings.clear()📝 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 fetchDraftDiary(year: Int, month: Int, day: Int) { | |
| viewModelScope.launch { | |
| _entries.clear() | |
| _showWarnings.clear() | |
| val result = fetchDraftDiaryUseCase(year, month, day) | |
| result.onSuccess { response -> | |
| val drafts = response.draftDiaries.ifEmpty { listOf("") } | |
| _entries.addAll(drafts) | |
| initialEntries = drafts.toList() | |
| _showWarnings.addAll(List(_entries.size) { false }) | |
| checkLimitMessage() | |
| checkEmptyFieldsMessage() | |
| }.onFailure { | |
| ensureDefaultEntry() | |
| _failureMessage.value = it.localizedMessage ?: UNKNOWN_ERROR | |
| _showFailureDialog.value = true | |
| } | |
| } | |
| } | |
| fun fetchDraftDiary(year: Int, month: Int, day: Int) { | |
| viewModelScope.launch { | |
| if (!networkUtil.isNetworkAvailable()) { | |
| ensureDefaultEntry() | |
| _failureMessage.value = FAILURE_NETWORK_MESSAGE | |
| _showFailureDialog.value = true | |
| return@launch | |
| } | |
| _entries.clear() | |
| _showWarnings.clear() | |
| val result = fetchDraftDiaryUseCase(year, month, day) | |
| result.onSuccess { response -> | |
| val drafts = response.draftDiaries.ifEmpty { listOf("") } | |
| _entries.addAll(drafts) | |
| initialEntries = drafts.toList() | |
| _showWarnings.addAll(List(_entries.size) { false }) | |
| checkLimitMessage() | |
| checkEmptyFieldsMessage() | |
| }.onFailure { | |
| ensureDefaultEntry() | |
| _failureMessage.value = it.localizedMessage ?: UNKNOWN_ERROR | |
| _showFailureDialog.value = true | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt
between lines 182 and 202, the fetchDraftDiary function lacks a network
availability check, unlike the writeDiary method. To fix this, add a network
connectivity validation at the start of fetchDraftDiary to ensure the device is
online before proceeding. If the network is unavailable, handle it gracefully by
showing an appropriate message or dialog and avoid making the
fetchDraftDiaryUseCase call.
There was a problem hiding this comment.
[p3]
createdAt 정보가 쓰이는 곳이 없는 걸로 파악이 되는데, 일단 서버가 이 값을 내려주는 이유도 궁금하고 굳이 Domain Layer에도 만들어야하나? 라는 생각이 듭니다😂
There was a problem hiding this comment.
저 생성시간을 홈 분기에서 쓰는 걸로 처음에 이해해서 만들어놨었는데 지금 보니 그냥 any?로 처리해도 될거 같네요! 제거 하겠습니다잉
There was a problem hiding this comment.
[p3]
CreatedDraftDiaryInfo 와 비슷한 리뷰인데요, Domain 레이어의 model을 선언해야하는 것의 기준은 어떤 것일까요?
- Clean Architecture를 위해 Dto를 Mapping하여 Presentation Layer에서 의존성을 지킬 수 있도록 하는 것.
- 비즈니스 로직에 걸맞는 꼭 필요한 Model일 것.
1번만큼 중요한게 2번 기준이라고 개인적으로 생각합니다. 그래서 꼭 필요한 Model인가? 비즈니스적인 성격이 잘 드러나는가? 변경 가능성이 작은가? 등을 항상 고려하려고 하는데 지금처럼 간단한 List 타입 하나만 들어있으며, 쓰임새도 확실한(?) 것 같은 경우에는 그냥 별도의 모델 제작하지 않고 List 가 아닌 List으로 사용하는 것은 어떻게 생각하세요?
There was a problem hiding this comment.
도메인 관점에서 "임시 저장된 일기 모음"이라는 개념을 갖고 있다고 생각해서 별도 모델로 분리를 했습니다...
그리고 DraftDiaryContents.draftDiaries 같은 명시적 구조가 더 직관적일 수 있다고 생각합니다?
선생님께서 말씀하신 List로 하면 Presnetation layer에서 의미를 유추하는 구조?로 될것 같은데
혹시 이걸 의도 하신 설계를 생각하신 건지 궁금합니데이~
| }.fold( | ||
| onSuccess = { Result.success(it) }, | ||
| onFailure = { | ||
| Result.failure(Exception(ErrorMessages.FETCH_TEMP_DIARY_FAILED)) | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
[p3]
fold 에 대한 처리는 viewModel에서 항상 진행했었는데 이제 옮기는건가요?
There was a problem hiding this comment.
이전 프로젝트에서 Repository단에서 처리해서 자연스럽게 여기에 놨네요..
뷰모델로 이동시키겠습니다!
지금 보니 어떤건 뷰모델 어떤건 리포지토리에 있는걸 보아 나중에 대규모 공사 한번 들어가야될 것 같습니다ㅠ
2 participants
📌 ISSUE
closed #265
📄 Work Description
임시저장 기능 구현을 위해 다음과 같은 작업을 진행했습니다.
1. 임시저장 API 연결
fetchDraftDiary,saveDraftDiary메서드 추가Service → DataSource → Repository계층을 통해 도메인까지 연결2. 전체 구조 적용
Service → DataSource → Repository → UseCase → ViewModel → UIWriteDiaryViewModel에 임시저장 fetch/save 로직 구현initialEntries값을 기준으로 변경 여부 판단 로직 작성3. UI 연동 및 생명주기 처리
WriteDiaryScreen진입 시fetchDraftDiary자동 호출repeatOnLifecycle(Lifecycle.State.STARTED)기반의 유틸 함수LaunchedEffectWhenStarted활용4. 간단한 테스트 코드 작성
FakeDiaryRepository,FakeDiaryRemoteDataSource구현✨ PR Point
이론상 제대로 동작할 겁니다? ㅋㅋㅋㅋㅋ.....
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests