Conversation
- 공통 예외 핸들링 유틸리티 handleException 추가 - 예외 상황에 따라 토스트 또는 다이얼로그를 선택적으로 적용
- 필수 속성을 confirm 기준으로 변경 (dismiss -> confirm)
- 사용하지 않는 문자열 리소스 제거
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Android CI Summary
Step Results:
- Unit Test: ❌ Failure (0s)
- Debug Build: ⏭️ Skipped (0s)
- Code Style Check: ✅ Success (3m 57s)
Total Time: 3m 57s
See the Actions Log for details.
📝 WalkthroughWalkthroughThis PR introduces a centralized event-driven dialog system with standardized error handling. It adds EventHandler with Channel-based event dispatch, creates ErrorScope enum and error utilities, updates MetaSearchDialog and MetaSearchToast component APIs, and integrates error handling across features including MainActivity and the person feature module. Changes
Sequence DiagramsequenceDiagram
actor User
participant Feature as Feature Module
participant ErrorHandler as Exception Handler
participant EventHandler as EventHandler
participant MainActivity as MainActivity
participant Dialog as MetaSearchDialog
User->>Feature: Trigger error (e.g., delete, load)
Feature->>ErrorHandler: handleException(exception)
ErrorHandler->>ErrorHandler: Classify error type<br/>(network/HTTP/unknown)
ErrorHandler->>EventHandler: showErrorDialog(scope, exception)
EventHandler->>EventHandler: Build MetaSearchDialogSpec<br/>with title & message
EventHandler->>EventHandler: Send ShowDialog event
MainActivity->>EventHandler: Observe eventFlow
EventHandler->>MainActivity: Emit ShowDialog(spec)
MainActivity->>MainActivity: Update dialogSpec state
MainActivity->>Dialog: Render MetaSearchDialog
User->>Dialog: Click confirm button
Dialog->>MainActivity: onConfirm callback
MainActivity->>MainActivity: Clear dialogSpec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/designsystem/src/main/java/com/example/metasearch/core/designsystem/component/MetaSearchToast.kt (1)
23-56: Consider making the Surface rendering conditional on message being non-null.Currently, when
isVisibleistruebutmessageisnull, theSurfacewill still render as an empty black rounded rectangle. This could create a confusing visual glitch where users see an empty toast.Consider wrapping the
Surfaceinside themessage?.letblock or adding an early return condition.🎨 Proposed fix to prevent empty toast rendering
@Composable fun MetaSearchToast( modifier: Modifier = Modifier, message: String? = null, isVisible: Boolean, ) { Box( modifier = Modifier .fillMaxSize(), contentAlignment = Alignment.Center, ) { - AnimatedVisibility( - visible = isVisible, + AnimatedVisibility( + visible = isVisible && message != null, enter = fadeIn() + slideInVertically(initialOffsetY = { it / 2 }), exit = fadeOut() + slideOutVertically(targetOffsetY = { it / 2 }), ) { Surface( color = Color.Black.copy(alpha = 0.6f), shape = RoundedCornerShape(MetaSearchTheme.radius.full), modifier = modifier.padding(horizontal = MetaSearchTheme.spacing.spacing8), ) { - message?.let { - Text( - text = it, - color = LightPink, - style = MetaSearchTheme.typography.captionSmall, - modifier = Modifier.padding( - horizontal = MetaSearchTheme.spacing.spacing4, - vertical = MetaSearchTheme.spacing.spacing2, - ), - ) - } + Text( + text = message ?: "", + color = LightPink, + style = MetaSearchTheme.typography.captionSmall, + modifier = Modifier.padding( + horizontal = MetaSearchTheme.spacing.spacing4, + vertical = MetaSearchTheme.spacing.spacing2, + ), + ) } } } }
🤖 Fix all issues with AI agents
In
@feature/main/src/main/java/com/example/metasearch/feature/main/MainActivity.kt:
- Around line 65-67: The dialog dismissal handler currently only clears
dialogSpec.value and never triggers the spec's onDismiss callback; update the
onDismissRequest lambda that sets dialogSpec.value = null to also invoke the
MetaSearchDialogSpec's onDismiss (e.g., call the selected spec's onDismiss
function or safe-invoke it) so any cleanup or state updates in the spec run when
the dialog is dismissed.
- Around line 63-76: The dialog is missing its message body because
MetaSearchDialog isn't receiving the MetaSearchDialogSpec.description; update
the MetaSearchDialog call inside the dialogSpec.value?.let { spec -> ... } block
to pass the description as the composable content (e.g., supply a content = {
Text(spec.description) } or the appropriate content parameter on
MetaSearchDialog) and add the import androidx.compose.material3.Text so the
description is rendered.
🧹 Nitpick comments (5)
core/common/build.gradle.kts (1)
4-4: Remove the unnecessary Retrofit plugin from core.common.The Retrofit plugin is not needed in this module. While
core.commonimportsretrofit2.HttpExceptionfor error handling, it doesn't use any Retrofit annotations (@get, @post, etc.) or configure Retrofit instances. TheHttpExceptionclass is available through thecore.networkdependency, making the plugin redundant. Remove line 4 to keep the build configuration clean.feature/person/src/main/java/com/example/metasearch/feature/person/PersonPresenter.kt (1)
109-109: Consider clearingtoastMessagewhen hiding the toast.This prevents stale message content from potentially being visible if
showToastis set totruebefore a new message is assigned.♻️ Suggested change
- PersonUiEvent.HideToast -> showToast = false + PersonUiEvent.HideToast -> { + showToast = false + toastMessage = "" + }core/common/src/main/java/com/example/metasearch/core/common/utils/Exception.kt (1)
36-41: Consider simplifyingisNetworkErrorcheck.
UnknownHostException,ConnectException, andSocketTimeoutExceptionall extendIOException, making the individual checks redundant. However, keeping them explicit documents the specific network errors you're targeting.♻️ Option A: Simplified (covers all network I/O)
fun Throwable.isNetworkError(): Boolean { - return this is UnknownHostException || - this is ConnectException || - this is SocketTimeoutException || - this is IOException + return this is IOException }♻️ Option B: Explicit only (more targeted)
fun Throwable.isNetworkError(): Boolean { return this is UnknownHostException || this is ConnectException || - this is SocketTimeoutException || - this is IOException + this is SocketTimeoutException }The current implementation works correctly; this is purely a readability consideration.
core/common/src/main/java/com/example/metasearch/core/common/utils/EventHandler.kt (2)
12-14:trySendresult is not checked.
trySendreturns aChannelResultindicating success or failure. If the channel buffer is full, the event will be silently dropped.♻️ Option A: Log failures for debugging
fun sendEvent(event: MetaSearchEvent) { - _eventFlow.trySend(event) + val result = _eventFlow.trySend(event) + if (result.isFailure) { + // Consider logging: event dropped + } }♻️ Option B: Use suspending send (requires coroutine scope)
If event delivery is critical, consider making this a suspend function or providing a coroutine scope to ensure events are never dropped.
8-15: Consider lifecycle awareness for the event flow.The singleton
EventHandlerwith a buffered channel has some caveats:
- Events sent when no collector exists are buffered but may be lost if buffer overflows
- If multiple components collect from
eventFlow, each event goes to only one collector (fan-out behavior fromreceiveAsFlow)For the current use case (single collector in
MainActivity), this works, but document these constraints for future maintainers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/common/build.gradle.ktscore/common/src/main/java/com/example/metasearch/core/common/constants/ErrorScope.ktcore/common/src/main/java/com/example/metasearch/core/common/utils/EventHandler.ktcore/common/src/main/java/com/example/metasearch/core/common/utils/Exception.ktcore/designsystem/src/main/java/com/example/metasearch/core/designsystem/component/MetaSearchToast.ktcore/ui/src/main/java/com/example/metasearch/core/ui/component/MetaSearchDialog.ktfeature/detail/src/main/java/com/example/metasearch/feature/detail/graph/GraphDetailUi.ktfeature/graph/src/main/java/com/example/metasearch/feature/graph/GraphUi.ktfeature/main/src/main/java/com/example/metasearch/feature/main/MainActivity.ktfeature/person/src/main/java/com/example/metasearch/feature/person/PersonPresenter.ktfeature/person/src/main/java/com/example/metasearch/feature/person/PersonUi.ktfeature/person/src/main/java/com/example/metasearch/feature/person/PersonUiState.ktfeature/person/src/main/res/values/strings.xmlfeature/search/src/main/java/com/example/metasearch/feature/search/nls/NLSearchUi.ktfeature/splash/src/main/java/com/example/metasearch/feature/splash/SplashUi.kt
💤 Files with no reviewable changes (1)
- feature/person/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (5)
core/common/build.gradle.kts (1)
build-logic/src/main/java/com/example/metasearch/convention/Dependencies.kt (1)
implementation(7-9)
feature/main/src/main/java/com/example/metasearch/feature/main/MainActivity.kt (1)
core/ui/src/main/java/com/example/metasearch/core/ui/component/MetaSearchDialog.kt (1)
MetaSearchDialog(29-110)
feature/person/src/main/java/com/example/metasearch/feature/person/PersonPresenter.kt (1)
core/common/src/main/java/com/example/metasearch/core/common/utils/Exception.kt (1)
handleException(10-34)
core/common/src/main/java/com/example/metasearch/core/common/utils/Exception.kt (1)
core/common/src/main/java/com/example/metasearch/core/common/utils/EventHandler.kt (1)
showErrorDialog(32-68)
core/ui/src/main/java/com/example/metasearch/core/ui/component/MetaSearchDialog.kt (1)
core/designsystem/src/main/java/com/example/metasearch/core/designsystem/component/MetaSearchButton.kt (1)
MetaSearchButton(21-46)
⏰ 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: ci-build
🔇 Additional comments (14)
core/common/build.gradle.kts (1)
11-13: LGTM! Core.network dependency supports centralized error handling.Adding the
core.networkdependency makes sense for implementing centralized error handling that needs to reference network-related error types likeHttpExceptionandErrorScope.feature/graph/src/main/java/com/example/metasearch/feature/graph/GraphUi.kt (1)
65-72: LGTM!The dialog API updates are consistent with the centralized error handling approach. The shift from
onDismissRequesttoonConfirmRequestanddismissButtonTexttoconfirmButtonTextprovides clearer semantics for the user action.feature/splash/src/main/java/com/example/metasearch/feature/splash/SplashUi.kt (1)
53-70: LGTM!The dialog API updates align with the centralized error handling pattern introduced in this PR. The changes are consistent with other features.
feature/search/src/main/java/com/example/metasearch/feature/search/nls/NLSearchUi.kt (1)
111-124: LGTM!The dialog updates are consistent with the new centralized error handling approach. The API changes improve clarity by distinguishing confirm actions from dismiss actions.
feature/person/src/main/java/com/example/metasearch/feature/person/PersonUi.kt (1)
126-131: LGTM!The shift from a hard-coded toast message to dynamic state-driven content (
state.toastMessage) provides flexibility for displaying different messages. This aligns well with the optional message parameter introduced inMetaSearchToast.core/common/src/main/java/com/example/metasearch/core/common/constants/ErrorScope.kt (1)
1-6: LGTM!Clean enum definition for error scoping. The two scopes (
GLOBALandIMAGE_ANALYSIS) provide clear categorization for error dialog routing.feature/person/src/main/java/com/example/metasearch/feature/person/PersonUiState.kt (1)
13-14: LGTM!Good addition of the nullable
toastMessageproperty to support dynamic toast content. The pairing withshowToastboolean allows for clean separation of visibility control and message content.feature/detail/src/main/java/com/example/metasearch/feature/detail/graph/GraphDetailUi.kt (1)
53-60: LGTM!The dialog API update is correctly applied. The
OnErrorDialogDismissevent name still makes sense semantically since confirming an error dialog effectively dismisses it.feature/person/src/main/java/com/example/metasearch/feature/person/PersonPresenter.kt (1)
86-96: LGTM!Good integration with
handleException. The error handling correctly routes 401 errors to the global dialog system while displaying other errors as toasts.core/common/src/main/java/com/example/metasearch/core/common/utils/Exception.kt (1)
16-17: TheonErrorcallback is not invoked for 401 errors.When a 401
HttpExceptionoccurs,showErrorDialogis called butonErroris never invoked. Callers usinghandleExceptionwon't receive any callback to update their local state (e.g., dismiss loading indicators, reset UI state). Verify if this is the intended behavior.core/common/src/main/java/com/example/metasearch/core/common/utils/EventHandler.kt (1)
32-68: LGTM!The
showErrorDialogfunction provides clean error-to-dialog mapping with appropriate user-friendly messages for different error types. The use ofErrorScopefor context-specific messaging is a good pattern.core/ui/src/main/java/com/example/metasearch/core/ui/component/MetaSearchDialog.kt (3)
32-35: Verify defaultonDismissRequestbehavior for system-initiated dismissals.Defaulting
onDismissRequestto an empty lambda may cause the dialog to appear stuck when dismissed via back press or outside tap (ifpropertiesallows these). TheDialogcomposable will invoke this callback, but with an empty default, nothing happens—no state change to close the dialog.Ensure that callers either:
- Always provide a proper
onDismissRequesthandler, or- Use
DialogProperties(dismissOnBackPress = false, dismissOnClickOutside = false)when relying on the default.
93-106: LGTM!Button rendering logic correctly handles the optional dismiss button. Using
weight(1f)on both buttons ensures proper space distribution in both single-button and dual-button layouts.
112-127: LGTM!Preview is correctly updated to use the new required
onConfirmRequestparameter, and demonstrates the typical two-button dialog configuration.
| dialogSpec.value?.let { spec -> | ||
| MetaSearchDialog( | ||
| onDismissRequest = { | ||
| dialogSpec.value = null | ||
| }, | ||
| onConfirmRequest = { | ||
| spec.onConfirm() | ||
| dialogSpec.value = null | ||
| }, | ||
| dismissButtonText = spec.dismissText, | ||
| confirmButtonText = spec.confirmText, | ||
| title = spec.title, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing dialog description/content.
The MetaSearchDialogSpec contains a description field, but it's not being passed to MetaSearchDialog. The dialog will display without any message body.
🐛 Proposed fix to add content
dialogSpec.value?.let { spec ->
MetaSearchDialog(
onDismissRequest = {
dialogSpec.value = null
},
onConfirmRequest = {
spec.onConfirm()
dialogSpec.value = null
},
dismissButtonText = spec.dismissText,
confirmButtonText = spec.confirmText,
title = spec.title,
+ content = { Text(spec.description) },
)
}Note: You'll need to import androidx.compose.material3.Text.
🤖 Prompt for AI Agents
In
@feature/main/src/main/java/com/example/metasearch/feature/main/MainActivity.kt
around lines 63 - 76, The dialog is missing its message body because
MetaSearchDialog isn't receiving the MetaSearchDialogSpec.description; update
the MetaSearchDialog call inside the dialogSpec.value?.let { spec -> ... } block
to pass the description as the composable content (e.g., supply a content = {
Text(spec.description) } or the appropriate content parameter on
MetaSearchDialog) and add the import androidx.compose.material3.Text so the
description is rendered.
| onDismissRequest = { | ||
| dialogSpec.value = null | ||
| }, |
There was a problem hiding this comment.
spec.onDismiss callback is not invoked.
The MetaSearchDialogSpec includes an onDismiss callback, but it's never called when the dialog is dismissed. This could lead to missed cleanup or state updates.
🐛 Proposed fix
onDismissRequest = {
+ spec.onDismiss()
dialogSpec.value = 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.
| onDismissRequest = { | |
| dialogSpec.value = null | |
| }, | |
| onDismissRequest = { | |
| spec.onDismiss() | |
| dialogSpec.value = null | |
| }, |
🤖 Prompt for AI Agents
In
@feature/main/src/main/java/com/example/metasearch/feature/main/MainActivity.kt
around lines 65 - 67, The dialog dismissal handler currently only clears
dialogSpec.value and never triggers the spec's onDismiss callback; update the
onDismissRequest lambda that sets dialogSpec.value = null to also invoke the
MetaSearchDialogSpec's onDismiss (e.g., call the selected spec's onDismiss
function or safe-invoke it) so any cleanup or state updates in the spec run when
the dialog is dismissed.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.