fix: Client profile photo and details not refreshing after update #2570
Conversation
📝 WalkthroughWalkthroughAdds a repository-level Flow and trigger API for client update events; edit flows emit triggers after changes; profile ViewModels subscribe to refresh details/images; profile image edit flow shows a ResultStatus dialog; MifosStatusDialog layout and strings updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditVM as Edit ViewModel
participant Repo as ClientDetailsRepository
participant DetailVM as Profile Details VM
participant UI as UI Layer
User->>EditVM: Submit update / upload / delete
EditVM->>Repo: perform update/upload/delete
Repo-->>EditVM: operation result
EditVM->>Repo: triggerClientUpdate()
Note over Repo: emits Unit on clientUpdateEvents
Repo-->>DetailVM: event emitted
DetailVM->>Repo: loadClientDetailsAndImage(id)
Repo-->>DetailVM: updated data
DetailVM->>UI: refresh UI / present status dialog
UI-->>User: updated profile / dialog shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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: 1
Fix all issues with AI Agents 🤖
In
@core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientDetailsRepository.kt:
- Around line 76-77: The MutableSharedFlow used for client updates currently
uses replay=0 and will suspend emitters when there are no collectors; update the
backing flow declaration (_clientUpdateEvents) to add extraBufferCapacity = 1 so
emits from triggerClientUpdate() don’t suspend, i.e., modify the
MutableSharedFlow<Unit> construction to include extraBufferCapacity = 1 and keep
the exposed clientUpdateEvents Flow and triggerClientUpdate() logic unchanged.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt (1)
151-168: Hardcoded success message should be externalized for i18n.The message
"Profile photo updated successfully"on line 165 should be a string resource for localization support.Additionally, the intermediate
dialogState = Loadingat line 154 before the finalShowStatusDialogat line 163 may cause a brief visual flicker. Consider settingShowStatusDialogdirectly after the async operations complete.🔎 Proposed fix
is DataState.Success -> { - mutableStateFlow.update { - it.copy( - dialogState = ClientProfileEditState.DialogState.Loading, - openImagePicker = false, - ) - } clientDetailsRepo.triggerClientUpdate() loadImage(route.id) mutableStateFlow.update { it.copy( openImagePicker = false, dialogState = ClientProfileEditState.DialogState.ShowStatusDialog( status = ResultStatus.SUCCESS, - msg = "Profile photo updated successfully", + msg = "Profile photo updated successfully", // TODO: Use string resource ), ) } }
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.kt (1)
68-74: Consider extracting the duplicated observer pattern.This
observeGlobalUpdates()implementation is identical to the one inClientProfileViewModel.kt(lines 51-57). Consider extracting this pattern to a base class method or extension function to follow the DRY principle.💡 Potential refactor approach
One option is to add a protected helper in
BaseViewModel:// In BaseViewModel or a shared utility protected fun ViewModel.observeClientUpdates( clientUpdateEvents: Flow<Unit>, onUpdate: suspend () -> Unit ) { viewModelScope.launch { clientUpdateEvents.collect { onUpdate() } } }Then in each ViewModel:
init { getClientAndObserveNetwork() observeClientUpdates(clientDetailsRepo.clientUpdateEvents) { loadClientDetailsAndImage(route.id) } }core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
106-119: Consider adding a preview for the FAILURE state.Currently only
ResultStatus.SUCCESSis previewed. Adding a second preview forResultStatus.FAILUREwould help verify the error icon and messaging render correctly.🔎 Proposed addition
@Composable @Preview private fun MifosFailureStatusDialogPreview() { MifosTheme { MifosStatusDialog( status = ResultStatus.FAILURE, onConfirm = {}, btnText = "Retry", successTitle = "Success", successMessage = "Profile photo updated successfully", failureTitle = "Failure", failureMessage = "Failed to update profile photo", ) } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt (1)
174-195: Consider adding loading state during delete and externalizing error message.
- The delete operation doesn't show a loading indicator while the network request is in progress, unlike upload which shows
Loadingstate.- The fallback
"Unknown Error"on line 189 should be a string resource for i18n.🔎 Proposed improvement
fun deleteClientImage() { viewModelScope.launch { + mutableStateFlow.update { + it.copy(dialogState = ClientProfileEditState.DialogState.Loading) + } try { clientDetailsRepo.deleteClientImage(state.id) clientDetailsRepo.triggerClientUpdate() mutableStateFlow.update { it.copy( profileImage = null, dialogState = null, ) } } catch (e: Exception) { mutableStateFlow.update { it.copy( dialogState = ClientProfileEditState.DialogState.Error( - e.message ?: "Unknown Error", + e.message ?: "Unknown Error", // TODO: Use string resource ), ) } } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientDetailsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(34-99)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileViewModel.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.kt (2)
observeGlobalUpdates(68-74)loadClientDetailsAndImage(86-131)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileViewModel.kt (2)
observeGlobalUpdates(51-57)loadClientDetailsAndImage(69-114)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosButton.kt (2)
MifosButton(52-78)MifosButton(91-117)
⏰ 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 Checks / Static Analysis Check
🔇 Additional comments (10)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.kt (1)
65-66: LGTM! Correct initialization of update observer.The addition of
observeGlobalUpdates()in the init block properly sets up the event listener alongside the existing initialization logic.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileViewModel.kt (1)
49-57: LGTM! Event listener correctly implemented.The
observeGlobalUpdates()method properly sets up the listener for client update events and triggers data refresh. The implementation is correct and follows the same pattern asClientProfileDetailsViewModel.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
270-294: LGTM! Status dialog implementation is correct.The
ShowStatusDialogimplementation properly displays success/failure feedback using theMifosStatusDialogcomponent with appropriate wrapping and action handling.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsViewModel.kt (2)
42-42: LGTM! Repository dependency correctly added.The
clientDetailsRepoparameter properly extends the ViewModel's dependencies to support triggering update events.
159-159: ClientEditDetailsViewModel does not handle profile photo uploads.This ViewModel is specifically for editing client basic details (name, office, staff, etc.). Profile photo uploads are handled by
ClientProfileEditViewModel, which already properly invokesclientDetailsRepo.triggerClientUpdate()at line 158 of itsuploadImage()method and at line 178 of itsdeleteClientImage()method.Likely an incorrect or invalid review comment.
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.kt (1)
36-42: LGTM! Clean event broadcast implementation.The use of
MutableSharedFlow<Unit>(replay = 0)withasSharedFlow()is idiomatic for an event bus pattern. Note that withreplay = 0, iftriggerClientUpdate()is called before any collectors are active, the event will be lost. This is typically acceptable for UI refresh signals where collectors are already observing before updates occur.core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
58-98: LGTM! Clean and well-structured dialog component.The Column-based layout with DesignToken spacing is consistent with the design system. The destructuring pattern for status-based content selection is readable and maintainable.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt (3)
91-91: LGTM!Clean action handling that delegates to the existing
NavigateBackevent.
100-123: Preserving status dialog across image load states is correctly implemented.The conditional logic to preserve
ShowStatusDialogacrossSuccess,Loading, andErrorstates prevents the image reload from overwriting the confirmation dialog. While this adds complexity, it correctly handles the async nature ofloadImage()being called after settingShowStatusDialoginuploadImage().
207-213: LGTM!The
ShowStatusDialogstate properly encapsulates the status and message for the dialog, fitting cleanly into the existing sealed interface pattern.
| val clientUpdateEvents: Flow<Unit> | ||
| suspend fun triggerClientUpdate() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*ClientDetailsRepositoryImp.kt" | head -20Repository: openMF/android-client
Length of output: 162
🏁 Script executed:
rg -A 10 -B 2 "clientUpdateEvents|triggerClientUpdate" --type=kotlinRepository: openMF/android-client
Length of output: 12498
Add extraBufferCapacity = 1 to the SharedFlow configuration to prevent suspension on emit.
The implementation uses MutableSharedFlow<Unit>(replay = 0) but is missing the extraBufferCapacity = 1 parameter. Without an extra buffer capacity, emit() will suspend if there are no active collectors, which can cause unexpected blocking behavior. Configure it as:
Suggested fix
private val _clientUpdateEvents = MutableSharedFlow<Unit>(replay = 0, extraBufferCapacity = 1)🤖 Prompt for AI Agents
In
@core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientDetailsRepository.kt
around lines 76-77, The MutableSharedFlow used for client updates currently uses
replay=0 and will suspend emitters when there are no collectors; update the
backing flow declaration (_clientUpdateEvents) to add extraBufferCapacity = 1 so
emits from triggerClientUpdate() don’t suspend, i.e., modify the
MutableSharedFlow<Unit> construction to include extraBufferCapacity = 1 and keep
the exposed clientUpdateEvents Flow and triggerClientUpdate() logic unchanged.
| } | ||
|
|
||
| is ClientProfileEditState.DialogState.ShowStatusDialog -> { | ||
| androidx.compose.ui.window.Dialog( |
| openImagePicker = false, | ||
| dialogState = ClientProfileEditState.DialogState.ShowStatusDialog( | ||
| status = ResultStatus.SUCCESS, | ||
| msg = "Profile photo updated successfully", |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt (2)
155-175: Race condition betweenloadImageand status dialog update.After calling
loadImage(route.id)on line 163, the state is immediately updated withShowStatusDialogon lines 164-172. SinceloadImagelaunches a coroutine that collects a Flow and updates state, there's a potential race. The preservation logic inloadImage(lines 104-111) handles this correctly by checking ifdialogStateisShowStatusDialogbefore overwriting. This works but is fragile.Consider updating state with the status dialog before calling
loadImage, or restructure to make the ordering explicit:Suggested refactor
is DataState.Success -> { + clientDetailsRepo.triggerClientUpdate() mutableStateFlow.update { it.copy( - dialogState = ClientProfileEditState.DialogState.Loading, openImagePicker = false, + dialogState = ClientProfileEditState.DialogState.ShowStatusDialog( + status = ResultStatus.SUCCESS, + msg = getString(Res.string.client_profile_photo_updated_success), + ), ) } - clientDetailsRepo.triggerClientUpdate() loadImage(route.id) - mutableStateFlow.update { - it.copy( - openImagePicker = false, - dialogState = ClientProfileEditState.DialogState.ShowStatusDialog( - status = ResultStatus.SUCCESS, - msg = getString(Res.string.client_profile_photo_updated_success), - ), - ) - } }
178-199: Consider makingdeleteClientImageprivate.This function is only called internally from
handleActionviaOnConfirmDeleteClick. Making itprivatewould better encapsulate the ViewModel's internal logic.Suggested change
- fun deleteClientImage() { + private fun deleteClientImage() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.ktfeature/client/src/commonMain/composeResources/values/strings.xmlfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.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 Checks / Static Analysis Check
🔇 Additional comments (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.kt (1)
37-43: LGTM! Well-designed event signaling pattern.The
MutableSharedFlowconfiguration withreplay = 0,extraBufferCapacity = 1, andDROP_OLDESTis appropriate for fire-and-forget UI refresh signals. UsingasSharedFlow()correctly prevents external emission.feature/client/src/commonMain/composeResources/values/strings.xml (1)
473-474: LGTM!The new string resources follow the existing naming conventions and provide appropriate user feedback for photo update operations.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt (2)
92-96: LGTM!The
OnImageSelectedtriggers upload andOnNextcorrectly navigates back after the status dialog is dismissed, completing the user flow.
211-217: LGTM!The
ShowStatusDialogstate correctly encapsulates both the status type and message for displaying user feedback after photo operations.
|
@kartikey004 I noticed that you didn't navigate through the MifosBreadCrumb; you navigated through the back |
@ufuomaisaac I used the back arrow only for the demo but the repository-level broadcast does not depend on navigation. since the Profile Screen listens directly to the data layer, it refreshes automatically no matter how the user goes back whether it’s the breadcrumb, the hardware back button or any other way. |
|
Fairs |
|
@kartikey004 Can you tell me if we really need a continue button in the success dialog? We don't have any other options for the user to interact with. We could simply load the screen details page after the dialog is briefly displayed. Let me know your thoughts on this. 
|
@biplab1 I understand your point. this can reduce an extra step. but auto closing a dialog can feel a bit abrupt since it appears and disappears quickly and some users may miss the message. also different users read at different speeds so its hard to decide an optimal auto-close time for the dialog. |
@kartikey004 A continue button makes sense for error dialogs, where users may need time to read and acknowledge the message. For success messages, since we already show a success icon along with |
That makes sense. I'll remove the button and add a short delay before it navigates. |
|
@biplab1 is this flow correct for success dialog? autoClosing_statusDialog.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (3)
29-29: Remove unused import.The
alphamodifier is imported but never used in the code.♻️ Remove unused import
-import androidx.compose.ui.draw.alpha
63-68: Consider extracting the delay duration to a constant.The hardcoded
1500milliseconds makes the timing less discoverable and harder to adjust consistently across the codebase if needed.♻️ Extract delay to a named constant
At the top of the file or in DesignToken:
private const val SUCCESS_DIALOG_AUTO_DISMISS_DELAY_MS = 1500LThen use it:
if (status == ResultStatus.SUCCESS) { LaunchedEffect(Unit) { - delay(1500) + delay(SUCCESS_DIALOG_AUTO_DISMISS_DELAY_MS) onConfirm() } }
78-82: Consider adding a content description for accessibility.The image has
contentDescription = null, which means screen readers will skip it. If the icon provides additional context beyond the text (e.g., visually reinforcing success/failure), consider adding a descriptive label.However, if the icon is purely decorative and the title/message fully convey the status, the current approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.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 Checks / Static Analysis Check
🔇 Additional comments (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
94-94: Improved readability with larger text style.Changing from
bodySmalltobodyMediumenhances message legibility, especially for longer status messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (2)
62-67: Consider making the auto-close delay configurable.The 1500ms delay works for this use case, but consider exposing it as a parameter with a default value for reusability in other contexts where different timing may be appropriate.
♻️ Optional: Make delay configurable
fun MifosStatusDialog( status: ResultStatus, onConfirm: () -> Unit, btnText: String, successTitle: String, successMessage: String, failureTitle: String, failureMessage: String, modifier: Modifier = Modifier, + successAutoDismissDelayMs: Long = 1500L, ) { // ... if (status == ResultStatus.SUCCESS) { LaunchedEffect(Unit) { - delay(1500) + delay(successAutoDismissDelayMs) onConfirm() } }
77-81: Add contentDescription for accessibility.The status icon conveys meaningful information. While the title text provides context, adding a
contentDescriptionimproves screen reader support.♻️ Suggested fix
Image( painter = icon, - contentDescription = null, + contentDescription = title, modifier = Modifier.size(DesignToken.sizes.profile), )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.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 Checks / Static Analysis Check
🔇 Additional comments (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (2)
98-113: LGTM!The conditional rendering correctly implements the agreed-upon behavior: SUCCESS status auto-closes after delay (no button needed), while FAILURE requires explicit user acknowledgment via the button. The spacer maintains layout consistency.
117-136: Implementation aligns with PR objectives.The
ResultStatusenum and updated preview reflect the discussed changes. The auto-close behavior for success dialogs and manual button for failure dialogs match the requirements from the PR discussion.
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Please address the requested changes below.
There was a problem hiding this comment.
Can you tell me why the core UI components was changed? Core components shouldn't be touched unless there is a global change required wherever the component is used.
There was a problem hiding this comment.
Can you tell me why the core UI components was changed? Core components shouldn't be touched unless there is a global change required wherever the component is used.
I updated the core component because I thought the auto-closing dialog as a global UX improvement rather than just for this specific screen.
now I understand the requirement to keep core components purely presentational. I will add a showButton parameter instead allowing the feature layer to decide the behavior without enforcing it globally.
| if (status == ResultStatus.SUCCESS) { | ||
| LaunchedEffect(Unit) { | ||
| delay(1500) | ||
| onConfirm() | ||
| } | ||
| } |
There was a problem hiding this comment.
The delay should be added at the feature level and not at the core level. Any customization required for a particular outcome should be handled at the feature level unless we require that change everywhere, in which case, the core components can be updated. Please review.
| observeGlobalUpdates() | ||
| } | ||
|
|
||
| private fun observeGlobalUpdates() { |
There was a problem hiding this comment.
Can you tell me thought behind using this name observerGlobalUpdates. It doesn't clearly say anything about the kind of update it is handling.
There was a problem hiding this comment.
Initially I thought that maybe in the future we can use this function to update clients from anywhere in the application, from any screen. hence kept it as observeGlobalUpdates.
I'll rename it, I think observeClientUpdates is clearer.
| onConfirm = { | ||
| onAction(ClientProfileEditAction.OnNext) | ||
| }, | ||
| btnText = stringResource(Res.string.dialog_continue), |
There was a problem hiding this comment.
Please follow the naming convention for the string resources. The naming convention of the string resource should be of the following format:
[feature_name]_[string_resource_name]
Say, we are working with client charges and the string resource is "No charges available". Then the name of string resource should be:
client_charges_no_charges_available
There was a problem hiding this comment.
I used dialog_continue because it was previously defined in the strings.xml. I reused the existing resource.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt:
- Around line 273-304: The dialog currently always calls
onAction(ClientProfileEditAction.OnNext) from onDismissRequest which lets users
skip the 1500ms success display and dismiss failures without hitting the CTA;
update the Dialog to use DialogProperties and conditional onDismissRequest based
on state.dialogState.status (e.g., disable outside/back dismiss for
ResultStatus.FAILURE and optionally for SUCCESS until the 1500ms has elapsed),
make the LaunchedEffect tied to the dialog visibility (scoped to
ClientProfileEditState.DialogState.ShowStatusDialog) so it cancels when the
dialog is dismissed, and ensure ClientProfileEditAction.OnNext is
idempotent/safe to call multiple times; also replace the unrelated
client_identifiers_* strings with the profile-photo-specific string resources in
MifosStatusDialog (status, successTitle/successMessage,
failureTitle/failureMessage) so copy matches the action.
🧹 Nitpick comments (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
111-114:ResultStatusenum location: keep an eye on module layering if it spreads beyond UI.If
ResultStatusstays purely UI-facing,core/uiis fine. If you later need it in repositories / domain models, consider moving it to a lower-level module (e.g., core/model) to avoid dependency inversion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(38-109)
⏰ 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 Checks / Static Analysis Check
🔇 Additional comments (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (2)
39-49:showButton+ layout refactor looks solid and enables the auto-close UX cleanly.The new
showButtonflag (Line 47) and the simplified single-icon layout (Line 63-90) make it straightforward for callers to present a “toast-like” success dialog without an extra CTA, while preserving the explicit action for failure states.Also applies to: 63-108
120-128: No additional concerns with the preview updates.
.../src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
103-105: Consider removing the spacer when button is hidden.When
showButtonisfalse, a spacer is added to maintain layout height. However, the parentColumnalready usesverticalArrangement = Arrangement.spacedBy(DesignToken.spacing.large)(line 67), which should handle spacing consistently. The spacer may be redundant and could cause inconsistent spacing between success and failure states.♻️ Proposed simplification
- if (showButton) { + if (showButton) { MifosButton( onClick = onConfirm, text = { Text( text = btnText, style = MifosTypography.labelLarge, ) }, modifier = Modifier .fillMaxWidth() .padding(top = DesignToken.padding.small), ) - } else { - Spacer(modifier = Modifier.height(DesignToken.spacing.large)) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(36-107)
⏰ 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 Checks / Static Analysis Check
🔇 Additional comments (5)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (3)
45-45: LGTM! Good design for conditional button rendering.The
showButtonparameter with a default value oftruemaintains backward compatibility while enabling the success dialog auto-dismiss flow.
109-112: LGTM! Enum adds type safety.The
ResultStatusenum provides clear, type-safe status values and is appropriately scoped to this component.
61-88: Layout change from Box to Column is compatible with existing usages.All identified call sites properly pass the required parameters and use the modifier correctly. The Column layout with
fillMaxWidth()andwrapContentHeight()modifiers is compatible with theModifier.fillMaxSize()passed by callers, and spacing/alignment are properly configured. No rendering issues detected across the 12+ usages in the codebase.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (2)
273-304: Auto-dismiss timing is brief but aligns with PR discussion.The implementation looks correct:
- SUCCESS status auto-dismisses after 1500ms (lines 274-279)
- FAILURE status shows a manual "Continue" button (line 300)
- Users can manually dismiss SUCCESS dialog by tapping outside (line 281)
The 1500ms delay for reading the success message may be brief for some users, especially those with accessibility needs or slower reading speeds. However, based on the PR comments, this trade-off was explicitly discussed and agreed upon.
If accessibility is a concern in the future, consider:
- Increasing the delay to 2000-2500ms
- Respecting system accessibility settings for extended timeouts
- Adding haptic/sound feedback to complement the visual notification
274-279: LaunchedEffect key is correct for one-time execution.Using
Unitas the key ensures the delay and auto-dismiss logic runs exactly once when theShowStatusDialogstate is entered, which is the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt:
- Around line 17-18: The screen imports and reuses client_identifiers_error_text
and client_identifiers_identities_success_text for the profile photo/edit status
dialog, which is the wrong string family; update ClientProfileEditScreen to use
the dedicated profile-photo/edit success and error strings (or add neutral
"Success"/"Error" titles to the client feature resources) instead of
client_identifiers_*; change the imports at the top (remove
client_identifiers_error_text and client_identifiers_identities_success_text)
and replace all usages of those identifiers in the status/dialog code paths
(including the other occurrences referenced) to the appropriate profile-specific
resource identifiers.
- Around line 274-280: The LaunchedEffect currently uses a constant key
(LaunchedEffect(Unit)) inside the
ClientProfileEditState.DialogState.ShowStatusDialog branch, so if
state.dialogState.status transitions from FAILURE to SUCCESS while still showing
ShowStatusDialog the effect won't restart and the auto-dismiss (delay +
onAction(ClientProfileEditAction.OnNext)) may never run; change the effect to
key off state.dialogState.status (e.g.,
LaunchedEffect(state.dialogState.status)) so the coroutine restarts when status
becomes SUCCESS, keeping the auto-dismiss behavior reliable and avoiding a stuck
dialog.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
281-308: Make the modal’s dismissal semantics explicit (even if you keep it non-dismissable).Right now
onDismissRequest = {}plusdismissOnBackPress/ClickOutside=falseis an intentional “blocking” modal. Consider at least wiringonDismissRequesttoOnNextto avoid edge-case traps on platforms where dismiss might still be invoked (or future property changes).Example tweak
Dialog( - onDismissRequest = {}, + onDismissRequest = { onAction(ClientProfileEditAction.OnNext) }, properties = DialogProperties( dismissOnBackPress = false, dismissOnClickOutside = false, ), ) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(36-107)
⏰ 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 Checks / Static Analysis Check
.../src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
272-308: Implementation looks correct; consider extracting the delay constant.The auto-close logic using
LaunchedEffectis well-structured—the effect properly cancels if the dialog state changes before completion. The non-dismissable dialog with conditional button display aligns with the PR requirements.Minor suggestion: extract the hardcoded
1500to a named constant for clarity and easier tuning.♻️ Suggested refactor
+private const val STATUS_DIALOG_AUTO_CLOSE_DELAY_MS = 1500L // Inside the when branch: LaunchedEffect(state.dialogState.status) { if (state.dialogState.status == ResultStatus.SUCCESS) { - delay(1500) + delay(STATUS_DIALOG_AUTO_CLOSE_DELAY_MS) onAction(ClientProfileEditAction.OnNext) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/client/src/commonMain/composeResources/values/strings.xmlfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/composeResources/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(36-107)
⏰ 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 Checks / Static Analysis Check
🔇 Additional comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
17-18: LGTM!All new imports are necessary for the status dialog functionality and follow the existing import organization pattern.
Also applies to: 22-22, 41-41, 44-44, 51-52, 66-66, 68-68, 70-70
4 participants
Fixes - Jira-#595
@ufuomaisaac
Worked on the initial implementation of the fix, handled the navigation-based refresh logic and added the status dialog for profile photo updates. His work helped shape the final solution.
Description - The client profile was not updating when navigating back from the edit screen. I implemented a repository-level broadcast flow so the profile screen listens for update signals and refreshes automatically.
Also added a status dialog box after photo uploads to confirm the changes.
Video -
Update_ClientProfile.mp4
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.