-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 強制リフレッシュ処理を追加 #113
🐛 強制リフレッシュ処理を追加 #113
Conversation
Walkthrough認証関連のコードベースにおいて、セッションのリフレッシュ機能が強化されました。 Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- core/data/src/commonMain/kotlin/club/nito/core/data/DefaultAuthRepository.kt (1 hunks)
- core/datastore/src/commonMain/kotlin/club/nito/core/datastore/DataStore.kt (1 hunks)
- core/datastore/src/commonMain/kotlin/club/nito/core/datastore/SettingsDataStore.kt (1 hunks)
- core/network/src/commonMain/kotlin/club/nito/core/network/auth/AuthRemoteDataSource.kt (1 hunks)
- core/network/src/commonMain/kotlin/club/nito/core/network/auth/FakeAuthRemoteDataSource.kt (1 hunks)
- core/network/src/commonMain/kotlin/club/nito/core/network/auth/SupabaseAuthRemoteDataSource.kt (1 hunks)
Additional comments: 3
core/datastore/src/commonMain/kotlin/club/nito/core/datastore/DataStore.kt (1)
- 3-5: 新しいサスペンド関数
setRefreshed
とisRefreshed
がDataStore
インターフェースに追加されました。これらの変更は、インターフェースを実装するクラスに新しいリフレッシュ状態管理メソッドを導入します。実装クラスがこれらの新しいメソッドを適切にサポートするように更新されていることを確認してください。core/network/src/commonMain/kotlin/club/nito/core/network/auth/FakeAuthRemoteDataSource.kt (1)
- 79-81: 現在の実装では
refreshCurrentSession
メソッドは何も行わないプレースホルダーとして追加されています。将来的な実装やテストのためにこのメソッドが準備されていることを確認してください。また、このメソッドが呼び出される際には、適切な処理が実装されるまで何も行われないことを意識する必要があります。core/network/src/commonMain/kotlin/club/nito/core/network/auth/SupabaseAuthRemoteDataSource.kt (1)
- 59-59: 新しいメソッド
refreshCurrentSession
がSupabaseAuthRemoteDataSource
クラスに追加されました。このメソッドはgoTrue.refreshCurrentSession()
を直接呼び出しており、ロジック上の問題は見受けられません。ただし、セッションのリフレッシュが失敗した場合のエラーハンドリングについての情報が不足しています。この点については、コードベースの他の部分で管理されているかどうかを確認する必要があります。
override val authStatus: Flow<AuthStatus> = remoteDataSource.authStatus.map { | ||
if (it is AuthStatus.Authenticated && dataStore.isRefreshed().not()) { | ||
remoteDataSource.refreshCurrentSession() | ||
dataStore.setRefreshed(true) | ||
} | ||
|
||
it | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authStatus
フローの変換ロジックに例外処理が含まれていません。remoteDataSource.refreshCurrentSession()
の呼び出しに失敗した場合に備えて、エラーハンドリングを追加することを検討してください。また、セッションのリフレッシュが成功した後に新しいAuthStatus
を発行する必要があるかどうかを確認してください。
+ try {
+ } catch (e: Exception) {
+ // Handle the exception, possibly by logging or rethrowing
+ }
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.
override val authStatus: Flow<AuthStatus> = remoteDataSource.authStatus.map { | |
if (it is AuthStatus.Authenticated && dataStore.isRefreshed().not()) { | |
remoteDataSource.refreshCurrentSession() | |
dataStore.setRefreshed(true) | |
} | |
it | |
} | |
override val authStatus: Flow<AuthStatus> = remoteDataSource.authStatus.map { | |
if (it is AuthStatus.Authenticated && dataStore.isRefreshed().not()) { | |
try { | |
remoteDataSource.refreshCurrentSession() | |
dataStore.setRefreshed(true) | |
} catch (e: Exception) { | |
// Handle the exception, possibly by logging or rethrowing | |
} | |
} | |
it | |
} |
public suspend fun logout() | ||
public suspend fun modifyAuthUser(email: String?, password: String?): UserInfo | ||
public suspend fun authIfNeeded() | ||
public suspend fun refreshCurrentSession() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 NOTE
Codebase verification is a beta feature.
AuthRemoteDataSource
インターフェースにrefreshCurrentSession
メソッドが追加されましたが、このインターフェースを実装するクラスに新しいメソッドの実装が見つかりませんでした。すべての実装クラスが新しいメソッドを適切に実装しているか確認する必要があります。
@OptIn(ExperimentalSettingsApi::class) | ||
public class SettingsDataStore( | ||
private val settings: Settings, | ||
@OptIn(ExperimentalSettingsApi::class) | ||
private val suspendSettings: SuspendSettings = settings.toSuspendSettings(), | ||
private val json: Json, | ||
) : DataStore | ||
) : DataStore { | ||
override suspend fun setRefreshed(isRefreshed: Boolean) { | ||
suspendSettings.putBoolean(REFRESHED_KEY, isRefreshed) | ||
} | ||
|
||
override suspend fun isRefreshed(): Boolean = suspendSettings.getBoolean(REFRESHED_KEY, false) | ||
|
||
public companion object { | ||
private const val REFRESHED_KEY = "refreshed-v0-3-3" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 NOTE
Codebase verification is a beta feature.
SettingsDataStore
クラスに対する変更は、DataStore
インターフェースの実装と実験的な SuspendSettings
API の使用を含んでいます。@OptIn(ExperimentalSettingsApi::class)
アノテーションは、他の2つのファイルでも使用されていることが確認されましたが、DataStore
インターフェースの setRefreshed
および isRefreshed
メソッドを実装している他のクラスは見つかりませんでした。このことは、新しいインターフェースメソッドが現在のコードベースで広く採用されていない可能性があることを示唆しています。この変更が他の部分にどのような影響を与えるかを検証する必要があります。
Issue
概要
強制リフレッシュ処理を追加します。
レビュー観点
特になし
レビューレベル
レビュー優先度
参考リンク
スクリーンショット
Summary by CodeRabbit
新機能
バグ修正
ドキュメント