Conversation
- OAuthProvider 클래스의 apiValue 속성명을platform으로 변경하여 서버와 통일하고, 직관적으로 개선했습니다. - UserInfoResponseDto는 서버로 오는 데이터 타입과 최대한 유사하게 받고 OAuthProvider 등 커스텀 클래스로 매핑하는 부분은 추후 Domain Layer를 이용하는게 좋을 것 같다 생각되어 수정했습니다. - 서버와 협의 후에 로그인 API의 404 결과를 핸들링하여 회원가입 분기처리를 하도록 통일했습니다. (500삭제)
- LanguageProviderImpl에서 locale 변수 가져오는 방식을 조금 더 가볍게 개선했습니다. 기존 방식은 앱 내 다국어 지원 기능이 있을때 효과적인 방법이라고 합니다.
- 기존 로직은 닉네임 설정 완료 후 다시 한번 카카오 sdk를 호출하여 토큰을 받아 회원가입 api에 사용했습니다. - 로그인 화면에서 카카오 sdk 호출 후 OAuthDataStore에 토큰을 저장하도록 수정하여 두 번 호출하는 로직을 수정하며, 구글 로그인과 로직을 비슷하게 수정하였습니다.
WalkthroughThis update removes the internal WebView feature and its secure domain restrictions, shifting all web link navigation to external browsers. It also refactors OAuth token handling, updates diary writing to support localization, adjusts UI layouts and spacing, and enhances version info display with remote config checks. Minor bug fixes and code cleanups are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppScreen
participant ExternalBrowser
User->>AppScreen: Clicks web link (e.g., Terms, Notice)
AppScreen->>ExternalBrowser: Calls openExternalBrowser(context, url)
ExternalBrowser-->>User: Opens URL in external browser
sequenceDiagram
participant User
participant WriteDiaryViewModel
participant LanguageProvider
participant DiaryRepository
User->>WriteDiaryViewModel: Submit diary entry (date, content)
WriteDiaryViewModel->>LanguageProvider: getCurrentLanguageTag()
LanguageProvider-->>WriteDiaryViewModel: lang
WriteDiaryViewModel->>WriteDiaryViewModel: convertDateToKstDateTime()
WriteDiaryViewModel->>DiaryRepository: writeDiary(lang, kstDate, content)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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: 4
🔭 Outside diff range comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingViewModel.kt (1)
35-51: Fetch Remote Config and Separate Error Paths in getVersionInfoWe need to ensure you’re actually fetching the latest remote values before comparing—and handle those failures independently from local package errors.
• Location to update:
• app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingViewModel.kt → getVersionInfo()• Key concerns:
- remoteConfigDataSource.getLatestVersion() falls back to BuildConfig.VERSION_NAME if no fetch has happened—so you’ll always see “latest” unless you call fetch().
- Exceptions from fetching remote config are currently silent—consider catching them separately so a bad network doesn’t mark a stale version as “latest.”
- Make sure the string returned by PackageManager (which may include build‐type suffixes) matches exactly your KEY_LATEST_VERSION format.
Suggested update (diff):
viewModelScope.launch { - runCatching { - val info: PackageInfo = context.packageManager.getPackageInfo(context.packageName, 0) - info.versionName - }.onSuccess { versionName -> - val latestVersion = remoteConfigDataSource.getLatestVersion() - versionInfo = if (versionName == latestVersion) { - context.getString(R.string.setting_option_app_version_info_latest) - } else { - versionName - } - }.onFailure { - versionInfo = context.getString(R.string.setting_option_app_version_info_failure) - } + // 1) Load local version + val versionName = runCatching { + context.packageManager.getPackageInfo(context.packageName, 0).versionName + }.getOrElse { + versionInfo = context.getString(R.string.setting_option_app_version_info_failure) + return@launch + } + + // 2) Fetch remote config and load latest version + val latestVersion = runCatching { + remoteConfigDataSource.fetch() + remoteConfigDataSource.getLatestVersion() + }.getOrNull() + + // 3) Compare exactly—and fallback to local if fetch failed or mismatch + versionInfo = if (latestVersion != null && versionName == latestVersion) { + context.getString(R.string.setting_option_app_version_info_latest) + } else { + versionName + } }This ensures you’re comparing truly up-to-date values, surfaces fetch errors separately, and preserves your existing “failure” UI only for package manager issues.
🧹 Nitpick comments (13)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
62-67: Clarify the function's intended behaviorThe documentation states this function converts "user's current time point" to KST, but the implementation suggests it may handle both current and past dates differently. Please clarify the intended use case and expected behavior.
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt (1)
86-87: Swallowing Google-sign-in errors may hide real issues
onFailureimmediately dispatchesClearError, clearing any message thatgoogleSignInHelpermight have set moments before. Users get no feedback.Consider surfacing the actual failure via
LoginIntent.ShowError(message)(or similar) instead of wiping it.app/src/main/java/com/sopt/clody/presentation/ui/setting/component/AccountManagementLogoutOption.kt (1)
24-24: Consider adding validation for platform parameter.Changing from
OAuthProvider?toStringreduces type safety. The enum previously provided compile-time validation of valid platform values, while strings can accept any value.Consider adding validation to ensure only valid platform strings are accepted:
@Composable fun AccountManagementLogoutOption( userEmail: String, platform: String, updateLogoutDialog: (Boolean) -> Unit, ) { + require(platform in setOf("kakao", "google")) { "Invalid platform: $platform" } val platformIconRes = when (platform) {app/src/main/java/com/sopt/clody/presentation/utils/OpenExternalBrowser.kt (1)
7-17: LGTM! Consider improvements for robustness.The implementation correctly follows Android best practices for opening URLs externally. The function properly handles the case where no browser app is available.
Consider these improvements for robustness:
- Add URL validation:
fun openExternalBrowser(context: Context, url: String) { + if (url.isBlank()) return + val uri = Uri.parse(url) + if (uri.scheme.isNullOrEmpty()) return
- Use the newer resolveActivity API for better compatibility:
- context.packageManager.resolveActivity(intent, 0)?.let { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + context.packageManager.resolveActivity(intent, PackageManager.ResolveInfoFlags.of(0)) + } else { + @Suppress("DEPRECATION") + context.packageManager.resolveActivity(intent, 0) + }?.let {app/src/main/java/com/sopt/clody/data/datastore/OAuthDataStore.kt (2)
14-20: LGTM! Consider improving platform validation.The function correctly supports multiple OAuth providers. However, the else clause assumes all non-Google platforms are Kakao, which could cause issues when adding more providers.
Consider explicit platform validation:
suspend fun saveIdToken(platform: String, token: String) { - if (platform == OAuthProvider.GOOGLE.platform) { - dataStore.edit { it[OAuthDataStoreKeys.GOOGLE_ID_TOKEN] = token } - } else { - dataStore.edit { it[OAuthDataStoreKeys.KAKAO_ID_TOKEN] = token } - } + when (platform) { + OAuthProvider.GOOGLE.platform -> dataStore.edit { it[OAuthDataStoreKeys.GOOGLE_ID_TOKEN] = token } + OAuthProvider.KAKAO.platform -> dataStore.edit { it[OAuthDataStoreKeys.KAKAO_ID_TOKEN] = token } + else -> throw IllegalArgumentException("Unsupported platform: $platform") + } }
22-28: LGTM! Same validation concerns assaveIdToken.The function correctly retrieves tokens based on platform but has the same robustness concerns regarding the else clause assumption.
Apply the same explicit validation pattern suggested for
saveIdToken:suspend fun getIdToken(platform: String): String? { - return if (platform == OAuthProvider.GOOGLE.platform) { - dataStore.data.first()[OAuthDataStoreKeys.GOOGLE_ID_TOKEN] - } else { - dataStore.data.first()[OAuthDataStoreKeys.KAKAO_ID_TOKEN] - } + return when (platform) { + OAuthProvider.GOOGLE.platform -> dataStore.data.first()[OAuthDataStoreKeys.GOOGLE_ID_TOKEN] + OAuthProvider.KAKAO.platform -> dataStore.data.first()[OAuthDataStoreKeys.KAKAO_ID_TOKEN] + else -> throw IllegalArgumentException("Unsupported platform: $platform") + } }app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingViewModel.kt (1)
31-33: Consider startup performance impact.Automatically fetching version info during initialization ensures it's ready when the settings screen loads. However, since this involves a remote config call, consider whether this could impact app startup performance.
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt (3)
102-102: Consider using constants for platform strings.The explicit platform parameters support multi-provider OAuth handling correctly. Consider using constants instead of string literals to prevent typos.
+ companion object { + private const val PLATFORM_KAKAO = "kakao" + private const val PLATFORM_GOOGLE = "google" + } + - oauthDataStore.saveIdToken(platform = "kakao", token = kakaoToken) + oauthDataStore.saveIdToken(platform = PLATFORM_KAKAO, token = kakaoToken) - oauthDataStore.saveIdToken(platform = "google", token = idToken) + oauthDataStore.saveIdToken(platform = PLATFORM_GOOGLE, token = idToken)Also applies to: 127-127
101-103: Consider using a constant for platform string.The error handling simplification and explicit platform parameter are good improvements. However, consider using
OAuthProvider.KAKAO.platforminstead of the hardcoded "kakao" string for consistency and maintainability.Apply this diff to use the enum value:
- oauthDataStore.saveIdToken(platform = "kakao", token = kakaoToken) + oauthDataStore.saveIdToken(platform = OAuthProvider.KAKAO.platform, token = kakaoToken)
126-127: Consider using a constant for platform string.Good consistency with the Kakao implementation. Same suggestion applies here - consider using
OAuthProvider.GOOGLE.platforminstead of the hardcoded "google" string.Apply this diff to use the enum value:
- oauthDataStore.saveIdToken(platform = "google", token = idToken) + oauthDataStore.saveIdToken(platform = OAuthProvider.GOOGLE.platform, token = idToken)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (3)
159-159: Consider using a constant for platform string.Good refactoring to use the unified OAuth data store. Consider using
OAuthProvider.GOOGLE.platforminstead of the hardcoded "google" string for consistency.
173-185: Excellent refactoring with minor suggestions.Great unification of OAuth handling logic. Both providers now follow the same pattern, eliminating complexity. Two suggestions:
- Use
OAuthProvider.KAKAO.platforminstead of hardcoded "kakao"- Consider internationalizing the error message for consistency
Apply this diff for the platform constant:
- val idToken = oAuthDataStore.getIdToken(platform = "kakao") + val idToken = oAuthDataStore.getIdToken(platform = OAuthProvider.KAKAO.platform) - platform = OAuthProvider.KAKAO.platform, + platform = OAuthProvider.KAKAO.platform,
161-161: Verify error message tone consistency.The error messages are clear and appropriate. However, the sign-up failure message uses an informal tone with "~" - ensure this aligns with the app's overall error messaging style.
Also applies to: 175-175, 199-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
app/build.gradle.kts(0 hunks)app/src/main/java/com/sopt/clody/core/login/KakaoLoginSdk.kt(1 hunks)app/src/main/java/com/sopt/clody/core/security/DefaultLoginSecurityChecker.kt(1 hunks)app/src/main/java/com/sopt/clody/core/security/LoginSecurityChecker.kt(1 hunks)app/src/main/java/com/sopt/clody/core/security/SecurityModule.kt(1 hunks)app/src/main/java/com/sopt/clody/core/security/weview/SecureWebViewClient.kt(0 hunks)app/src/main/java/com/sopt/clody/data/datastore/OAuthDataStore.kt(2 hunks)app/src/main/java/com/sopt/clody/data/datastore/OAuthDataStoreKeys.kt(1 hunks)app/src/main/java/com/sopt/clody/data/datastore/OAuthProvider.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/api/DiaryService.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/request/WriteDiaryRequestDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/UserInfoResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/GuideScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/GoogleSignInHelper.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/component/AccountManagementLogoutOption.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/component/SettingVersionInfo.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingViewModel.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/webview/WebViewNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/webview/WebViewScreen.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/utils/OpenExternalBrowser.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/language/LanguageProvider.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProviderImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt(0 hunks)app/src/main/res/values-ko/strings.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
💤 Files with no reviewable changes (8)
- app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt
- app/build.gradle.kts
- app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt
- app/src/main/java/com/sopt/clody/presentation/ui/webview/WebViewNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt
- app/src/main/java/com/sopt/clody/core/security/weview/SecureWebViewClient.kt
- app/src/main/java/com/sopt/clody/presentation/ui/webview/WebViewScreen.kt
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/OpenExternalBrowser.kt (1)
openExternalBrowser(7-17)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/OpenExternalBrowser.kt (1)
openExternalBrowser(7-17)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
convertDateToKstDateTime(68-82)
⏰ 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 (66)
app/src/main/java/com/sopt/clody/data/remote/dto/request/WriteDiaryRequestDto.kt (1)
9-9: LGTM: Formatting improvementThe added space after the annotation improves code readability and follows consistent formatting patterns.
app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProvider.kt (1)
7-7: LGTM: Clean interface extension for localizationThe addition of
getCurrentLanguageTag()properly extends the interface to support language localization without breaking existing contracts.app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt (1)
22-24: LGTM: Consistent parameter propagationThe
langparameter is correctly added to the method signature and properly forwarded to the data source layer, maintaining consistency with the interface contract.app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt (1)
12-12: LGTM: Well-designed interface extensionThe addition of the
langparameter enables proper localization support for diary writing. The parameter ordering is logical with language context as the first argument.app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt (1)
14-14: LGTM! Interface updated correctly for localization support.The addition of the
langparameter as the first argument follows good practices for API design and aligns with the coordinated changes across the data layer.app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt (1)
20-21: LGTM! Implementation correctly updated to match interface.The
langparameter is properly forwarded to the service layer, maintaining consistency with the interface changes.app/src/main/java/com/sopt/clody/data/remote/api/DiaryService.kt (2)
16-16: LGTM! Correct import added for header annotation.
22-25: LGTM! Proper use of Accept-Language header for localization.Using the
Accept-LanguageHTTP header follows standard practices for API internationalization and content negotiation.app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (1)
15-15: LGTM! Correct import for timezone utility.app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProviderImpl.kt (3)
8-9: Simplified constructor improves testability.Removing the Context dependency and using
Locale.getDefault()simplifies the class and makes it more testable. However, ensure this doesn't break app-specific locale settings that might override the system locale.
13-14: LGTM! Correct implementation of language tag retrieval.The
getCurrentLanguageTag()method properly useslocale.toLanguageTag()which returns the standard language tag format (e.g., "ko-KR", "en-US").
16-27: LGTM! Single-expression functions improve readability.Converting to single-expression function syntax makes the code more concise and readable while maintaining the same functionality.
app/src/main/res/values/strings.xml (1)
95-95: LGTM!The new string resource follows proper naming conventions and is appropriately placed in the Setting section. The value is clear and concise for displaying version status.
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/GuideScreen.kt (5)
14-14: Good addition for layout spacing.Adding the height import supports the improved spacing implementation below.
113-113: Improved modifier simplification.Removing
fillMaxSize()from AnimatedVisibility and keeping only padding makes the modifier more focused and avoids potential layout conflicts.
116-116: Excellent layout refactoring using Compose best practices.The switch from custom proportional heights to using
Spacerwithweight()modifiers follows Compose layout best practices. This approach provides:
- More predictable and flexible layouts
- Better maintainability
- Standard Compose patterns that other developers will recognize
Also applies to: 119-119, 122-122
134-134: Consistent spacing implementation.Using fixed
height()values andweight()modifiers provides consistent spacing that will adapt better to different screen sizes while maintaining visual hierarchy.Also applies to: 141-141, 149-149
153-153: Clean layout for pager indicators.Replacing horizontal arrangement with vertical padding simplifies the Row layout and achieves the same visual result more efficiently.
app/src/main/java/com/sopt/clody/core/security/SecurityModule.kt (1)
1-1: Good package consolidation.Moving from
com.sopt.clody.core.security.logintocom.sopt.clody.core.securitysimplifies the package structure and consolidates security-related classes under a unified namespace.app/src/main/java/com/sopt/clody/core/security/LoginSecurityChecker.kt (1)
1-1: Consistent package restructuring.The package change aligns with the broader security module consolidation, maintaining the interface contract while improving code organization.
app/src/main/java/com/sopt/clody/core/security/DefaultLoginSecurityChecker.kt (1)
1-1: Package restructuring completed.The package change completes the consolidation of security-related classes under the unified
com.sopt.clody.core.securitypackage, maintaining all existing security functionality.app/src/main/java/com/sopt/clody/core/login/KakaoLoginSdk.kt (1)
10-10: No residual old imports found
A full-text search forimport com.sopt.clody.core.security.login.LoginSecurityCheckerreturned no matches—every reference now points tocom.sopt.clody.core.security.LoginSecurityChecker.app/src/main/java/com/sopt/clody/presentation/ui/setting/component/SettingVersionInfo.kt (1)
36-36: Typography change LGTMUsing
body4Mediumaligns with the new design-system scale and the previousletterSpacingremoval simplifies styling.app/src/main/java/com/sopt/clody/data/datastore/OAuthProvider.kt (1)
3-6: Enum update looks consistentRenaming the ctor param to
platformmatches downstream usage; no functional impact observed.app/src/main/java/com/sopt/clody/data/datastore/OAuthDataStoreKeys.kt (1)
7-8: All Kakao token accesses use theKAKAO_ID_TOKENconstant
Verified that every read, write, and removal of the Kakao ID token inOAuthDataStore.ktreferencesOAuthDataStoreKeys.KAKAO_ID_TOKEN; no hard-coded"kakao_id_token"strings remain. No further action needed.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (1)
8-8: Good UI improvement for edge-to-edge support.Adding
navigationBarsPadding()to the toast message ensures it won't be obscured by the device's navigation bar, improving the user experience on modern Android devices.Also applies to: 96-96
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt (1)
72-72: Good code simplification.Removing the unnecessary object copying and intermediate variable assignment makes the code cleaner and more efficient, especially since the platform type transformation is no longer needed.
app/src/main/res/values-ko/strings.xml (1)
94-94: Proper localization for version info feature.The Korean translation "최신 버전" for "Latest version" is accurate and supports the enhanced version information display mentioned in the PR objectives.
app/src/main/java/com/sopt/clody/presentation/ui/login/GoogleSignInHelper.kt (3)
4-4: Good import addition for type clarity.Adding the explicit
Intentimport improves code readability and ensures proper type resolution.
31-42: Excellent encapsulation improvement.Making
buildSignInRequest()private is a good encapsulation practice since this method is only used internally within the class.
44-44: Improved parameter type specification.Using
Intent?as the parameter type instead of a generic type improves type safety and makes the method signature more explicit about its expected input.app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt (2)
15-15: LGTM!The import for
openExternalBrowseris correctly added to support the shift from internal WebView to external browser navigation.
32-32: LGTM!The side effect handling correctly uses
openExternalBrowserto open URLs externally instead of navigating to an internal WebView. This improves security and simplifies the navigation flow.app/src/main/java/com/sopt/clody/presentation/ui/setting/component/AccountManagementLogoutOption.kt (1)
27-31: LGTM! Clarify server contract for platform values.The icon selection logic correctly handles string-based platform identifiers. However, the comment on Line 30 suggests uncertainty about how the server will provide the "google" platform value.
Please verify the server API contract to ensure consistent platform string values. Consider documenting the expected platform values to avoid confusion.
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt (3)
13-13: LGTM!The imports for
LocalContextandopenExternalBrowserare correctly added to support the shift from internal WebView to external browser navigation.Also applies to: 23-23
38-38: LGTM!The context is correctly obtained using
LocalContext.currentto support the external browser functionality.
49-52: LGTM!The onClick handlers are correctly updated to use
openExternalBrowserfor all web links, providing a consistent user experience and improved security by avoiding internal WebView usage.app/src/main/java/com/sopt/clody/data/datastore/OAuthDataStore.kt (1)
43-43: LGTM!Adding Kakao token removal to the clear function ensures all OAuth tokens are properly cleared during logout, preventing token leakage.
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingViewModel.kt (9)
11-11: LGTM! Clean dependency injection.The
RemoteConfigDataSourceis properly injected following Hilt patterns.Also applies to: 21-21
11-11: LGTM!Clean import addition for the new remote config dependency.
21-21: LGTM!Proper dependency injection following established patterns.
31-33: LGTM!Good practice to initialize version info in the constructor for immediate availability.
35-35: Incorrect suggestion on remote config error handlingThe
getLatestVersion()method is non-suspending and simply reads the cached value (with aBuildConfig.VERSION_NAMEfallback). It doesn’t perform any network I/O or callfetchAndActivate(), so it cannot throw on failure. The network fetch and its error handling occur in the suspend function that invokesremoteConfig.fetchAndActivate(), which isn’t called here. You can ignore the original recommendation to add error handling aroundgetLatestVersion().Likely an incorrect or invalid review comment.
11-11: LGTM!The dependency injection for
RemoteConfigDataSourceis properly implemented with correct import and constructor parameter.Also applies to: 21-21
31-33: LGTM!Initializing version info in the
initblock ensures the data is available when the UI is rendered.
35-51: LGTM!The version comparison logic is well-implemented with proper error handling. The function correctly compares current version with remote config and provides appropriate fallback behavior.
35-35: Good encapsulation practice.Making
getVersionInfo()private is appropriate since it's only used internally during initialization.app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt (7)
90-90: LGTM! Consistent platform field usage.The update to use
.platformaligns with the enum refactoring and maintains consistency across OAuth providers.Also applies to: 114-114
90-90: LGTM!Clean field name change aligning with the OAuthProvider enum refactoring.
114-114: LGTM!Consistent with the platform field usage pattern established in the Kakao flow.
90-90: LGTM!The platform parameter usage aligns with the enum refactoring from
apiValuetoplatform.
101-104: LGTM!The simplified error handling using "404" as the indicator for sign-up navigation is cleaner, and the explicit platform parameter improves clarity.
126-129: LGTM!The error handling is now consistent between Google and Kakao providers, with clear explicit platform parameters for token saving.
114-114: ✅ OAuth DTO selection is intentionalThe Google flow uses a dedicated
GoogleSignUpRequestDto(idToken, fcmToken)against thesignUpWithGoogleendpoint, while Kakao uses the genericLoginRequestDto(platform, fcmToken)withsignIn. This matches the domain and remote API definitions:
- AuthService.kt
•suspend fun signUpWithGoogle(@Body GoogleSignUpRequestDto): ApiResponse<SignUpResponseDto>
•suspend fun signIn(@Header("Authorization") String, @Body LoginRequestDto): ApiResponse<LoginResponseDto>No changes are required.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (12)
159-163: LGTM! Proper token retrieval and error handling.The unified token retrieval from
OAuthDataStorewith explicit platform parameter is a good refactoring. Error handling for missing tokens is appropriate.
165-165: LGTM! Consistent platform field usage.The update to use
.platformmaintains consistency with the enum refactoring across the codebase.Also applies to: 179-179
171-171: LGTM! Simplified and consistent function calls.The unified
handleSignUpResultcalls remove the need to distinguish between OAuth providers, which is a good simplification.Also applies to: 185-185
195-195: LGTM! Appropriate cleanup of OAuth data.Unconditionally clearing the OAuth data store after successful signup is the right approach, as the temporary OAuth tokens are no longer needed once the user has access/refresh tokens.
173-182: Token storage for Kakao flow is correctly implemented.LoginViewModel saves both the Kakao ID token and platform before emitting NavigateToSignUp, so
oAuthDataStore.getIdToken("kakao")will always return a value at signup. No further changes needed.
165-165: LGTM!Consistent with the OAuthProvider enum refactoring.
171-171: LGTM!Good simplification removing the unnecessary isGoogle parameter.
195-195: LGTM!Good simplification to unconditionally clear OAuth data after successful sign-up, aligning with the unified OAuth handling approach.
159-172: LGTM!The unified token retrieval from
OAuthDataStorewith explicit platform parameter and added error handling for missing tokens improves robustness and consistency.
173-186: LGTM!The Kakao token handling now mirrors the Google implementation, creating consistency between OAuth providers. The unified approach through
OAuthDataStoreis much cleaner.
189-203: LGTM!The simplified
handleSignUpResultfunction reduces complexity by removing theisGoogleparameter, since both providers now follow the same flow.
165-165: LGTM!The consistent use of
.platforminstead of.apiValuealigns with the enum refactoring across the codebase.Also applies to: 179-179
| @SerialName("email") val email: String, | ||
| @SerialName("name") val name: String, | ||
| @SerialName("platform") val platform: OAuthProvider?, | ||
| @SerialName("platform") val platform: String, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validation for platform string values.
The change from OAuthProvider? enum to String reduces compile-time type safety. Ensure that proper validation is implemented elsewhere in the codebase to prevent invalid platform values from causing runtime issues.
Consider adding validation in the data layer or creating a sealed class/enum for platform types:
+@Serializable
+enum class PlatformType {
+ @SerialName("google") GOOGLE,
+ @SerialName("kakao") KAKAO
+}
@Serializable
data class UserInfoResponseDto(
@SerialName("email") val email: String,
@SerialName("name") val name: String,
- @SerialName("platform") val platform: String,
+ @SerialName("platform") val platform: PlatformType,
)📝 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("platform") val platform: String, | |
| // Add this enum (e.g. at the top of the file, after imports) | |
| @Serializable | |
| enum class PlatformType { | |
| @SerialName("google") GOOGLE, | |
| @SerialName("kakao") KAKAO | |
| } | |
| @Serializable | |
| data class UserInfoResponseDto( | |
| @SerialName("email") val email: String, | |
| @SerialName("name") val name: String, | |
| @SerialName("platform") val platform: PlatformType, | |
| ) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/dto/response/UserInfoResponseDto.kt
at line 10, the platform field was changed from an OAuthProvider? enum to a
String, which removes compile-time type safety. To fix this, implement
validation logic either in the data layer or by defining a sealed class or enum
representing valid platform values. This validation should check that the
platform string matches one of the allowed values before it is used, preventing
invalid platform values from causing runtime errors.
| if (msg.contains("404")) { | ||
| oauthDataStore.saveIdToken(platform = "kakao", token = kakaoToken) | ||
| oauthDataStore.savePlatform(OAuthProvider.KAKAO) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify simplified error handling doesn't degrade UX.
The error handling was simplified to only check for "404" to determine signup navigation. This could impact user experience by:
- Treating all non-404 errors the same way (generic error message)
- Using fragile string matching instead of proper error codes
- Potentially losing specific error handling for network issues, rate limits, etc.
Please verify this change aligns with the backend API design and doesn't degrade error handling:
Also applies to: 126-128
🏁 Script executed:
#!/bin/bash
# Description: Check for other error handling patterns in the codebase
# Expected: Find if this simplified approach is used elsewhere or if more sophisticated error handling exists
# Look for other error handling patterns in auth-related files
rg -A 5 -B 2 "error|Error|failure|Failure" --type kotlin app/src/main/java/com/sopt/clody/presentation/ui/login/ app/src/main/java/com/sopt/clody/presentation/ui/auth/
# Check for error code or status code handling
rg -A 3 "404|status|code|Code" --type kotlin app/src/main/java/com/sopt/clody/Length of output: 46896
Improve error handling in LoginViewModel
The current logic in LoginViewModel.kt only checks error.message.contains("404") to decide whether to navigate to the sign-up screen, which:
- Collapses all non-404 failures into a generic error, masking network or server issues
- Relies on brittle string matching instead of using actual HTTP status codes
- Deviates from the app’s standard error-mapping approach (see ErrorMessages and repository exception.code())
To align with the rest of the codebase and ensure a better UX, please:
- Extract the HTTP status code (e.g. via
HttpException.code()or your API response wrapper) instead of string searches - Reuse the centralized
ErrorMessagesfor 4xx (temporary/client), 5xx (server), and unknown errors - Add a network-availability check (as in TimeReminderViewModel) before attempting login
- Clearly distinguish “user not found” (404 → sign-up) from other error classes
Locations requiring updates:
- app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt
• onFailure block for OAuthProvider.KAKAO (lines ~101–103)
• onFailure block for OAuthProvider.GOOGLE (lines ~126–128)
Example refactor snippet:
val status = (error as? HttpException)?.code() ?: -1
when {
status == 404 -> {
oauthDataStore.saveIdToken(…, token)
_sideEffects.send(NavigateToSignUp)
}
status in 400..499 -> showError(FAILURE_TEMPORARY_MESSAGE)
status in 500..599 -> showError(FAILURE_SERVER_MESSAGE)
else -> showError(error.localizedMessage ?: UNKNOWN_ERROR)
}🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt
around lines 101 to 103, replace the current error handling that checks
error.message.contains("404") with logic that extracts the HTTP status code
using HttpException.code(). Use a when block to distinguish 404 errors (trigger
sign-up navigation and save token), 4xx errors (show temporary failure message),
5xx errors (show server failure message), and other errors (show unknown error).
Also, add a network availability check before login attempts similar to
TimeReminderViewModel. Apply the same pattern to the onFailure block for
OAuthProvider.GOOGLE around lines 126 to 128.
| val lang = languageProvider.getCurrentLanguageTag() | ||
| val date = convertDateToKstDateTime(year, month, day) | ||
| val result = diaryRepository.writeDiary(lang, date, contents) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review the date conversion logic for correctness.
The language provider usage is correct, but the convertDateToKstDateTime function has questionable logic. Based on the implementation in TimeZoneExt.kt, it only considers whether the target date equals today's date, then either uses current KST time or yesterday's KST time. This doesn't properly utilize the input year, month, day parameters for historical dates.
Run this script to verify how the date conversion function is used across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check usage of convertDateToKstDateTime and verify the logic
# Search for usages of the function
echo "=== Usage of convertDateToKstDateTime ==="
rg -A 3 -B 3 "convertDateToKstDateTime"
echo "=== Full implementation of convertDateToKstDateTime ==="
rg -A 15 "fun convertDateToKstDateTime"Length of output: 4845
Adjust convertDateToKstDateTime to respect the input date parameters
The current implementation in TimeZoneExt.kt only ever returns “now in KST” or “now minus one day,” regardless of the year/month/day you pass in. As a result, any diary entry for a historical or future date will incorrectly get yesterday’s timestamp. We need to build a ZonedDateTime from the actual input date:
• File: app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
Lines: around the convertDateToKstDateTime function
Suggested change:
- val targetZonedDateTime = if (targetDate == localNowDate) {
- nowKst
- } else {
- nowKst.minusDays(1)
- }
+ // Build a ZonedDateTime for the exact input date at the current KST time
+ val targetZonedDateTime = LocalDateTime.of(
+ year, month, day,
+ nowKst.hour, nowKst.minute, nowKst.second
+ ).atZone(kstZone)This ensures that:
- Entries for today use the current KST time.
- Entries for any other date use that date with the same time-of-day in KST.
📝 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 lang = languageProvider.getCurrentLanguageTag() | |
| val date = convertDateToKstDateTime(year, month, day) | |
| val result = diaryRepository.writeDiary(lang, date, contents) | |
| fun convertDateToKstDateTime(year: Int, month: Int, day: Int): ZonedDateTime { | |
| val kstZone = ZoneId.of("Asia/Seoul") | |
| val nowKst = ZonedDateTime.now(kstZone) | |
| val localNowDate = LocalDate.now(kstZone) | |
| val targetDate = LocalDate.of(year, month, day) | |
| // Build a ZonedDateTime for the exact input date at the current KST time | |
| val targetZonedDateTime = LocalDateTime.of( | |
| year, month, day, | |
| nowKst.hour, nowKst.minute, nowKst.second | |
| ).atZone(kstZone) | |
| return targetZonedDateTime | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt
lines 85-87, the call to convertDateToKstDateTime passes year, month, and day,
but the function in
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
ignores these parameters and always returns now or now minus one day in KST. To
fix this, update convertDateToKstDateTime to construct a ZonedDateTime using the
input year, month, and day combined with the current time-of-day in KST,
returning the current KST time only if the input date is today; otherwise,
return the ZonedDateTime for the specified date at the same time-of-day in KST.
| fun convertDateToKstDateTime(year: Int, month: Int, day: Int): String { | ||
| val localNowDate = LocalDate.now() | ||
| val targetDate = LocalDate.of(year, month, day) | ||
| val kstZone = ZoneId.of("Asia/Seoul") | ||
| val nowKst = ZonedDateTime.now(kstZone) | ||
|
|
||
| val targetZonedDateTime = if (targetDate == localNowDate) { | ||
| nowKst | ||
| } else { | ||
| nowKst.minusDays(1) | ||
| } | ||
|
|
||
| val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss") | ||
| return targetZonedDateTime.format(formatter) | ||
| } |
There was a problem hiding this comment.
Review the date-time calculation logic
The conditional logic in lines 74-78 appears problematic:
val targetZonedDateTime = if (targetDate == localNowDate) {
nowKst
} else {
nowKst.minusDays(1)
}This logic always uses either "current KST time" or "current KST time minus 1 day", regardless of what the actual target date is. If a user is writing a diary for a specific past date, the function should use that target date, not just subtract 1 day from now.
Consider this approach instead:
-val targetZonedDateTime = if (targetDate == localNowDate) {
- nowKst
-} else {
- nowKst.minusDays(1)
-}
+val targetZonedDateTime = if (targetDate == localNowDate) {
+ nowKst
+} else {
+ ZonedDateTime.of(targetDate, LocalTime.now(kstZone), kstZone)
+}This ensures the target date is respected while maintaining current time context.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
around lines 68 to 82, the current logic incorrectly assigns the
targetZonedDateTime by either using the current KST time or subtracting one day
from it, ignoring the actual target date. To fix this, construct the
ZonedDateTime using the targetDate combined with the current time components in
KST, ensuring the returned datetime accurately reflects the specified date with
the current time context.
- airbridge 라이브러리 때문에 dataExtractionRules, fullBackupContent 부분에서 중복선언이 이루어져 replace를 확장했습니다.
2 participants
📌 ISSUE
closed #314
📄 Work Description
✨ PR Point
에러 처리 관련 개선과 국제화만 하면 5차 스프린트 마무리 지을 수 있을 것 같습니다!
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Localization