서버 로그인 제거 및 Room DB 기반 로그인 구조 개선 #313
Conversation
kez-lab
added
🐱의진
WalkthroughThis change removes all Kakao login, authentication, and point-related features from the codebase. It deletes associated modules, classes, repositories, network services, UI elements, and dependencies. The build configuration, navigation, and resource files are updated to reflect these removals. Some onboarding and profile flows are simplified, and related data models and interfaces are eliminated. Changes
Sequence Diagram(s)Omitted due to the large-scale and multi-feature nature of the changes. Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 12
🔭 Outside diff range comments (1)
core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt (1)
51-62: Avoid using GlobalScope for database operationsUsing
GlobalScopeis discouraged as it can lead to memory leaks and uncontrolled coroutine lifecycles. Consider using a more controlled scope or making this a suspend function.Replace GlobalScope with a suspend function:
- @OptIn(DelicateCoroutinesApi::class) - fun deleteAll() { - GlobalScope.launch { - usageGoalsDao().deleteAll() - usageTotalGoalDao().deleteAll() - challengeDao().deleteAll() - deletedGoalsDao().deleteAll() - lockDao().deleteAll() - userAuthDao().clearUserAuth() - userProfileDao().clearUserProfile() - } + suspend fun deleteAll() { + usageGoalsDao().deleteAll() + usageTotalGoalDao().deleteAll() + challengeDao().deleteAll() + deletedGoalsDao().deleteAll() + lockDao().deleteAll() + userAuthDao().clearUserAuth() + userProfileDao().clearUserProfile() }
♻️ Duplicate comments (1)
domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignUpUser.kt (1)
3-7: Mirror theLoginchange – same migration caveat applies.Same comment as for
Login.userId; ensure the DB & serialization layers can ingest / emitLongseamlessly.
🧹 Nitpick comments (24)
core/common/src/main/java/com/hmh/hamyeonham/common/context/ContextExt.kt (2)
4-4: Unused importandroid.app.Activity
Activityis not referenced anywhere in this file. Keeping unused imports will trigger IDE / lint warnings and clutters the diff.
Remove it (or add the intended extension/utility that requires it).
7-7: Unused importandroid.content.ContextWrapperSame as above – the symbol is currently unused. Delete the import or introduce the code that depends on it.
app/src/main/res/layout/activity_sample.xml (2)
36-36: Animation speed increase looks good but consider synchronization.The logo animation speed increase to 1.5 aligns well with the new animation-driven navigation flow in StartActivity.kt. However, consider whether the background animation (currently at speed 1.0) should also be adjusted to maintain visual harmony between the two animations.
36-36: Consider the user experience impact of faster animation speed.Increasing the logo animation speed from 1.0 to 1.5 will make the splash screen complete 50% faster. While this improves app startup time, consider:
- Visual quality: Ensure the animation still looks smooth at 1.5x speed
- Accessibility: Some users may need more time to process the splash screen visually
- Synchronization: The background animation (line 21) remains at speed 1.0, which may create visual inconsistency
Consider either:
- Synchronizing both animations to the same speed for visual consistency
- Testing the animation at 1.5x speed to ensure it maintains quality and readability
</review_comment_end>app/src/main/java/com/hmh/hamyeonham/StartActivity.kt (6)
29-33: Consider more robust animation completion detection.The animation completion listener implementation is clean and effective. However, consider using a more robust completion check to handle potential floating-point precision issues.
Apply this diff for more reliable completion detection:
binding.splashLottieAppLogo.addAnimatorUpdateListener { - if (it.animatedFraction == 1.0f) { + if (it.animatedFraction >= 1.0f) { navigateToLogin() } }
17-17: Remove unused CoroutineScope.The
coroutineScopeproperty is declared but never used in the current implementation.Apply this diff to clean up unused code:
-private val coroutineScope = CoroutineScope(Dispatchers.Main)Also remove the unused import:
-import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers
17-17: Remove unused CoroutineScope.The
coroutineScopeproperty is declared but not used in the current implementation since the modal dialog logic was removed.-private val coroutineScope = CoroutineScope(Dispatchers.Main)Also remove the unused import:
-import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers
27-34: Consider adding animation failure safety mechanism.While the animation-driven navigation is cleaner than the previous modal dialog approach, consider adding a safety mechanism in case the animation fails to complete (e.g., due to device performance issues or animation file corruption).
You could add a timeout mechanism:
private fun initLottieSplash() { binding.splashLottieAppLogo.playAnimation() var hasNavigated = false binding.splashLottieAppLogo.addAnimatorUpdateListener { animator -> if (animator.animatedFraction >= 0.99f && !hasNavigated) { hasNavigated = true binding.splashLottieAppLogo.removeAllUpdateListeners() navigateToLogin() } } // Safety timeout after 3 seconds binding.splashLottieAppLogo.postDelayed({ if (!hasNavigated) { hasNavigated = true navigateToLogin() } }, 3000) }
29-33: Consider more robust animation completion detection.The animation completion logic is clean, but using exact floating-point comparison (
== 1.0f) can be unreliable due to precision issues. Consider using a threshold-based approach or the animation's end callback.- binding.splashLottieAppLogo.addAnimatorUpdateListener { - if (it.animatedFraction == 1.0f) { - navigateToLogin() - } - } + binding.splashLottieAppLogo.addAnimatorListener(object : android.animation.Animator.AnimatorListener { + override fun onAnimationEnd(animation: android.animation.Animator) { + navigateToLogin() + } + override fun onAnimationStart(animation: android.animation.Animator) {} + override fun onAnimationCancel(animation: android.animation.Animator) {} + override fun onAnimationRepeat(animation: android.animation.Animator) {} + })Alternatively, if you prefer the update listener approach:
- if (it.animatedFraction == 1.0f) { + if (it.animatedFraction >= 0.99f) {
17-17: Remove unused coroutineScope.The
coroutineScopefield appears to be unused after removing the dialog logic. Consider removing it to clean up the code.- private val coroutineScope = CoroutineScope(Dispatchers.Main)Also remove the unused import:
-import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchersdomain/login/src/main/java/com/hmh/hamyeonham/login/model/User.kt (1)
8-18: Resolve the design uncertainty and finalize the User model structure.The TODO comment indicates uncertainty about domain-level user data requirements. The commented code suggests a comprehensive user model with social login integration, synchronization status, and various identifiers. Consider finalizing this design decision to avoid technical debt.
The commented code shows a well-structured user model that includes:
- Social login properties (Kakao integration)
- Synchronization tracking (
connectedAt,synchedAt)- User lifecycle flags (
hasSignedUp)- Multiple identifier types (
id,uuid)Would you like me to help analyze the authentication flow requirements and suggest which properties should be included in the final User model?
data/login/build.gradle.kts (1)
10-18: Review network dependency usage in data/loginOur automated searches across all
.ktand.javafiles underdata/logindid not find any references to thecore.networkmodule. Please manually verify there are no indirect usages (for example, DI bindings or extension functions) before dropping this dependency.• No occurrences of
core.networkin source files
• Noimport …core.networkstatements detectedIf you confirm it’s truly unused, remove the line from
data/login/build.gradle.kts:- implementation(projects.core.network)domain/login/src/main/java/com/hmh/hamyeonham/login/di/AuthProvider.kt (1)
1-7: Enum location: consider moving out ofdipackage.
AuthProvideris a pure domain concept but it lives under adi(dependency-injection) package, which usually only hosts Dagger/Hilt annotations & modules.
Placing the enum under…login.model(or…login.type) keeps package semantics clean and avoids accidental coupling with DI infrastructure.No functional impact, just package hygiene.
-package com.hmh.hamyeonham.login.di +package com.hmh.hamyeonham.login.modelcore/database/src/main/java/com/hmh/hamyeonham/core/database/di/DatabaseModule.kt (2)
5-5: Verify BuildConfig usage.The
BuildConfigimport was added but doesn't appear to be used in the visible code. Consider removing it if not needed.
5-5: Potentially unused import.The
BuildConfigimport appears to be unused in the current code. Consider removing it unless it's needed for conditional database configuration.domain/login/src/main/java/com/hmh/hamyeonham/login/repository/SocialAuthDataStore.kt (1)
9-33: Well-designed abstraction for social authentication providers.The interface provides a clean contract for social login implementations with appropriate async support. The separation of concerns between authentication state and user data is good architectural design.
Consider using a Result pattern instead of nullable return types for better error handling and to distinguish between "no user found" vs "network error" scenarios.
interface SocialAuthDataStore { - suspend fun login(): UserInfo? + suspend fun login(): Result<UserInfo> - suspend fun refreshUserInfo(): UserInfo? + suspend fun refreshUserInfo(): Result<UserInfo> }domain/login/src/main/java/com/hmh/hamyeonham/login/model/UserInfo.kt (1)
6-21: Consider the relationship between UserInfo.id and Profile.userId.The model has both
UserInfo.idandProfile.userIdwhich could be confusing. Clarify the relationship - are these always the same value, or do they serve different purposes?Also consider if
Longtype for IDs is appropriate for all social login providers, as some might use string-based identifiers.data class UserInfo( val id: Long, val profile: Profile? = null ) { data class Profile( - val userId: Long, + // Remove userId if it's always the same as UserInfo.id val nickname: String? = null, val profileImageUrl: String? = null, val email: String? = null, val ageRange: String? = null, val gender: String? = null ) }core/database/src/main/java/com/hmh/hamyeonham/core/database/dao/UserProfileDao.kt (1)
21-22: Consider renaming clearUserProfile() for clarity.The method
clearUserProfile()deletes all user profiles from the table, not just a single user's profile. Consider renaming toclearAllUserProfiles()to make the scope clearer.- @Query("DELETE FROM user_profile") - suspend fun clearUserProfile() + @Query("DELETE FROM user_profile") + suspend fun clearAllUserProfiles()core/database/src/main/java/com/hmh/hamyeonham/core/database/dao/UserAuthDao.kt (1)
12-26: Consider adding @transaction annotation for data consistencyThe DAO operations that modify user authentication state should be wrapped in a transaction to ensure data consistency, especially when called in sequence.
Add the
@Transactionannotation to methods that might be called together:@Dao interface UserAuthDao { @Query("SELECT * FROM user_auth WHERE id = 1") suspend fun getUserAuth(): UserAuth? @Query("SELECT * FROM user_auth WHERE id = 1") fun observeUserAuth(): Flow<UserAuth?> + @Transaction @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun insertOrUpdateUserAuth(userAuth: UserAuth) + @Transaction @Query("UPDATE user_auth SET isLoggedIn = :isLoggedIn WHERE id = 1") suspend fun updateLoginStatus(isLoggedIn: Boolean) + @Transaction @Query("DELETE FROM user_auth") suspend fun clearUserAuth() }domain/login/src/main/java/com/hmh/hamyeonham/login/usecase/AuthUseCase.kt (1)
40-44: Consider implementing proper withdrawal logicThe withdrawal method currently only performs logout. Consider implementing proper user data deletion or at least documenting the expected behavior more clearly.
Would you like me to help implement a proper withdrawal method that clears user data from the database? I can open an issue to track this task.
data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt (1)
102-108: Consider optimizing observeUserProfile with JOIN queryThe current implementation triggers a separate profile query for each auth state change, which could be optimized.
Consider adding a method in UserAuthDao that returns a combined Flow of auth and profile data using a JOIN query. This would reduce the number of database queries and improve performance.
Would you like me to help create an optimized DAO method that combines user auth and profile data in a single query?
feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt (1)
175-182: Remove unnecessary object creation.The
getRequestDomain(state)call on line 178 creates an object that isn't used, which is wasteful.fun signUp() { viewModelScope.launch { - val state = onBoardingState.value - val request = getRequestDomain(state) // TODO 나중에 온보딩 데이터를 쓰고 싶은 경우 + // TODO 나중에 온보딩 데이터를 쓰고 싶은 경우 _onboardEffect.emit(OnboardEffect.OnboardSuccess) AmplitudeUtils.trackEventWithProperties("complete_onboarding_finish") } }data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoSocialAuthDataStoreImpl.kt (1)
52-58: Add logging for debugging failed user info refresh.Similar to the login method, exceptions are swallowed without logging.
override suspend fun refreshUserInfo(): UserInfo? { return try { fetchUserInfo() } catch (e: Exception) { + android.util.Log.e("KakaoAuth", "Failed to refresh user info", e) null } }domain/login/src/main/java/com/hmh/hamyeonham/login/model/LoginInfo.kt (1)
3-3: Consider relocating AuthProvider enum.The
AuthProviderenum is imported from thedipackage, which typically contains dependency injection modules rather than domain models. Consider movingAuthProviderto a more appropriate package likemodelorcommon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
app/src/main/java/com/hmh/hamyeonham/StartActivity.kt(1 hunks)app/src/main/res/layout/activity_sample.xml(1 hunks)core/common/src/main/java/com/hmh/hamyeonham/common/context/ContextExt.kt(1 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt(4 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/dao/UserAuthDao.kt(1 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/dao/UserProfileDao.kt(1 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/di/DaoModule.kt(1 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/di/DatabaseModule.kt(2 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/entity/UserAuth.kt(1 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/entity/UserProfile.kt(1 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/AuthenticatorUtil.kt(2 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/HMHAuthenticator.kt(2 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/DefaultUserPreference.kt(2 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/UserPreference.kt(1 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/HeaderInterceptor.kt(1 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/di/AuthModule.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/di/DataStoreModule.kt(2 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/login/AuthService.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/login/model/LoginRequest.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/login/model/LoginResponse.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/signup/model/SignUpRequest.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/signup/model/SignUpResponse.kt(0 hunks)data/login/build.gradle.kts(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/DefaultAuthRepository.kt(0 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoAuthCodeClient.kt(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoSocialAuthDataStoreImpl.kt(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthBinder.kt(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthDataSourceKey.kt(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthModule.kt(0 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/mapper/LoginMapper.kt(0 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/mapper/UserProfileMapper.kt(1 hunks)data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt(1 hunks)domain/login/build.gradle.kts(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/di/AuthProvider.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/Login.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/LoginInfo.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignUpUser.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/User.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/UserInfo.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/UserProfile.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/repository/AuthRepository.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/repository/SocialAuthDataStore.kt(1 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/usecase/AuthUseCase.kt(1 hunks)feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginActivity.kt(1 hunks)feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt(1 hunks)feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/MyPageFragment.kt(2 hunks)feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt(2 hunks)feature/mypage/src/main/res/values/strings.xml(1 hunks)feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/OnBoardingActivity.kt(1 hunks)feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt(2 hunks)
💤 Files with no reviewable changes (9)
- core/network/src/main/java/com/hmh/hamyeonham/core/network/login/model/LoginResponse.kt
- data/login/src/main/java/com/hmh/hamyeonham/login/mapper/LoginMapper.kt
- data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthModule.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/login/model/LoginRequest.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/di/AuthModule.kt
- data/login/src/main/java/com/hmh/hamyeonham/login/DefaultAuthRepository.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/signup/model/SignUpResponse.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/login/AuthService.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/signup/model/SignUpRequest.kt
👮 Files not reviewed due to content moderation or server errors (2)
- app/src/main/res/layout/activity_sample.xml
- app/src/main/java/com/hmh/hamyeonham/StartActivity.kt
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt (1)
data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt (1)
userAuthDao(16-109)
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginActivity.kt (1)
core/common/src/main/java/com/hmh/hamyeonham/common/context/ContextExt.kt (1)
toast(28-30)
🪛 detekt (1.23.8)
data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoSocialAuthDataStoreImpl.kt
[warning] 31-31: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 55-55: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ 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: PR Checker
🔇 Additional comments (50)
domain/login/src/main/java/com/hmh/hamyeonham/login/model/User.kt (1)
3-5: UnreferencedUsermodel—validate its necessity or intended useThe new
Userdata class in
domain/login/src/main/java/com/hmh/hamyeonham/login/model/User.kt
is currently not imported or referenced anywhere in the codebase. Before expanding its properties:• Confirm whether this
Userclass is meant to drive any authentication or user-related flows (e.g., replace or map toUserProfile/UserInfo).
• Remove the file if it’s a placeholder or obsolete.
• If it should serve as the domain model, integrate it into the use cases and repository layers, and clarify which fields are essential.Please verify its intended role and update or remove accordingly.
app/src/main/res/layout/activity_sample.xml (1)
36-36: Animation speed increase aligns with new navigation logic.The increased speed from 1.0 to 1.5 for the logo animation is appropriate given that
StartActivity.ktnow uses animation completion as the trigger for navigation. This will provide a more responsive user experience.app/src/main/java/com/hmh/hamyeonham/StartActivity.kt (1)
36-39: LGTM! Clean navigation implementation.The navigation logic properly transitions to the login activity and finishes the splash screen, preventing unwanted back navigation.
domain/login/build.gradle.kts (1)
7-10: Confirm the variant of the coroutine artifact being pulled in.
domain/loginis a plain JVM/Kotlin module (java-library+kotlin("jvm")).
Double-check thatlibs.kotlin.coroutinesresolves toorg.jetbrains.kotlinx:kotlinx-coroutines-core(not the Android classifier) to avoid leaking Android-specific transitive deps into a pure JVM module.If the version catalog entry currently points to
…:kotlinx-coroutines-android, add an explicit core binary for this module:-implementation(libs.kotlin.coroutines) +implementation(libs.kotlin.coroutines.core)domain/login/src/main/java/com/hmh/hamyeonham/login/model/Login.kt (1)
3-7: DB migration risk after switchinguserIdtoLong.Changing the type from
Int→Longis fine at the domain layer, but it requires:
- A Room schema migration for every table/column that persisted the old
Inttype.- Safe casting in mapping layers that still receive an
Intfrom legacy sources (cached JSON, Preference values, etc.).Please verify that
Migration(1, 2)(or newer) alters column types correctly and that no old serialization code forces the value back toInt.core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/UserPreference.kt (2)
3-3: LGTM! Interface rename improves clarity.The renaming from
HMHNetworkPreferencetoUserPreferenceis more descriptive and aligns well with the new user-centric authentication architecture.
7-7: Type update verification passedAll references to
userIdhave been confirmed to useLongconsistently—no remainingIntassumptions were found.• LocalAuthRepository (data/login/.../LocalAuthRepository.kt) passes
userAuth.userId(Long) to DAO
• UserProfileDao signature (core/database/.../UserProfileDao.kt) definesgetUserProfile(userId: Long)
• UserProfileMapper (data/login/.../UserProfileMapper.kt) mapsuserIdfields as LongNo further changes required.
core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/HMHAuthenticator.kt (1)
4-4: LGTM! Consistent interface update.The import and constructor parameter changes from
HMHNetworkPreferencetoUserPreferenceare correct and maintain the same functionality while aligning with the new authentication architecture.Also applies to: 19-19
core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/AuthenticatorUtil.kt (1)
6-6: LGTM! Interface update maintains functionality.The changes from
HMHNetworkPreferencetoUserPreferenceare correctly applied. The logout logic remains intact and properly handles clearing authentication data, database cleanup, and Kakao logout.Also applies to: 21-21
data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthDataSourceKey.kt (1)
1-7: AuthProvider enum verified; AuthDataSourceKey is correctly implemented
TheAuthProviderenum is defined indomain/login/src/main/java/com/hmh/hamyeonham/login/di/AuthProvider.ktwithKAKAO,@MapKeyannotation with@Retention(AnnotationRetention.BINARY)is correctly set up for Dagger multibinding. No further changes needed.feature/mypage/src/main/res/values/strings.xml (1)
29-36: LGTM! Comprehensive string resources for profile features.The new string resources properly support the enhanced profile and authentication features. The naming convention is consistent, and the Korean text covers all necessary UI states including login status, profile fields, and user messaging.
core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/HeaderInterceptor.kt (2)
3-3: LGTM! Clean interface refactoring.The update from
HMHNetworkPreferencetoUserPreferenceis consistent with the broader authentication refactoring. The functionality remains intact while aligning with the new user preference abstraction.Also applies to: 10-10
3-3: LGTM! Clean interface rename.The import and constructor parameter updates from
HMHNetworkPreferencetoUserPreferenceare consistent with the broader authentication refactoring. The core functionality remains unchanged.Also applies to: 10-10
domain/login/src/main/java/com/hmh/hamyeonham/login/model/LoginInfo.kt (2)
8-13: LGTM! Well-structured login state model.The
LoginInfodata class provides a comprehensive representation of user authentication state with appropriate properties for userId, provider, login status, and timestamp tracking.
5-13: Well-structured login information model.The
LoginInfodata class effectively encapsulates user authentication state with appropriate property types. TheuserIdasLongaligns with the broader type changes mentioned in the refactoring.feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/MyPageFragment.kt (4)
79-79: LGTM! Simplified ViewModel API.The method names have been simplified from
handleLogout()/handleWithdrawal()tologout()/withdrawal(), which provides a cleaner and more direct API.Also applies to: 92-92
99-106: LGTM! Improved effect handling structure.The consolidation from
viewModel.userEffecttoviewModel.effectand the use ofeffect.messagefor error handling provides better consistency and error messaging capabilities.
79-79: LGTM! Simplified method names.The method calls have been appropriately updated to use the simplified names
logout()andwithdrawal()instead ofhandleLogout()andhandleWithdrawal(), aligning with the ViewModel refactoring.Also applies to: 92-92
99-105: Improved effect handling with dynamic error messages.The effect flow has been properly renamed to
effect, and the error handling now uses dynamicmessageproperties from the effect objects, providing better flexibility for error reporting.core/database/src/main/java/com/hmh/hamyeonham/core/database/di/DatabaseModule.kt (2)
21-29: LGTM! Proper database migration setup.The addition of
MIGRATION_1_2is essential for handling the schema changes when upgrading from version 1 to 2. The migration ensures existing users can upgrade without data loss.
22-29: Essential migration handling for schema changes.The addition of
MIGRATION_1_2is necessary to handle the database schema update that introduces the newUserAuthandUserProfileentities. This ensures existing user data is preserved during the upgrade.core/database/src/main/java/com/hmh/hamyeonham/core/database/di/DaoModule.kt (2)
43-53: LGTM! Consistent DAO provider implementation.The new DAO providers for
UserAuthDaoandUserProfileDaofollow the established pattern and are properly configured for dependency injection. They support the new authentication entities introduced in the database migration.
43-53: Well-structured DAO providers for new authentication entities.The new
UserAuthDaoandUserProfileDaoproviders follow the established pattern and correctly integrate with the dependency injection system. These are essential for the local authentication functionality.domain/login/src/main/java/com/hmh/hamyeonham/login/model/UserProfile.kt (1)
1-13: LGTM! Clean domain model design.The
UserProfiledomain model is well-structured with appropriate nullable fields and clear documentation. The design aligns well with the local authentication architecture changes described in the PR objectives.core/network/src/main/java/com/hmh/hamyeonham/core/network/di/DataStoreModule.kt (2)
8-9: LGTM! Consistent import updates.The import statements have been correctly updated to use the new
DefaultUserPreferenceandUserPreferenceclasses, aligning with the broader refactoring of authentication preferences.
74-74: LGTM! Dependency injection binding updated correctly.The binding has been properly updated to bind
DefaultUserPreferencetoUserPreference, maintaining consistency with the import changes and the overall authentication refactoring.core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/DefaultUserPreference.kt (1)
9-11: LGTM! Class rename aligns with architectural changes.The class has been appropriately renamed to
DefaultUserPreferenceand implements the updatedUserPreferenceinterface, consistent with the authentication system refactoring.data/login/src/main/java/com/hmh/hamyeonham/login/di/AuthBinder.kt (1)
14-29: Excellent DI module design for extensible social authentication.The use of multibinding with
@AuthDataSourceKeyandAuthProviderenum creates a clean, extensible pattern for adding multiple social login providers. The singleton scoping is appropriate for authentication components.core/database/src/main/java/com/hmh/hamyeonham/core/database/dao/UserProfileDao.kt (1)
10-19: Well-designed DAO with both reactive and non-reactive access patterns.The DAO provides good coverage of CRUD operations with appropriate Room annotations. The combination of suspend functions and Flow-based observation supports different usage patterns well.
data/login/src/main/java/com/hmh/hamyeonham/login/mapper/UserProfileMapper.kt (2)
3-6: Good use of type alias to distinguish between similar UserProfile types.The type alias
import com.hmh.hamyeonham.core.database.entity.UserProfile as DbUserProfileeffectively prevents confusion between the database entity and domain model with the same name.
10-47: Clean and straightforward mapping functions.The extension functions provide clear conversion paths between different UserProfile representations. The 1:1 field mappings are appropriate for these data transformation operations.
feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/OnBoardingActivity.kt (1)
117-146: Formatting improvements enhance readabilityThe JSON object construction for Amplitude tracking is now more readable with better line breaks and consistent formatting.
core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt (1)
66-98: Migration SQL is correct and handles schema upgrade properlyThe migration correctly creates the new tables with appropriate column types and primary keys. The SQL ensures backward compatibility.
domain/login/src/main/java/com/hmh/hamyeonham/login/usecase/AuthUseCase.kt (1)
11-73: Well-structured use case following clean architecture principlesThe AuthUseCase properly encapsulates authentication operations and provides a clean interface for the presentation layer. The use of Result type for error handling and Flow for reactive streams is appropriate.
data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt (1)
16-109: Solid implementation of local authentication repositoryThe repository properly implements the authentication interface with good error handling, reactive streams support, and clean separation of concerns. The integration with social auth data stores is well-designed.
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginActivity.kt (2)
37-37: LGTM! Simplified login method call.The removal of context parameter aligns with the new authentication architecture where the ViewModel handles the login flow internally.
40-41: Well-structured reactive login handling.The new approach using separate methods for state observation (
checkLoginState()) and effect handling (handleLoginEffect()) provides clear separation of concerns. The error handling covers both failure scenarios with appropriate user feedback via toast messages.Also applies to: 44-60
feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt (1)
71-71: LGTM! Simplified constructor aligns with new architecture.Removing the AuthRepository and HMHNetworkPreference dependencies is consistent with the shift to local authentication management.
feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt (2)
5-5: Excellent architectural improvement.Replacing multiple dependencies with a single
AuthUseCaseprovides better abstraction and follows the clean architecture principle of depending on use cases rather than repositories directly.Also applies to: 21-21
27-49: Well-implemented Result pattern with proper error handling.Both
logout()andwithdrawal()methods demonstrate consistent error handling with user-friendly fallback messages. The use of the Result pattern makes the success/failure flows explicit and easier to handle.data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoSocialAuthDataStoreImpl.kt (1)
60-85: Verify login success/failure handling.Both
loginWithKakaoTalk()andloginWithAccount()always resume withUnitregardless of success or failure. This means thelogin()method cannot distinguish between successful and failed login attempts when deciding whether to fetch user info.The current flow assumes login always succeeds and attempts to fetch user info. Consider modifying these methods to return boolean success indicators or throw exceptions on failure.
domain/login/src/main/java/com/hmh/hamyeonham/login/repository/AuthRepository.kt (1)
8-51: Excellent interface design with comprehensive documentation.This refactored
AuthRepositoryinterface demonstrates several architectural best practices:
- Consistent error handling: All suspend functions return
Result<T>for predictable error handling- Reactive programming: Flow-based observation methods enable reactive UI updates
- Provider abstraction:
AuthProviderparameter allows supporting multiple authentication providers- Comprehensive documentation: Each method has clear KDoc explaining purpose, parameters, and return values
- Clean separation: Distinguishes between one-time operations and continuous state observation
The interface provides a solid foundation for the new authentication architecture.
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt (3)
6-7: Good architectural improvement with AuthUseCase abstractionThe refactoring to use
AuthUseCaseinstead of direct repository dependencies improves the separation of concerns and makes the ViewModel more testable.Also applies to: 31-31
26-26: Proper state management with loading flag and effect flowGood addition of the
isLoadingflag for better UI state management and proper encapsulation of the effect flow.Also applies to: 36-37
50-56: Clean provider-specific login methodsGood implementation of provider-specific public methods that delegate to the common private implementation.
data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoAuthCodeClient.kt (5)
28-36: Verify the source and licensing of this implementationThe comment indicates this is based on Kakao SDK's internal
AuthCodeClient. Please ensure:
- This implementation complies with Kakao SDK's license terms
- Consider using the official SDK methods if available instead of reimplementing internal classes
40-103: Well-implemented KakaoTalk authorization methodThe method properly handles the case when KakaoTalk is not installed and includes comprehensive OAuth parameters with PKCE support.
157-179: Robust OAuth callback handlingGood implementation of the result receiver with proper error parsing and appropriate use of HTTP 302 status for OAuth redirect errors.
187-199: Correct PKCE implementation following OAuth 2.0 standardsThe code verifier and challenge generation properly implements PKCE with S256 method, using appropriate Base64 encoding flags for URL safety.
189-190: Verify Kakao SDK algorithm constantsWe weren’t able to locate the definitions for
Constants.CODE_VERIFIER_ALGORITHMorConstants.CODE_CHALLENGE_ALGORITHMin the local codebase (they’re provided by the Kakao SDK). Please confirm in the SDK’sConstantsclass (e.g., in its source or JAR):
CODE_VERIFIER_ALGORITHMis defined as"SHA-256"CODE_CHALLENGE_ALGORITHMmatches the required PKCE algorithm (typically"S256"or the same"SHA-256"identifier)No code changes are needed here if those values are correct. If they differ, consider overriding or defining the expected constants locally to ensure the PKCE flow uses the proper hash algorithm.
| binding.splashLottieAppLogo.addAnimatorUpdateListener { | ||
| if (it.animatedFraction == 1.0f) { | ||
| navigateToLogin() | ||
| } | ||
| .showAllowingStateLoss(supportFragmentManager) | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve animation completion detection and cleanup.
The current implementation has a few potential issues:
-
Floating point precision: Using exact equality (
== 1.0f) for floating point comparison can be unreliable. Animation fraction might not reach exactly 1.0f due to precision issues. -
Missing cleanup: The animator listener is not removed, which could potentially cause memory leaks.
Apply this diff to improve the implementation:
private fun initLottieSplash() {
binding.splashLottieAppLogo.playAnimation()
- binding.splashLottieAppLogo.addAnimatorUpdateListener {
- if (it.animatedFraction == 1.0f) {
+ binding.splashLottieAppLogo.addAnimatorUpdateListener { animator ->
+ if (animator.animatedFraction >= 0.99f) {
+ binding.splashLottieAppLogo.removeAllUpdateListeners()
navigateToLogin()
}
}
}📝 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.
| binding.splashLottieAppLogo.addAnimatorUpdateListener { | |
| if (it.animatedFraction == 1.0f) { | |
| navigateToLogin() | |
| } | |
| .showAllowingStateLoss(supportFragmentManager) | |
| } | |
| private fun initLottieSplash() { | |
| binding.splashLottieAppLogo.playAnimation() | |
| binding.splashLottieAppLogo.addAnimatorUpdateListener { animator -> | |
| if (animator.animatedFraction >= 0.99f) { | |
| binding.splashLottieAppLogo.removeAllUpdateListeners() | |
| navigateToLogin() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/hmh/hamyeonham/StartActivity.kt around lines 29 to 33,
replace the exact floating point equality check on animatedFraction with a
threshold comparison (e.g., check if animatedFraction is greater than or equal
to 0.99f) to handle precision issues. Additionally, after navigating to login,
remove the animator update listener to prevent potential memory leaks by
unregistering the listener from the animation.
| @Entity(tableName = "user_auth") | ||
| data class UserAuth( | ||
| @PrimaryKey | ||
| val id: Int = 1, // 항상 같은 ID 사용 (싱글 인스턴스) | ||
| val userId: Long = -1, | ||
| val providerType: String = "", // "KAKAO", "GOOGLE" 등 AuthProvider의 name | ||
| val isLoggedIn: Boolean = false, | ||
| val lastLoginTimestamp: Long = 0 | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternatives to the singleton pattern and improve type safety.
The entity design has a few areas for improvement:
-
Singleton with fixed ID: Using
id: Int = 1to enforce a single instance is unconventional for Room. Consider using a repository-level singleton pattern instead. -
String-based provider type: The
providerTypefield would benefit from an enum for better type safety:
+enum class AuthProvider {
+ KAKAO, GOOGLE, NONE;
+
+ companion object {
+ fun fromString(value: String): AuthProvider =
+ values().find { it.name == value } ?: NONE
+ }
+}
data class UserAuth(
@PrimaryKey
val id: Int = 1,
val userId: Long = -1,
- val providerType: String = "", // "KAKAO", "GOOGLE" 등 AuthProvider의 name
+ val providerType: String = AuthProvider.NONE.name,
val isLoggedIn: Boolean = false,
val lastLoginTimestamp: Long = 0
)🤖 Prompt for AI Agents
In
core/database/src/main/java/com/hmh/hamyeonham/core/database/entity/UserAuth.kt
around lines 10 to 18, avoid using a fixed ID value to enforce a singleton in
the Room entity; instead, remove the default fixed ID and handle singleton
behavior at the repository or service layer. Also, replace the String type of
the providerType field with an enum class representing the authentication
providers to improve type safety and prevent invalid values.
| @Entity(tableName = "user_profile") | ||
| data class UserProfile( | ||
| @PrimaryKey | ||
| val userId: Long, | ||
| val nickname: String? = null, | ||
| val profileImageUrl: String? = null, | ||
| val email: String? = null, | ||
| val ageRange: String? = null, | ||
| val gender: String? = null, | ||
| val updatedAt: Long = System.currentTimeMillis() | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving timestamp generation to repository layer.
The entity design is mostly well-structured, but there's one concern with the default timestamp:
data class UserProfile(
@PrimaryKey
val userId: Long,
val nickname: String? = null,
val profileImageUrl: String? = null,
val email: String? = null,
val ageRange: String? = null,
val gender: String? = null,
- val updatedAt: Long = System.currentTimeMillis()
+ val updatedAt: Long
)Rationale: Using System.currentTimeMillis() as a default value in the entity can cause issues with:
- Unit testing (timestamps become unpredictable)
- Data consistency (multiple instances created at different times)
- Database operations (updates vs inserts might get different timestamps)
Consider generating the timestamp in the repository or DAO layer where you have better control over when it's set.
📝 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.
| @Entity(tableName = "user_profile") | |
| data class UserProfile( | |
| @PrimaryKey | |
| val userId: Long, | |
| val nickname: String? = null, | |
| val profileImageUrl: String? = null, | |
| val email: String? = null, | |
| val ageRange: String? = null, | |
| val gender: String? = null, | |
| val updatedAt: Long = System.currentTimeMillis() | |
| ) | |
| @Entity(tableName = "user_profile") | |
| data class UserProfile( | |
| @PrimaryKey | |
| val userId: Long, | |
| val nickname: String? = null, | |
| val profileImageUrl: String? = null, | |
| val email: String? = null, | |
| val ageRange: String? = null, | |
| val gender: String? = null, | |
| val updatedAt: Long | |
| ) |
🤖 Prompt for AI Agents
In
core/database/src/main/java/com/hmh/hamyeonham/core/database/entity/UserProfile.kt
lines 9 to 19, the default timestamp for updatedAt is set using
System.currentTimeMillis() in the entity, which can cause issues with testing
and data consistency. Remove the default value from the entity and instead set
the updatedAt timestamp explicitly in the repository or DAO layer when creating
or updating UserProfile instances to ensure better control and predictability.
| override var userId: Long | ||
| get() = preferences.getLong("user_id", -1) | ||
| set(value) { | ||
| preferences.edit(commit = true) { | ||
| putInt("user_id", value) | ||
| putLong("user_id", value) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify migration impact for existing users.
The userId type change from Int to Long is significant and requires careful consideration for existing users who may have Int values stored in SharedPreferences.
🏁 Script executed:
#!/bin/bash
# Description: Search for any migration or compatibility handling for the userId type change
# Expected: Find migration logic or compatibility handling for existing Int userId values
echo "Searching for userId migration or compatibility handling..."
rg -A 5 -B 5 "userId.*migration|migration.*userId|user_id.*int|int.*user_id" --type kt
echo "Searching for any SharedPreferences migration patterns..."
rg -A 10 "SharedPreferences.*migration|migration.*SharedPreferences" --type kt
echo "Checking for userId type usage patterns..."
rg "userId.*Int|Int.*userId" --type ktLength of output: 640
🏁 Script executed:
#!/bin/bash
# Verify if any legacy Int-based API for “user_id” still exists or if migration code is present
echo "🔍 Searching for legacy getInt usage for 'user_id'..."
rg -n 'getInt\("user_id"' -g '*.kt'
echo "🔍 Searching for legacy putInt usage for 'user_id'..."
rg -n 'putInt\("user_id"' -g '*.kt'
echo "🔍 Searching for migration/convert patterns in Kotlin sources..."
rg -n -E 'migrate|migration|convert|upgrade' -g '*.kt'Length of output: 914
Implement migration for legacy Int-based userId in SharedPreferences
No compatibility or migration logic was found for existing users who have user_id stored as an Int. Calling getLong("user_id") on an Int-backed preference will throw a ClassCastException at runtime. Please add fallback handling to read the old Int value and re-save it as a Long.
• File: core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/DefaultUserPreference.kt, lines 33–39
• Add a try/catch around getLong (or migrate on initialization) to preserve existing data
Suggested diff:
override var userId: Long
- get() = preferences.getLong("user_id", -1)
+ get() = try {
+ preferences.getLong("user_id", -1)
+ } catch (e: ClassCastException) {
+ // Migrate legacy Int value to Long
+ val intValue = preferences.getInt("user_id", -1)
+ preferences.edit(commit = true) {
+ putLong("user_id", intValue.toLong())
+ }
+ intValue.toLong()
+ }
set(value) {
preferences.edit(commit = true) {
putLong("user_id", value)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override var userId: Long | |
| get() = preferences.getLong("user_id", -1) | |
| set(value) { | |
| preferences.edit(commit = true) { | |
| putInt("user_id", value) | |
| putLong("user_id", value) | |
| } | |
| } | |
| override var userId: Long | |
| get() = try { | |
| preferences.getLong("user_id", -1) | |
| } catch (e: ClassCastException) { | |
| // Migrate legacy Int value to Long | |
| val intValue = preferences.getInt("user_id", -1) | |
| preferences.edit(commit = true) { | |
| putLong("user_id", intValue.toLong()) | |
| } | |
| intValue.toLong() | |
| } | |
| set(value) { | |
| preferences.edit(commit = true) { | |
| putLong("user_id", value) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/datastore/network/DefaultUserPreference.kt
lines 33-39, the userId getter directly calls getLong on preferences which can
throw ClassCastException if the stored user_id is an Int. To fix this, wrap the
getLong call in a try/catch block; in the catch, read the user_id as an Int,
convert it to Long, save it back as Long, and return the Long value. This
migration ensures backward compatibility with legacy Int-based user_id values.
| override suspend fun login(): UserInfo? { | ||
| return try { | ||
| // 카카오톡이 설치되어 있으면 카카오톡으로 로그인, 아니면 계정으로 로그인 | ||
| if (UserApiClient.instance.isKakaoTalkLoginAvailable(context)) { | ||
| loginWithKakaoTalk() | ||
| } else { | ||
| loginWithAccount() | ||
| } | ||
|
|
||
| // 로그인 성공 후 사용자 정보 조회 | ||
| fetchUserInfo() | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging.
The current implementation swallows exceptions without logging, making debugging difficult. The login flow should also handle and propagate errors properly.
override suspend fun login(): UserInfo? {
return try {
// 카카오톡이 설치되어 있으면 카카오톡으로 로그인, 아니면 계정으로 로그인
if (UserApiClient.instance.isKakaoTalkLoginAvailable(context)) {
loginWithKakaoTalk()
} else {
loginWithAccount()
}
// 로그인 성공 후 사용자 정보 조회
fetchUserInfo()
} catch (e: Exception) {
+ // Log the exception for debugging
+ android.util.Log.e("KakaoAuth", "Login failed", e)
null
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun login(): UserInfo? { | |
| return try { | |
| // 카카오톡이 설치되어 있으면 카카오톡으로 로그인, 아니면 계정으로 로그인 | |
| if (UserApiClient.instance.isKakaoTalkLoginAvailable(context)) { | |
| loginWithKakaoTalk() | |
| } else { | |
| loginWithAccount() | |
| } | |
| // 로그인 성공 후 사용자 정보 조회 | |
| fetchUserInfo() | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| override suspend fun login(): UserInfo? { | |
| return try { | |
| // 카카오톡이 설치되어 있으면 카카오톡으로 로그인, 아니면 계정으로 로그인 | |
| if (UserApiClient.instance.isKakaoTalkLoginAvailable(context)) { | |
| loginWithKakaoTalk() | |
| } else { | |
| loginWithAccount() | |
| } | |
| // 로그인 성공 후 사용자 정보 조회 | |
| fetchUserInfo() | |
| } catch (e: Exception) { | |
| // Log the exception for debugging | |
| android.util.Log.e("KakaoAuth", "Login failed", e) | |
| null | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 31-31: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In
data/login/src/main/java/com/hmh/hamyeonham/login/datasource/kakao/KakaoSocialAuthDataStoreImpl.kt
around lines 20 to 34, the login function catches exceptions but does not log
them, which hinders debugging and error tracking. Modify the catch block to log
the exception details using an appropriate logger before returning null, and
consider rethrowing or propagating the exception if needed to allow higher-level
handling of login errors.
| override suspend fun logout(): Result<Unit> = runCatching { | ||
| val userAuth = userAuthDao.getUserAuth() ?: return@runCatching Unit // 이미 로그아웃 상태 | ||
|
|
||
| // 현재 로그인된 제공자 확인 | ||
| val provider = AuthProvider.valueOf(userAuth.providerType) | ||
| val dataStore = socialAuthDataStores[provider] | ||
| ?: throw IllegalArgumentException("지원하지 않는 소셜 로그인 제공자입니다: $provider") | ||
|
|
||
| // SDK를 사용한 소셜 로그아웃 처리 | ||
| dataStore.logout() | ||
|
|
||
| // DB에서 로그인 상태 업데이트 | ||
| userAuthDao.updateLoginStatus(false) | ||
| } |
There was a problem hiding this comment.
Handle invalid provider type in logout
Similar to the getCurrentAuth method, the AuthProvider.valueOf() call here can also throw an exception.
Add error handling for invalid provider types:
- val provider = AuthProvider.valueOf(userAuth.providerType)
+ val provider = try {
+ AuthProvider.valueOf(userAuth.providerType)
+ } catch (e: IllegalArgumentException) {
+ // Log the error and clear the invalid auth data
+ userAuthDao.clearUserAuth()
+ return@runCatching Unit
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun logout(): Result<Unit> = runCatching { | |
| val userAuth = userAuthDao.getUserAuth() ?: return@runCatching Unit // 이미 로그아웃 상태 | |
| // 현재 로그인된 제공자 확인 | |
| val provider = AuthProvider.valueOf(userAuth.providerType) | |
| val dataStore = socialAuthDataStores[provider] | |
| ?: throw IllegalArgumentException("지원하지 않는 소셜 로그인 제공자입니다: $provider") | |
| // SDK를 사용한 소셜 로그아웃 처리 | |
| dataStore.logout() | |
| // DB에서 로그인 상태 업데이트 | |
| userAuthDao.updateLoginStatus(false) | |
| } | |
| override suspend fun logout(): Result<Unit> = runCatching { | |
| val userAuth = userAuthDao.getUserAuth() ?: return@runCatching Unit // 이미 로그아웃 상태 | |
| // 현재 로그인된 제공자 확인 | |
| val provider = try { | |
| AuthProvider.valueOf(userAuth.providerType) | |
| } catch (e: IllegalArgumentException) { | |
| // Log the error and clear the invalid auth data | |
| userAuthDao.clearUserAuth() | |
| return@runCatching Unit | |
| } | |
| val dataStore = socialAuthDataStores[provider] | |
| ?: throw IllegalArgumentException("지원하지 않는 소셜 로그인 제공자입니다: $provider") | |
| // SDK를 사용한 소셜 로그아웃 처리 | |
| dataStore.logout() | |
| // DB에서 로그인 상태 업데이트 | |
| userAuthDao.updateLoginStatus(false) | |
| } |
🤖 Prompt for AI Agents
In
data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt
around lines 68 to 81, the logout method uses AuthProvider.valueOf() without
handling invalid provider types, which can throw an exception. Wrap the valueOf
call in a try-catch block or use a safe method to handle invalid provider
strings gracefully, and return a failure Result or handle the error
appropriately to prevent crashes.
| @@ -0,0 +1,13 @@ | |||
| package com.hmh.hamyeonham.login.model | |||
|
|
|||
| import com.hmh.hamyeonham.login.di.AuthProvider | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify AuthProvider import location.
The AuthProvider enum is imported from the DI package (com.hmh.hamyeonham.login.di.AuthProvider). Consider whether this enum should be in a domain/model package instead of the DI package for better separation of concerns.
🏁 Script executed:
#!/bin/bash
# Description: Check where AuthProvider is defined and how it's used across the codebase
# Expected: AuthProvider should ideally be in a domain/model package, not DI
# Find the AuthProvider definition
fd "AuthProvider.kt" --exec cat {}
# Check other usages of AuthProvider to see if they also import from DI
rg "import.*AuthProvider" -A 2Length of output: 1991
Refactor AuthProvider into the domain model package
The AuthProvider enum currently lives in the DI package (com.hmh.hamyeonham.login.di), but it represents a domain concept used by multiple layers. To enforce clear separation of concerns, move it to the domain/model package and update all imports accordingly.
Files requiring import updates:
- feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt
- data/login/src/main/java/com/hmh/hamyeonham/login/repository/LocalAuthRepository.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/usecase/AuthUseCase.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/model/LoginInfo.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/repository/AuthRepository.kt
Example diff in AuthProvider.kt:
- package com.hmh.hamyeonham.login.di
+ package com.hmh.hamyeonham.login.model
enum class AuthProvider {
KAKAO,
GOOGLE,
EMAIL,
}And in each consumer:
- import com.hmh.hamyeonham.login.di.AuthProvider
+ import com.hmh.hamyeonham.login.model.AuthProvider📝 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.
| import com.hmh.hamyeonham.login.di.AuthProvider | |
| -import com.hmh.hamyeonham.login.di.AuthProvider | |
| +import com.hmh.hamyeonham.login.model.AuthProvider |
🤖 Prompt for AI Agents
In domain/login/src/main/java/com/hmh/hamyeonham/login/model/LoginInfo.kt at
line 3 and similarly in all listed files, move the AuthProvider enum from the di
package to the domain/model package to better represent its domain nature.
Update all import statements referencing AuthProvider to point to the new
location in the domain/model package. Ensure that all files consuming
AuthProvider reflect this change to maintain consistency and proper separation
of concerns.
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
| viewModelScope.launch { | ||
| authUseCase.isLoggedIn().onSuccess { isLoggedIn -> | ||
| _loginState.value = loginState.value.copy( | ||
| autoLogin = isLoggedIn, | ||
| isLoading = false | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential failure case in login status check
The init block only handles the success case. Consider handling potential failures and clarify the isLoading state management:
- The
isLoadingflag starts asfalseand is set tofalseagain - consider if initial loading state should betrue - Add error handling for the failure case
init {
viewModelScope.launch {
+ _loginState.value = loginState.value.copy(isLoading = true)
authUseCase.isLoggedIn()
.onSuccess { isLoggedIn ->
_loginState.value = loginState.value.copy(
autoLogin = isLoggedIn,
isLoading = false
)
}
+ .onFailure {
+ _loginState.value = loginState.value.copy(
+ autoLogin = false,
+ isLoading = false
+ )
+ }
}
}📝 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.
| viewModelScope.launch { | |
| authUseCase.isLoggedIn().onSuccess { isLoggedIn -> | |
| _loginState.value = loginState.value.copy( | |
| autoLogin = isLoggedIn, | |
| isLoading = false | |
| ) | |
| } | |
| } | |
| init { | |
| viewModelScope.launch { | |
| // show loading while we check login status | |
| _loginState.value = loginState.value.copy(isLoading = true) | |
| authUseCase.isLoggedIn() | |
| .onSuccess { isLoggedIn -> | |
| _loginState.value = loginState.value.copy( | |
| autoLogin = isLoggedIn, | |
| isLoading = false | |
| ) | |
| } | |
| .onFailure { | |
| _loginState.value = loginState.value.copy( | |
| autoLogin = false, | |
| isLoading = false | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt
around lines 40 to 47, the coroutine handling login status only processes the
success case and sets isLoading to false without considering the initial loading
state or failure scenarios. Update the code to set isLoading to true before
starting the check, handle the failure case by updating the login state
accordingly (e.g., setting an error flag or message), and ensure isLoading is
set to false after either success or failure to properly reflect the loading
state.
| private fun loginWith(provider: AuthProvider) { | ||
| viewModelScope.launch { | ||
| authUseCase.login(provider) | ||
| .onSuccess { | ||
| AmplitudeUtils.trackEventWithProperties("click_onboarding_kakao") | ||
| _effect.emit(LoginEffect.LoginSuccess) | ||
| } | ||
| .onFailure { exception -> | ||
| _effect.emit(LoginEffect.LoginError(exception.message ?: "로그인에 실패했습니다")) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect Amplitude event tracking for different providers
The Amplitude event is hardcoded to "click_onboarding_kakao" but this method handles both Kakao and Google logins. The event should be provider-specific.
private fun loginWith(provider: AuthProvider) {
viewModelScope.launch {
authUseCase.login(provider)
.onSuccess {
- AmplitudeUtils.trackEventWithProperties("click_onboarding_kakao")
+ val eventName = when (provider) {
+ AuthProvider.KAKAO -> "click_onboarding_kakao"
+ AuthProvider.GOOGLE -> "click_onboarding_google"
+ }
+ AmplitudeUtils.trackEventWithProperties(eventName)
_effect.emit(LoginEffect.LoginSuccess)
}
.onFailure { exception ->
_effect.emit(LoginEffect.LoginError(exception.message ?: "로그인에 실패했습니다"))
}
}
}📝 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.
| private fun loginWith(provider: AuthProvider) { | |
| viewModelScope.launch { | |
| authUseCase.login(provider) | |
| .onSuccess { | |
| AmplitudeUtils.trackEventWithProperties("click_onboarding_kakao") | |
| _effect.emit(LoginEffect.LoginSuccess) | |
| } | |
| .onFailure { exception -> | |
| _effect.emit(LoginEffect.LoginError(exception.message ?: "로그인에 실패했습니다")) | |
| } | |
| } | |
| } | |
| } | |
| private fun loginWith(provider: AuthProvider) { | |
| viewModelScope.launch { | |
| authUseCase.login(provider) | |
| .onSuccess { | |
| val eventName = when (provider) { | |
| AuthProvider.KAKAO -> "click_onboarding_kakao" | |
| AuthProvider.GOOGLE -> "click_onboarding_google" | |
| } | |
| AmplitudeUtils.trackEventWithProperties(eventName) | |
| _effect.emit(LoginEffect.LoginSuccess) | |
| } | |
| .onFailure { exception -> | |
| _effect.emit(LoginEffect.LoginError(exception.message ?: "로그인에 실패했습니다")) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt
between lines 58 and 69, the Amplitude event tracking is hardcoded to
"click_onboarding_kakao" regardless of the login provider. Modify the code to
dynamically set the event name based on the provider argument, for example using
a conditional or mapping to emit "click_onboarding_kakao" for Kakao and a
corresponding event for Google, ensuring the event accurately reflects the login
provider used.
Feat/remove_kakao_login: 카카오 로그인 삭제
Feat/remove server point
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionActivity.kt (2)
33-33: Missing permission-check methods will break compilationThe calls to
hasUsageStatsPermission()andhasOverlayPermission()inPermissionActivity.kthave no corresponding definitions in the codebase—this will cause build failures. You need to either restore/implement these methods (e.g. as class members orContextextensions) or remove/update their usages.Affected locations in core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionActivity.kt:
- Line 33:
if (hasOverlayPermission())- Line 42: subsequent call
- Line 72
- Line 80
- Lines 102–103
Please add the missing method implementations or refactor these calls accordingly.
33-33: Missing Permission Check Methods in PermissionActivityIt looks like
hasOverlayPermission()andhasUsageStatsPermission()are called inPermissionActivity.ktbut aren’t defined there (nor as extensions). Without these methods in scope, the class won’t compile.Please add or import implementations for these two helpers. For example, you can:
- Define them in
PermissionActivity(mirroring the logic fromRequestPermissionFragment)- Or move them into a shared utility/extension file and import that
References in
PermissionActivity.ktto update:
- Line 33:
if (hasOverlayPermission())- Line 42: similar overlay check
- Line 72: usage-stats check
- Line 80: another usage-stats call
- Lines 102–103: final permission checks
Ensure both methods return the correct Boolean based on Android’s permission APIs.
core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionExt.kt (2)
127-137: Inconsistency: Fragment version still uses old approach.The
Fragment.hasUsageStatsPermission()method still uses the oldUsageStatsManager.queryUsageStats()approach whileAppCompatActivity.hasUsageStatsPermission()was updated to useAppOpsManager. This creates inconsistent behavior between Activity and Fragment contexts.Apply this diff to align the Fragment version with the improved AppCompatActivity implementation:
fun Fragment.hasUsageStatsPermission(): Boolean { - val usageStatsManager = - requireContext().getSystemService(Context.USAGE_STATS_SERVICE) as UsageStatsManager - val time = System.currentTimeMillis() - val stats = usageStatsManager.queryUsageStats( - UsageStatsManager.INTERVAL_DAILY, - time - 1000 * 60, - time, - ) - return stats != null && stats.isNotEmpty() + val ops = requireContext().getSystemService(Context.APP_OPS_SERVICE) as AppOpsManager + val mode = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + ops.unsafeCheckOpNoThrow( + AppOpsManager.OPSTR_GET_USAGE_STATS, + android.os.Process.myUid(), + requireContext().packageName + ) + } else { + ops.checkOpNoThrow( + AppOpsManager.OPSTR_GET_USAGE_STATS, + android.os.Process.myUid(), + requireContext().packageName + ) + } + + return when (mode) { + AppOpsManager.MODE_ALLOWED -> true + AppOpsManager.MODE_DEFAULT, + AppOpsManager.MODE_IGNORED, + AppOpsManager.MODE_ERRORED -> false + else -> false + } }
127-137: Inconsistent implementation between Activity and Fragment extensions.The
Fragment.hasUsageStatsPermission()method still uses the oldUsageStatsManager.queryUsageStats()approach, whileAppCompatActivity.hasUsageStatsPermission()uses the newAppOpsManagerapproach. This inconsistency could lead to different permission check results.Consider updating the Fragment implementation to match the Activity implementation:
fun Fragment.hasUsageStatsPermission(): Boolean { - val usageStatsManager = - requireContext().getSystemService(Context.USAGE_STATS_SERVICE) as UsageStatsManager - val time = System.currentTimeMillis() - val stats = usageStatsManager.queryUsageStats( - UsageStatsManager.INTERVAL_DAILY, - time - 1000 * 60, - time, - ) - return stats != null && stats.isNotEmpty() + val ops = requireContext().getSystemService(Context.APP_OPS_SERVICE) as AppOpsManager + val mode = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + ops.unsafeCheckOpNoThrow( + AppOpsManager.OPSTR_GET_USAGE_STATS, + android.os.Process.myUid(), + requireContext().packageName + ) + } else { + ops.checkOpNoThrow( + AppOpsManager.OPSTR_GET_USAGE_STATS, + android.os.Process.myUid(), + requireContext().packageName + ) + } + + return when (mode) { + AppOpsManager.MODE_ALLOWED -> true + AppOpsManager.MODE_DEFAULT, + AppOpsManager.MODE_IGNORED, + AppOpsManager.MODE_ERRORED -> false + else -> false + } }
🧹 Nitpick comments (3)
feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt (1)
169-174: Consider renaming method for clarity.The simplified
signUp()method correctly removes server dependencies and immediately emits success. However, since it no longer performs actual user registration, consider renaming it to something more descriptive likecompleteOnboarding()orfinishOnboarding()to better reflect its current functionality.- fun signUp() { + fun completeOnboarding() { viewModelScope.launch { _onboardEffect.emit(OnboardEffect.OnboardSuccess) AmplitudeUtils.trackEventWithProperties("complete_onboarding_finish") } }feature/mypage/src/main/res/layout/fragment_my_page.xml (1)
2-5: Consider keeping thetoolsnamespace for better design-time previewsRemoving the
xmlns:toolsdeclaration eliminates the ability to use design-time attributes (e.g.,tools:context,tools:text).
If the project still relies on Android Studio’s layout preview, re-add the namespace even if notools:attributes are currently present—future UI tweaks often benefit from it.-<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" - xmlns:app="http://schemas.android.com/apk/res-auto" +<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" + xmlns:app="http://schemas.android.com/apk/res-auto" + xmlns:tools="http://schemas.android.com/tools"core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/AuthInterceptor.kt (1)
8-8: Address the TODO: Remove the AuthInterceptor entirely.The TODO comment indicates this class should be deleted, and given that it no longer performs any authentication logic, it has become dead code.
Since this interceptor no longer serves any purpose in the local authentication architecture, would you like me to help identify all the places where this class is still being injected or referenced so it can be completely removed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (84)
.github/workflows/develop_PR_builder.yml(0 hunks).github/workflows/release.yml(0 hunks)app/build.gradle.kts(0 hunks)app/proguard-rules.pro(0 hunks)app/src/main/AndroidManifest.xml(0 hunks)app/src/main/java/com/hmh/hamyeonham/HMHApplication.kt(0 hunks)app/src/main/java/com/hmh/hamyeonham/StartActivity.kt(2 hunks)app/src/main/java/com/hmh/hamyeonham/navigation/DefaultNavigationProvider.kt(0 hunks)build-logic/convention/src/main/kotlin/com/hmh/hamyeonham/plugin/CommonConfigs.kt(0 hunks)core/common/src/main/java/com/hmh/hamyeonham/common/navigation/NavigationProvider.kt(0 hunks)core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionActivity.kt(2 hunks)core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionExt.kt(2 hunks)core/common/src/main/java/com/hmh/hamyeonham/common/qualifier/OAuth.kt(0 hunks)core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt(3 hunks)core/designsystem/src/main/res/values/themes.xml(2 hunks)core/network/build.gradle.kts(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/AuthenticatorUtil.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/HMHAuthenticator.kt(2 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/AuthInterceptor.kt(1 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/di/NetModule.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/di/PointModule.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/PointService.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/ChallengeDateRequest.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/EarnPointRequest.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/EarnPointResponse.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/PointListResponse.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/UsablePointResponse.kt(0 hunks)core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/UsePointResponse.kt(0 hunks)core/viewmodel/main/build.gradle.kts(0 hunks)core/viewmodel/main/src/main/java/com/hmh/hamyeonham/core/viewmodel/MainEffect.kt(0 hunks)core/viewmodel/main/src/main/java/com/hmh/hamyeonham/core/viewmodel/MainViewModel.kt(1 hunks)data/login/.gitignore(0 hunks)data/login/build.gradle.kts(0 hunks)data/login/src/main/AndroidManifest.xml(0 hunks)data/point/.gitignore(0 hunks)data/point/build.gradle.kts(0 hunks)data/point/src/main/AndroidManifest.xml(0 hunks)data/point/src/main/java/com/hmh/hamyeonham/data/point/PointMapper.kt(0 hunks)data/point/src/main/java/com/hmh/hamyeonham/data/point/di/PointBinder.kt(0 hunks)data/point/src/main/java/com/hmh/hamyeonham/data/point/repository/DefaultPointRepository.kt(0 hunks)data/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/mapper/MypageMapper.kt(1 hunks)data/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/repository/DefaultUserInfoRepository.kt(1 hunks)domain/login/.gitignore(0 hunks)domain/login/build.gradle.kts(0 hunks)domain/login/src/main/AndroidManifest.xml(0 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/Login.kt(0 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignRequestDomain.kt(0 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignUpUser.kt(0 hunks)domain/login/src/main/java/com/hmh/hamyeonham/login/repository/AuthRepository.kt(0 hunks)domain/point/.gitignore(0 hunks)domain/point/build.gradle.kts(0 hunks)domain/point/src/main/AndroidManifest.xml(0 hunks)domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/EarnPoint.kt(0 hunks)domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/PointInfo.kt(0 hunks)domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/UsablePoint.kt(0 hunks)domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/UsePoint.kt(0 hunks)domain/point/src/main/java/com/hmh/hamyeonham/domain/point/repository/PointRepository.kt(0 hunks)domain/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/model/UserInfo.kt(0 hunks)feature/challenge/build.gradle.kts(0 hunks)feature/challenge/src/main/AndroidManifest.xml(0 hunks)feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/ChallengeFragment.kt(1 hunks)feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointActivity.kt(0 hunks)feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointAdapter.kt(0 hunks)feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointViewModel.kt(0 hunks)feature/challenge/src/main/res/layout/activity_point.xml(0 hunks)feature/challenge/src/main/res/layout/fragment_challenge.xml(0 hunks)feature/login/build.gradle.kts(0 hunks)feature/login/src/main/AndroidManifest.xml(0 hunks)feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginActivity.kt(0 hunks)feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt(0 hunks)feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewPagerAdapter.kt(0 hunks)feature/login/src/main/res/layout/activity_login.xml(0 hunks)feature/login/src/main/res/values/strings.xml(0 hunks)feature/main/src/main/java/com/hmh/hamyeonham/feature/main/MainActivity.kt(1 hunks)feature/main/src/main/res/values/strings.xml(0 hunks)feature/mypage/build.gradle.kts(0 hunks)feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/MyPageFragment.kt(3 hunks)feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt(1 hunks)feature/mypage/src/main/res/layout/fragment_my_page.xml(2 hunks)feature/mypage/src/main/res/values/strings.xml(1 hunks)feature/onboarding/build.gradle.kts(0 hunks)feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt(2 hunks)gradle/libs.versions.toml(0 hunks)settings.gradle.kts(0 hunks)
💤 Files with no reviewable changes (67)
- feature/login/src/main/AndroidManifest.xml
- core/network/build.gradle.kts
- data/login/build.gradle.kts
- feature/challenge/build.gradle.kts
- feature/mypage/build.gradle.kts
- feature/onboarding/build.gradle.kts
- domain/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/model/UserInfo.kt
- domain/point/src/main/AndroidManifest.xml
- data/login/.gitignore
- data/point/src/main/AndroidManifest.xml
- core/viewmodel/main/build.gradle.kts
- app/proguard-rules.pro
- domain/login/build.gradle.kts
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/EarnPointRequest.kt
- app/src/main/java/com/hmh/hamyeonham/navigation/DefaultNavigationProvider.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/model/Login.kt
- domain/login/.gitignore
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/UsePointResponse.kt
- app/src/main/java/com/hmh/hamyeonham/HMHApplication.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/EarnPointResponse.kt
- core/common/src/main/java/com/hmh/hamyeonham/common/navigation/NavigationProvider.kt
- data/login/src/main/AndroidManifest.xml
- data/point/.gitignore
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/ChallengeDateRequest.kt
- domain/point/.gitignore
- feature/login/build.gradle.kts
- domain/login/src/main/AndroidManifest.xml
- domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/UsePoint.kt
- feature/challenge/src/main/AndroidManifest.xml
- feature/login/src/main/res/layout/activity_login.xml
- data/point/src/main/java/com/hmh/hamyeonham/data/point/PointMapper.kt
- core/common/src/main/java/com/hmh/hamyeonham/common/qualifier/OAuth.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/AuthenticatorUtil.kt
- app/src/main/AndroidManifest.xml
- domain/point/build.gradle.kts
- .github/workflows/release.yml
- build-logic/convention/src/main/kotlin/com/hmh/hamyeonham/plugin/CommonConfigs.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/di/NetModule.kt
- data/point/build.gradle.kts
- settings.gradle.kts
- app/build.gradle.kts
- core/viewmodel/main/src/main/java/com/hmh/hamyeonham/core/viewmodel/MainEffect.kt
- domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/EarnPoint.kt
- domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/UsablePoint.kt
- .github/workflows/develop_PR_builder.yml
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/UsablePointResponse.kt
- feature/challenge/src/main/res/layout/fragment_challenge.xml
- core/network/src/main/java/com/hmh/hamyeonham/core/network/di/PointModule.kt
- feature/login/src/main/res/values/strings.xml
- domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignUpUser.kt
- gradle/libs.versions.toml
- data/point/src/main/java/com/hmh/hamyeonham/data/point/repository/DefaultPointRepository.kt
- feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewPagerAdapter.kt
- feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointActivity.kt
- domain/point/src/main/java/com/hmh/hamyeonham/domain/point/model/PointInfo.kt
- feature/main/src/main/res/values/strings.xml
- feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointViewModel.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/model/PointListResponse.kt
- data/point/src/main/java/com/hmh/hamyeonham/data/point/di/PointBinder.kt
- feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginActivity.kt
- domain/point/src/main/java/com/hmh/hamyeonham/domain/point/repository/PointRepository.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/repository/AuthRepository.kt
- feature/challenge/src/main/res/layout/activity_point.xml
- core/network/src/main/java/com/hmh/hamyeonham/core/network/point/PointService.kt
- feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/point/PointAdapter.kt
- domain/login/src/main/java/com/hmh/hamyeonham/login/model/SignRequestDomain.kt
- feature/login/src/main/java/com/hmh/hamyeonham/feature/login/LoginViewModel.kt
✅ Files skipped from review due to trivial changes (3)
- core/designsystem/src/main/res/values/themes.xml
- data/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/repository/DefaultUserInfoRepository.kt
- feature/main/src/main/java/com/hmh/hamyeonham/feature/main/MainActivity.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/com/hmh/hamyeonham/StartActivity.kt
- feature/mypage/src/main/res/values/strings.xml
- core/database/src/main/java/com/hmh/hamyeonham/core/database/HMHRoomDatabase.kt
- core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/authenticator/HMHAuthenticator.kt
- feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/MyPageFragment.kt
⏰ 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: PR Checker
🔇 Additional comments (16)
feature/onboarding/src/main/java/com/hmh/hamyeonham/feature/onboarding/viewmodel/OnBoardingViewModel.kt (1)
65-65: LGTM: Constructor simplified correctly.The removal of
AuthRepositoryandHMHNetworkPreferencedependencies aligns with the PR objective of transitioning from server-based to local authentication. The@Injectannotation is still appropriate for Hilt dependency injection.feature/mypage/src/main/res/layout/fragment_my_page.xml (2)
77-78: Constraint updated correctly after point section removalAnchoring
tv_journeydirectly belowtv_user_nameis the right adjustment and keeps the vertical flow intact after deleting the point block. No issues spotted.
98-100: Divider view constraints look good
v_journeynow referencestv_user_name, matching the new hierarchy. Layout consistency maintained.core/viewmodel/main/src/main/java/com/hmh/hamyeonham/core/viewmodel/MainViewModel.kt (1)
101-108: LGTM! Simplified logic aligns with point feature removal.The method correctly simplifies the daily challenge failure handling by removing point-related logic while maintaining the core functionality of unlocking and refreshing challenge status.
feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/ChallengeFragment.kt (1)
178-182: LGTM! Direct navigation simplifies the challenge creation flow.The method correctly removes point-related conditional logic and provides direct navigation to the new challenge creation activity, which aligns with the broader removal of point features.
data/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/mapper/MypageMapper.kt (1)
6-10: Verified:UserInfoMapping Is Correct and Point-Free
TheUserInfodata class now only definesname, and nopointreferences remain in the codebase. This aligns perfectly with the PR’s goal of removing point-related functionality.• domain/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/mapper/MypageMapper.kt – maps only
name
• domain/userinfo/src/main/java/com/hmh/hamyeonham/userinfo/model/UserInfo.kt – data class contains solelynamefeature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt (1)
20-21: Approve the SharedFlow renaming.The renaming from
_userEffect/userEffectto_effect/effectis more concise and appropriate given the reduced scope of this ViewModel.core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionActivity.kt (5)
3-3: LGTM on import optimizations.The import changes properly reflect the updated dependencies - removing unused
UsageStatsManagerand addingAppOpsManager,Build, andandroidx.core.net.toUri.Also applies to: 7-7, 13-13
120-120: Good use of Kotlin extension function.Using
toUri()extension function is more idiomatic Kotlin compared toUri.parse().
3-3: LGTM: Import updates align with implementation changes.The import changes from
UsageStatsManagertoAppOpsManagerand addition ofBuildare consistent with the refactored permission checking approach.Also applies to: 7-7
13-13: LGTM: Modern URI extension usage.Using the
toUri()extension fromandroidx.core.netis a good improvement overUri.parse().
120-120: LGTM: Consistent URI extension usage.The change to use
toUri()extension is consistent with the import addition and improves code readability.core/common/src/main/java/com/hmh/hamyeonham/common/permission/PermissionExt.kt (4)
4-4: LGTM on adding AppOpsManager import.Required for the improved permission checking implementation.
41-65: Excellent improvement to usage stats permission checking.The new implementation using
AppOpsManageris more reliable and direct compared to the previous approach of querying usage stats data. The version-specific handling for Android Q+ is appropriate, and the explicit mode checking covers all possible return states.
4-4: LGTM: Required import for new implementation.Adding
AppOpsManagerimport is necessary for the improved permission checking approach.
41-65: Excellent refactor to use AppOpsManager for more reliable permission checking.The new implementation using
AppOpsManageris more direct and reliable than the previousUsageStatsManager.queryUsageStats()approach. The API level handling for Android Q+ and comprehensive mode checking are well implemented.
| class AuthInterceptor @Inject constructor( | ||
| private val authenticatorUtil: AuthenticatorUtil | ||
| ) : Interceptor { | ||
|
|
||
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val response = chain.proceed(chain.request()) | ||
|
|
||
| if (response.code == 404) { | ||
| val responseBody = response.body?.string() ?: "" | ||
| if (responseBody.contains("유저를 찾을 수 없습니다.")) { | ||
| runBlocking { | ||
| authenticatorUtil.handleLogout() | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| return response | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove dead code - this interceptor serves no purpose.
The interceptor now only passes through requests without any processing, making it effectively dead code. Since the authentication logic has been moved to a local Room DB approach, this network interceptor is no longer needed.
Consider removing this class entirely and updating the DI configuration to exclude it from the OkHttp client setup. This will:
- Reduce unnecessary object instantiation
- Eliminate confusion for future maintainers
- Clean up the codebase as part of the authentication refactoring
Let me verify where this interceptor is still being used in the DI setup:
🏁 Script executed:
#!/bin/bash
# Description: Find all references to AuthInterceptor to ensure complete removal
# Expected: References in DI modules and potentially in tests
echo "=== Searching for AuthInterceptor references ==="
rg -A 3 -B 3 "AuthInterceptor"
echo "=== Searching for interceptor usage in DI modules ==="
rg -A 5 -B 5 "interceptor" --type kotlin | grep -E "(provides|inject|bind)"Length of output: 1409
🏁 Script executed:
#!/bin/bash
# Description: Locate OkHttpClient setup to check interceptor bindings
echo "=== Searching for OkHttpClient usage in codebase ==="
rg --type kotlin "OkHttpClient" -A5 -B5Length of output: 6646
Remove unused AuthInterceptor
AuthInterceptor isn’t referenced by any DI module or tests and only forwards requests. You can safely delete it and its TODO.
• Delete
- core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/AuthInterceptor.kt
🤖 Prompt for AI Agents
In
core/network/src/main/java/com/hmh/hamyeonham/core/network/auth/interceptor/AuthInterceptor.kt
lines 10 to 16, the AuthInterceptor class is unused and only forwards requests
without modification. Remove the entire AuthInterceptor.kt file to clean up
unused code and delete any related TODO comments referencing it.
| @@ -1,11 +1,10 @@ | |||
| package com.hmh.hamyeonham.login.mapper | |||
| package com.hmh.hamyeonham.userinfo.mapper | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that all imports referencing the old package are updated.
The package name has been changed from com.hmh.hamyeonham.login.mapper to com.hmh.hamyeonham.userinfo.mapper. Ensure that all files importing this mapper have been updated to use the new package path.
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining imports of the old package path
# Expected: No results should be found for the old package path
echo "Searching for old package imports..."
rg "com\.hmh\.hamyeonham\.login\.mapper" --type kotlin
echo "Searching for imports of MypageMapper..."
rg "import.*MypageMapper" --type kotlin -A 2 -B 2Length of output: 518
Update remaining references to the old mapper package
The search revealed two files still importing or declaring the old package. Please update them to the new com.hmh.hamyeonham.userinfo.mapper path:
-
data/main/src/main/java/com/hmh/hamyeonham/data/main/DefaultMainRepository.kt
Change
import com.hmh.hamyeonham.login.mapper.toBanner
to
import com.hmh.hamyeonham.userinfo.mapper.toBanner -
data/main/src/main/java/com/hmh/hamyeonham/data/main/mapper/Mapper.kt
Change
package com.hmh.hamyeonham.login.mapper
to
package com.hmh.hamyeonham.userinfo.mapper
This will ensure imports resolve correctly after the refactoring.
🤖 Prompt for AI Agents
In
data/main/src/main/java/com/hmh/hamyeonham/data/main/DefaultMainRepository.kt,
update the import statement from com.hmh.hamyeonham.login.mapper.toBanner to
com.hmh.hamyeonham.userinfo.mapper.toBanner. In
data/main/src/main/java/com/hmh/hamyeonham/data/main/mapper/Mapper.kt, change
the package declaration from com.hmh.hamyeonham.login.mapper to
com.hmh.hamyeonham.userinfo.mapper. These changes will align the references with
the new mapper package path.
| data class LogoutFail(val message: String) : UserEffect | ||
| data object WithdrawalSuccess : UserEffect | ||
|
|
||
| data object WithdrawalFail : UserEffect | ||
| data class WithdrawalFail(val message: String) : UserEffect |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused message parameters from failure cases.
The LogoutFail and WithdrawalFail data classes now include a message: String parameter, but the empty logout() and withdrawal() functions don't emit any failure events. This creates dead code and an inconsistent interface.
Apply this diff to simplify the interface:
- data class LogoutFail(val message: String) : UserEffect
+ data object LogoutFail : UserEffect
data object WithdrawalSuccess : UserEffect
- data class WithdrawalFail(val message: String) : UserEffect
+ data object WithdrawalFail : UserEffect📝 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 LogoutFail(val message: String) : UserEffect | |
| data object WithdrawalSuccess : UserEffect | |
| data object WithdrawalFail : UserEffect | |
| data class WithdrawalFail(val message: String) : UserEffect | |
| data object LogoutFail : UserEffect | |
| data object WithdrawalSuccess : UserEffect | |
| data object WithdrawalFail : UserEffect |
🤖 Prompt for AI Agents
In
feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt
around lines 11 to 13, remove the unused message parameters from the LogoutFail
and WithdrawalFail data classes since the logout() and withdrawal() functions do
not emit failure events. Change LogoutFail and WithdrawalFail from data classes
with a message parameter to simple objects without parameters to simplify and
make the interface consistent.
| class MyPageViewModel @Inject constructor( | ||
| private val authRepository: AuthRepository, | ||
| private val hmhPreference: DefaultHMHNetworkPreference, | ||
| private val databaseManager: DatabaseManager | ||
| ) : ViewModel() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the empty ViewModel or implementing placeholder logic.
The constructor has no dependencies and the core functions are empty, making this ViewModel essentially non-functional. This could lead to UI elements that appear interactive but don't perform any actions.
Consider one of these approaches:
- If authentication features are permanently removed, remove this ViewModel and update the UI accordingly
- If this is temporary during migration, add TODO comments and basic placeholder implementations
- If local authentication will be implemented later, add the necessary Room DB dependencies
@HiltViewModel
class MyPageViewModel @Inject constructor(
// TODO: Add local auth dependencies when implementing Room DB authentication
) : ViewModel() {🤖 Prompt for AI Agents
In
feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt
at lines 17-18, the ViewModel class is empty with no dependencies or
functionality, making it non-functional and potentially confusing for the UI. To
fix this, either remove the ViewModel if authentication features are permanently
removed, or if this is temporary, add TODO comments indicating planned
implementations and include basic placeholder logic. If local authentication is
planned, add the necessary Room DB dependencies in the constructor and annotate
the class with @HiltViewModel to prepare for future development.
| fun logout() { | ||
|
|
||
| fun handleWithdrawal() { | ||
| viewModelScope.launch { | ||
| authRepository.withdrawal(hmhPreference.accessToken).onSuccess { | ||
| deleteAllDatabase() | ||
| clearPreference() | ||
| _userEffect.emit(UserEffect.WithdrawalSuccess) | ||
| Log.d("hmhPreferenceAccessToken", hmhPreference.accessToken) | ||
| }.onFailure { | ||
| _userEffect.emit(UserEffect.WithdrawalFail) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun clearPreference() { | ||
| hmhPreference.clear() | ||
| } | ||
| fun withdrawal() { | ||
|
|
||
| private fun deleteAllDatabase() { | ||
| viewModelScope.launch { | ||
| databaseManager.deleteAll() | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement basic functionality or add clear documentation.
The empty logout() and withdrawal() functions create a poor user experience where UI interactions appear to do nothing. This could confuse users and developers.
Apply this diff to add placeholder implementations with proper communication:
fun logout() {
-
+ // TODO: Implement local logout when Room DB authentication is ready
+ viewModelScope.launch {
+ _effect.emit(UserEffect.LogoutSuccess)
+ }
}
fun withdrawal() {
-
+ // TODO: Implement local account deletion when Room DB authentication is ready
+ viewModelScope.launch {
+ _effect.emit(UserEffect.WithdrawalSuccess)
+ }
}Don't forget to add the missing import:
import androidx.lifecycle.viewModelScope
import kotlinx.coroutines.launch🤖 Prompt for AI Agents
In
feature/mypage/src/main/java/com/hmh/hamyeonham/mypage/viewmodel/MyPageViewModel.kt
around lines 23 to 29, the logout() and withdrawal() functions are empty,
leading to a poor user experience. Implement basic placeholder functionality by
launching coroutines within viewModelScope that perform minimal actions or log
messages to indicate these functions are called. Also, add the missing imports
for androidx.lifecycle.viewModelScope and kotlinx.coroutines.launch to support
coroutine usage.
개요
작업 사항
변경 사항
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation