feat: Create Client and Loan Application in One Shot - #2623
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes refactor the client creation flow by renaming Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (2)
233-239:⚠️ Potential issue | 🔴 CriticalNavigation called as a side effect during composition — will fire on every recomposition.
navigateToCreateLoanAccount(uiState.id)is called directly inside thewhenbranch, which executes during composition. SinceSetClientIdpersists in theStateFlow, any recomposition (rotation, returning from the loan screen) will re-trigger navigation, potentially pushing duplicate screens onto the back stack.Wrap this in a
LaunchedEffectkeyed on the state to ensure it fires only once:🐛 Proposed fix
is CreateNewClientUiState.SetClientId -> { if (createClientWithImage) { uploadImage(uiState.id) } else { - navigateToCreateLoanAccount(uiState.id) + LaunchedEffect(uiState) { + navigateToCreateLoanAccount(uiState.id) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 233 - 239, The navigation call is happening during composition in CreateNewClientScreen: when handling CreateNewClientUiState.SetClientId the code calls navigateToCreateLoanAccount(uiState.id) (or uploadImage(uiState.id)) directly which will re-fire on every recomposition; wrap the side-effectful calls in a LaunchedEffect keyed on the unique id (e.g., uiState.id) or the state to ensure the action runs only once per new id—move the navigateToCreateLoanAccount(uiState.id) and uploadImage(uiState.id) calls into that LaunchedEffect block so composition no longer triggers navigation/upload repeatedly.
250-258:⚠️ Potential issue | 🔴 CriticalSame recomposition-safety issue: navigation + snackbar launched during composition.
Both
scope.launch { snackbar }andnavigateToCreateLoanAccount(uiState.clientId)are side effects that run on every recomposition. This will show duplicate snackbars and fire duplicate navigations.🐛 Proposed fix
is CreateNewClientUiState.OnImageUploadSuccess -> { - scope.launch { - snackbarHostState.showSnackbar( - message = getString(Res.string.feature_client_Image_Upload_Successful), - duration = SnackbarDuration.Long, - ) + LaunchedEffect(uiState) { + snackbarHostState.showSnackbar( + message = getString(Res.string.feature_client_Image_Upload_Successful), + duration = SnackbarDuration.Long, + ) + navigateToCreateLoanAccount(uiState.clientId) } - navigateToCreateLoanAccount(uiState.clientId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 250 - 258, The code is launching a snackbar and calling navigateToCreateLoanAccount during composition for CreateNewClientUiState.OnImageUploadSuccess which causes duplicate side-effects on recomposition; wrap the side-effects in a Compose side-effect such as LaunchedEffect keyed to the success event (e.g., LaunchedEffect(uiState) or LaunchedEffect(uiState.clientId) and check uiState is CreateNewClientUiState.OnImageUploadSuccess inside it), then call snackbarHostState.showSnackbar(...) and navigateToCreateLoanAccount(uiState.clientId) from within that LaunchedEffect so both the snackbarHostState.showSnackbar and navigateToCreateLoanAccount are executed exactly once when the success state appears instead of on every recomposition.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt (2)
155-160:⚠️ Potential issue | 🟠 MajorHardcoded
delay(1000)is fragile and has side effects on recomposition.Issues with this approach:
- On configuration change or process death,
StateFlowreplays the last emitted value (SetClientId), which will re-trigger navigation.- The 1000ms is arbitrary and races against snackbar display duration — on slow devices the snackbar may not even render before navigation fires.
- If the coroutine scope is cancelled during the delay (e.g., navigating away), the
SetClientIdemission is silently lost.Consider using a one-time navigation event (e.g., a
Channel/SharedFlowwithreplay=0) instead of encoding navigation triggers in theStateFlowstate, or at minimum let the UI drive the timing (e.g., navigate after snackbar is dismissed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt` around lines 155 - 160, The current use of delay(1000) between setting _createNewClientUiState to CreateNewClientUiState.ShowClientCreatedSuccessfully and then emitting CreateNewClientUiState.SetClientId is fragile: it races with snackbar display, is cancelled silently, and StateFlow replay will re-trigger navigation on restore. Replace this pattern with a one-time event channel or shared flow (e.g., a Channel or MutableSharedFlow(replay = 0)) for navigation events, emitting the client id into that one-time stream instead of encoding it in _createNewClientUiState; alternatively, let the UI call a ViewModel method (e.g., onSnackbarDismissed) to trigger emitting SetClientId so the UI drives the timing rather than a fixed delay.
165-167:⚠️ Potential issue | 🟠 MajorMissing UI state update in
catchblock — user sees an infinite spinner on failure.When
repository.createClient(clientPayload)throws, the error is parsed but the UI state remainsShowProgressbar. The user has no way to recover.🐛 Proposed fix
} catch (e: Exception) { - MFErrorParser.errorMessage(e) + _createNewClientUiState.value = + CreateNewClientUiState.ShowStringError( + MFErrorParser.errorMessage(e), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt` around lines 165 - 167, The catch block in CreateNewClientViewModel currently only calls MFErrorParser.errorMessage(e) and leaves the UI stuck in ShowProgressbar; update the catch to set the view model's UI state to hide the progress indicator and surface the error (e.g., set state from ShowProgressbar to an Error/Idle state containing MFErrorParser.errorMessage(e) and/or an error flag/message) so the spinner stops and the user can retry; locate the catch inside the coroutine that calls repository.createClient(clientPayload) and set the appropriate state backing property (e.g., _state or uiState) and/or call a method like showError(...) to apply the parsed message.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt (1)
29-29: Minor naming inconsistency:clientIdhere vs.idinSetClientId.
OnImageUploadSuccessusesclientId(line 29) whileSetClientIdusesid(line 39). Both carry the same client identifier. Consider aligning the naming for consistency across the sealed class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt` at line 29, Rename the parameter in the CreateNewClientUiState subclass to keep the client identifier name consistent: update OnImageUploadSuccess to use the same property name as SetClientId (e.g., change its parameter from clientId to id) and update any usages/places that construct or deconstruct OnImageUploadSuccess accordingly so all sealed subclasses (OnImageUploadSuccess and SetClientId) use the identical identifier name (id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt`:
- Around line 378-379: The onFinish handler currently uses
navController::popBackStack which returns the user to the completed Create
Client screen; change onFinish to navigate away from that screen instead (e.g.,
call navController.popBackStack to the client list or use navController.navigate
to the newly-created client's profile) so the Create Client screen is removed
from the back stack; update the component wiring where onFinish is passed (look
for the onFinish parameter usage and the call site that also invokes
navigateToCreateLoanAccount and the navController::popBackStack binding) to pop
back to the client list route or navigate forward to the client profile with the
created client ID rather than popping to the Create Client route.
- Around line 185-189: The call site passes
navController::navigateToNewLoanAccountRoute (signature (Int, String) -> Unit)
where createClientRoute expects a (Int) -> Unit; replace the method reference
with a lambda that matches (Int) -> Unit, e.g. navigateToCreateLoanAccount = {
clientId -> navController.navigateToNewLoanAccountRoute(clientId, /*accountNo*/
) }, and supply the appropriate accountNo (either derive it from the surrounding
scope/state or add a parameter to the callback path so the UI can pass the
account number) so createClientRoute and navigateToNewLoanAccountRoute
signatures align.
- Around line 377-381: Remove the duplicate registration of the NewLoanAccount
composable by deleting the extra call to newLoanAccountDestination(...) in
ClientNavigation.kt (the block that passes onNavigateBack =
navController::popBackStack, onFinish = navController::popBackStack,
navController = navController); keep the existing loanDestination(...) call
which already registers newLoanAccountDestination internally (do not touch
loanDestination implementation in LoanNavigation.kt).
---
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 233-239: The navigation call is happening during composition in
CreateNewClientScreen: when handling CreateNewClientUiState.SetClientId the code
calls navigateToCreateLoanAccount(uiState.id) (or uploadImage(uiState.id))
directly which will re-fire on every recomposition; wrap the side-effectful
calls in a LaunchedEffect keyed on the unique id (e.g., uiState.id) or the state
to ensure the action runs only once per new id—move the
navigateToCreateLoanAccount(uiState.id) and uploadImage(uiState.id) calls into
that LaunchedEffect block so composition no longer triggers navigation/upload
repeatedly.
- Around line 250-258: The code is launching a snackbar and calling
navigateToCreateLoanAccount during composition for
CreateNewClientUiState.OnImageUploadSuccess which causes duplicate side-effects
on recomposition; wrap the side-effects in a Compose side-effect such as
LaunchedEffect keyed to the success event (e.g., LaunchedEffect(uiState) or
LaunchedEffect(uiState.clientId) and check uiState is
CreateNewClientUiState.OnImageUploadSuccess inside it), then call
snackbarHostState.showSnackbar(...) and
navigateToCreateLoanAccount(uiState.clientId) from within that LaunchedEffect so
both the snackbarHostState.showSnackbar and navigateToCreateLoanAccount are
executed exactly once when the success state appears instead of on every
recomposition.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt`:
- Around line 155-160: The current use of delay(1000) between setting
_createNewClientUiState to CreateNewClientUiState.ShowClientCreatedSuccessfully
and then emitting CreateNewClientUiState.SetClientId is fragile: it races with
snackbar display, is cancelled silently, and StateFlow replay will re-trigger
navigation on restore. Replace this pattern with a one-time event channel or
shared flow (e.g., a Channel or MutableSharedFlow(replay = 0)) for navigation
events, emitting the client id into that one-time stream instead of encoding it
in _createNewClientUiState; alternatively, let the UI call a ViewModel method
(e.g., onSnackbarDismissed) to trigger emitting SetClientId so the UI drives the
timing rather than a fixed delay.
- Around line 165-167: The catch block in CreateNewClientViewModel currently
only calls MFErrorParser.errorMessage(e) and leaves the UI stuck in
ShowProgressbar; update the catch to set the view model's UI state to hide the
progress indicator and surface the error (e.g., set state from ShowProgressbar
to an Error/Idle state containing MFErrorParser.errorMessage(e) and/or an error
flag/message) so the spinner stops and the user can retry; locate the catch
inside the coroutine that calls repository.createClient(clientPayload) and set
the appropriate state backing property (e.g., _state or uiState) and/or call a
method like showError(...) to apply the parsed message.
---
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt`:
- Line 29: Rename the parameter in the CreateNewClientUiState subclass to keep
the client identifier name consistent: update OnImageUploadSuccess to use the
same property name as SetClientId (e.g., change its parameter from clientId to
id) and update any usages/places that construct or deconstruct
OnImageUploadSuccess accordingly so all sealed subclasses (OnImageUploadSuccess
and SetClientId) use the identical identifier name (id).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
f2b6c97 to
03c97be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/createNewClient/CreateNewClientScreen.kt (1)
233-258:⚠️ Potential issue | 🔴 CriticalNavigation and
uploadImagecalled directly during Composable composition — causes infinite re-navigation on back press.Lines 237, 257, and 235 invoke side effects (
navigateToCreateLoanAccount,uploadImage) directly inside awhenbranch that runs on every recomposition. Two concrete bugs result:
Infinite navigation loop:
navigateToCreateLoanAccountis called during composition. The ViewModel'sSetClientId/OnImageUploadSuccessstate is never reset after the call. When the user presses Back from the loan-account screen, theCreateNewClientScreenrecomposes with the same state → navigation fires again → the user is immediately pushed forward again, with no way to go back.Repeated
uploadImagecalls: ifselectedImageisnullat the time of composition (early return inuploadImagewithout a state change), the state staysSetClientIdanduploadImageis invoked on every recomposition until the image is actually set.The fix requires two coordinated changes:
In
CreateNewClientScreen— wrap both side effects inLaunchedEffect:🐛 Proposed fix — screen side
is CreateNewClientUiState.SetClientId -> { - if (createClientWithImage) { - uploadImage(uiState.id) - } else { - navigateToCreateLoanAccount(uiState.id) + LaunchedEffect(uiState.id) { + if (createClientWithImage) { + uploadImage(uiState.id) + } else { + navigateToCreateLoanAccount(uiState.id) + } } } ... is CreateNewClientUiState.OnImageUploadSuccess -> { scope.launch { snackbarHostState.showSnackbar( message = getString(Res.string.feature_client_Image_Upload_Successful), duration = SnackbarDuration.Long, ) } - navigateToCreateLoanAccount(uiState.clientId) + LaunchedEffect(uiState.clientId) { + navigateToCreateLoanAccount(uiState.clientId) + } }In
CreateNewClientViewModel— reset state after the navigation event is consumed (e.g., back toShowProgressbaror a dedicatedIdlestate) so that a back-navigation does not re-trigger the side effect:// After emitting SetClientId, reset to avoid re-trigger on back press _createNewClientUiState.value = CreateNewClientUiState.ShowProgressbarWithout the ViewModel reset, even
LaunchedEffectwill re-run in a new composition lifecycle (i.e., after back-navigation), so both halves of the fix are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 233 - 258, Wrap composition-only side effects in the Composable by moving calls to uploadImage(...) and navigateToCreateLoanAccount(...) into LaunchedEffect blocks inside CreateNewClientScreen so they only run when the relevant state value changes (use uiState.id or uiState.clientId as the key) instead of invoking them directly during when-branch composition; additionally, modify CreateNewClientViewModel to reset _createNewClientUiState after emitting terminal states (e.g., after emitting CreateNewClientUiState.SetClientId or OnImageUploadSuccess set _createNewClientUiState to CreateNewClientUiState.ShowProgressbar or a dedicated Idle state) so the event is consumed and back-navigation does not re-trigger the side effect.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt (1)
159-160: Extract the magic-number delay to a named constant.The
1000ms literal is unexplained. If the value ever needs to change (or be adjusted per platform), there's no single place to update it.♻️ Suggested refactor
+private const val CLIENT_CREATION_NAVIGATION_DELAY_MS = 1_000L ... - delay(1000) + delay(CLIENT_CREATION_NAVIGATION_DELAY_MS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt` around lines 159 - 160, Replace the hardcoded delay(1000) in CreateNewClientViewModel with a named constant (e.g., CLIENT_ID_DELAY_MS) so the purpose is clear and it can be adjusted per platform; declare the constant near the class (or companion object) and use it in place of 1000 where delay(...) is called before setting _createNewClientUiState to CreateNewClientUiState.SetClientId(it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt`:
- Line 187: The binding for navigateToCreateLoanAccount is calling
navController::navigateToNewLoanAccountRoute but that target requires two
parameters (fun NavController.navigateToNewLoanAccountRoute(clientId: Int,
accountNo: String)); change the assignment to a lambda that accepts both
clientId and accountNo and calls
navController.navigateToNewLoanAccountRoute(clientId, accountNo) so both
arguments are passed when invoking the navigation function.
---
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 233-258: Wrap composition-only side effects in the Composable by
moving calls to uploadImage(...) and navigateToCreateLoanAccount(...) into
LaunchedEffect blocks inside CreateNewClientScreen so they only run when the
relevant state value changes (use uiState.id or uiState.clientId as the key)
instead of invoking them directly during when-branch composition; additionally,
modify CreateNewClientViewModel to reset _createNewClientUiState after emitting
terminal states (e.g., after emitting CreateNewClientUiState.SetClientId or
OnImageUploadSuccess set _createNewClientUiState to
CreateNewClientUiState.ShowProgressbar or a dedicated Idle state) so the event
is consumed and back-navigation does not re-trigger the side effect.
---
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt`:
- Around line 159-160: Replace the hardcoded delay(1000) in
CreateNewClientViewModel with a named constant (e.g., CLIENT_ID_DELAY_MS) so the
purpose is clear and it can be adjusted per platform; declare the constant near
the class (or companion object) and use it in place of 1000 where delay(...) is
called before setting _createNewClientUiState to
CreateNewClientUiState.SetClientId(it).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt
- cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt
|
There is one issue with this file file:///home/runner/work/android-client/android-client/feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt:187:58 Inapplicable candidate(s): fun NavController.navigateToNewLoanAccountRoute(clientId: Int, accountNo: String): Unit |
|
@IOhacker yeah, it looks like changes were made recently in that file. I didn’t have those changes locally, so it was building fine for me. |
There was a problem hiding this comment.
Actionable comments posted: 1
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt (1)
37-41:⚠️ Potential issue | 🔴 CriticalFix compile-time signature mismatch at ClientNavigation.kt line 351
The function
navigateToNewLoanAccountRoutehas signature(clientId: Int, accountNo: String? = null), but at line 351 it's passed to a parameter expecting(Int, String) -> UnitwhereaccountNois non-nullable and required. Making the parameter optional breaks this contract.Either update the parameter type at line 26 of ClientApplyNewApplicationRoute.kt to
(Int, String?) -> Unit, or update the call site at line 351 to pass an adapter that supplies the required String parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt` around lines 37 - 41, The compile error comes from a function-type mismatch: navigateToNewLoanAccountRoute(clientId: Int, accountNo: String? = null) exposes a nullable optional accountNo but is being assigned where a handler of type (Int, String) -> Unit (non-nullable accountNo) is required (the consumer is ClientApplyNewApplicationRoute's callback). Fix by either changing the consumer signature in ClientApplyNewApplicationRoute from (Int, String) -> Unit to (Int, String?) -> Unit so it accepts nullable accountNo, or change the call site that provides navigateToNewLoanAccountRoute to wrap/adapter it into a (Int, String) -> Unit by supplying a non-null String (e.g., map null to "" or some default) before calling NewLoanAccountRoute; update references to the symbols navigateToNewLoanAccountRoute and ClientApplyNewApplicationRoute accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt`:
- Line 20: The nullable property accountNo in NewLoanAccountRoute is currently
declared as "val accountNo: String?" which makes it a required path parameter in
Compose Navigation; change its declaration to include a default "= null" so it
becomes an optional query parameter (e.g., "val accountNo: String? = null").
Update any constructors or usages that rely on positional routing to handle the
optional query form (look for NewLoanAccountRoute and any route
builders/arguments referencing accountNo) so navigation can omit the parameter
without producing an invalid URI.
---
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt`:
- Around line 37-41: The compile error comes from a function-type mismatch:
navigateToNewLoanAccountRoute(clientId: Int, accountNo: String? = null) exposes
a nullable optional accountNo but is being assigned where a handler of type
(Int, String) -> Unit (non-nullable accountNo) is required (the consumer is
ClientApplyNewApplicationRoute's callback). Fix by either changing the consumer
signature in ClientApplyNewApplicationRoute from (Int, String) -> Unit to (Int,
String?) -> Unit so it accepts nullable accountNo, or change the call site that
provides navigateToNewLoanAccountRoute to wrap/adapter it into a (Int, String)
-> Unit by supplying a non-null String (e.g., map null to "" or some default)
before calling NewLoanAccountRoute; update references to the symbols
navigateToNewLoanAccountRoute and ClientApplyNewApplicationRoute accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
591456c to
cf15cc1
Compare
f64ab59 to
b85ce6a
Compare
There was a problem hiding this comment.
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/createNewClient/CreateNewClientScreen.kt (1)
233-238:⚠️ Potential issue | 🟠 MajorGuard navigation/upload side effects against recomposition.
uploadImage(...)andnavigateToCreateLoanAccount(...)inSetClientIdare triggered directly during composition without guards. InOnImageUploadSuccess, thenavigateToCreateLoanAccount(uiState.clientId)call at line 256 is outside thescope.launchblock and also executes during composition. Since these states are not reset after emission, any recomposition will re-execute these side effects, risking duplicate uploads and navigation. Wrap them inLaunchedEffectkeyed on the state/id.♻️ Suggested fix using LaunchedEffect
is CreateNewClientUiState.SetClientId -> { - if (createClientWithImage) { - uploadImage(uiState.id) - } else { - navigateToCreateLoanAccount(uiState.id) - } + LaunchedEffect(uiState.id, createClientWithImage) { + if (createClientWithImage) { + uploadImage(uiState.id) + } else { + navigateToCreateLoanAccount(uiState.id) + } + } } is CreateNewClientUiState.OnImageUploadSuccess -> { - scope.launch { - snackbarHostState.showSnackbar( - message = getString(Res.string.feature_client_Image_Upload_Successful), - duration = SnackbarDuration.Long, - ) - } - navigateToCreateLoanAccount(uiState.clientId) + LaunchedEffect(uiState.clientId) { + snackbarHostState.showSnackbar( + message = getString(Res.string.feature_client_Image_Upload_Successful), + duration = SnackbarDuration.Long, + ) + navigateToCreateLoanAccount(uiState.clientId) + } }Also applies to: 250-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 233 - 238, The SetClientId branch currently calls uploadImage(uiState.id) or navigateToCreateLoanAccount(uiState.id) directly during composition (and OnImageUploadSuccess calls navigateToCreateLoanAccount(uiState.clientId) outside scope.launch), causing duplicate side-effects on recomposition; wrap these calls in LaunchedEffect keyed on the unique id/state (e.g., LaunchedEffect(uiState.id) or LaunchedEffect(uiState.clientId)) and move any suspend work (scope.launch logic) inside that LaunchedEffect so uploadImage(...) and navigateToCreateLoanAccount(...) are only invoked once per emitted state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 233-238: The SetClientId branch currently calls
uploadImage(uiState.id) or navigateToCreateLoanAccount(uiState.id) directly
during composition (and OnImageUploadSuccess calls
navigateToCreateLoanAccount(uiState.clientId) outside scope.launch), causing
duplicate side-effects on recomposition; wrap these calls in LaunchedEffect
keyed on the unique id/state (e.g., LaunchedEffect(uiState.id) or
LaunchedEffect(uiState.clientId)) and move any suspend work (scope.launch logic)
inside that LaunchedEffect so uploadImage(...) and
navigateToCreateLoanAccount(...) are only invoked once per emitted state.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
There was a problem hiding this comment.
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/createNewClient/CreateNewClientScreen.kt (1)
232-257:⚠️ Potential issue | 🟠 MajorNavigation side effects called directly in composition — will fire on every recomposition
Both
navigateToCreateLoanAccount(uiState.id)(line 236) andnavigateToCreateLoanAccount(uiState.clientId)(line 256) are invoked in the composition body, outside anyLaunchedEffect. While the state machine may make it rare, any recomposition triggered while theViewModelis still emitting eitherSetClientIdorOnImageUploadSuccess(e.g., due to a layout pass, a system-level configuration change, or a parent recomposing) will re-execute navigation, silently stacking duplicate destinations onto the back stack.The same pattern exists pre-existingly at lines 234 (
uploadImage), 241/250/260 (scope.launch), and 266 (navigateBack.invoke()), but the new navigation calls make this actively worse because corrupted back stacks are harder to recover from than repeated snackbars.The fix is to wrap the entire branch body in a
LaunchedEffectkeyed on the stable ID. Doing so inOnImageUploadSuccessalso lets the snackbar complete before navigating away, which is the intended UX:🐛 Proposed fix
is CreateNewClientUiState.SetClientId -> { - if (createClientWithImage) { - uploadImage(uiState.id) - } else { - navigateToCreateLoanAccount(uiState.id) + LaunchedEffect(uiState.id) { + if (createClientWithImage) { + uploadImage(uiState.id) + } else { + navigateToCreateLoanAccount(uiState.id) + } } } is CreateNewClientUiState.OnImageUploadSuccess -> { - scope.launch { - snackbarHostState.showSnackbar( - message = getString(Res.string.feature_client_Image_Upload_Successful), - duration = SnackbarDuration.Long, - ) - } - navigateToCreateLoanAccount(uiState.clientId) + LaunchedEffect(uiState.clientId) { + snackbarHostState.showSnackbar( + message = getString(Res.string.feature_client_Image_Upload_Successful), + duration = SnackbarDuration.Long, + ) + navigateToCreateLoanAccount(uiState.clientId) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 232 - 257, The navigation and other side effects in the CreateNewClientUiState.SetClientId and CreateNewClientUiState.OnImageUploadSuccess branches (calls to navigateToCreateLoanAccount(uiState.id / uiState.clientId), uploadImage(uiState.id), and snackbarHostState.showSnackbar) must be moved out of the direct composition and executed inside LaunchedEffect blocks keyed on the stable id (uiState.id or uiState.clientId); wrap each branch body in LaunchedEffect(key1 = uiState.id/clientId) and perform uploadImage, showSnackbar and then call navigateToCreateLoanAccount from inside that coroutine so the snackbar can complete before navigation and to prevent repeated navigation on recomposition.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
154-168: Parameter naming inconsistency between the route wrapper and the inner composable
CreateNewClientScreenRouteexposesnavigateToClientDetails: (Int) -> Unit(line 156), but it is threaded intoCreateNewClientScreenunder the namenavigateToCreateLoanAccount(lines 168, 189). "Navigate to client details" and "navigate to create loan account" describe different destinations — a reader of either function's signature gets contradictory information about where navigation actually leads.Pick one name that reflects the true destination (loan account creation) and apply it consistently at both levels.
♻️ Proposed rename
internal fun CreateNewClientScreenRoute( navigateBack: () -> Unit, - navigateToClientDetails: (Int) -> Unit, + navigateToCreateLoanAccount: (Int) -> Unit, hasDatatables: (datatables: List<DataTableEntity>, clientPayload: ClientPayloadEntity) -> Unit, viewmodel: CreateNewClientViewModel = koinViewModel(), ) { ... CreateNewClientScreen( ... - navigateToCreateLoanAccount = navigateToClientDetails, + navigateToCreateLoanAccount = navigateToCreateLoanAccount, ... ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 154 - 168, The route wrapper CreateNewClientScreenRoute currently takes a parameter named navigateToClientDetails but passes it into the inner composable CreateNewClientScreen as navigateToCreateLoanAccount, causing a naming inconsistency; decide the true destination is loan account creation, rename the parameter in CreateNewClientScreenRoute from navigateToClientDetails to navigateToCreateLoanAccount (and update all call sites) and also rename the parameter in CreateNewClientScreen signature if needed so both functions use the same name (navigateToCreateLoanAccount) and type ((Int)->Unit) for clear, consistent navigation intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 232-257: The navigation and other side effects in the
CreateNewClientUiState.SetClientId and
CreateNewClientUiState.OnImageUploadSuccess branches (calls to
navigateToCreateLoanAccount(uiState.id / uiState.clientId),
uploadImage(uiState.id), and snackbarHostState.showSnackbar) must be moved out
of the direct composition and executed inside LaunchedEffect blocks keyed on the
stable id (uiState.id or uiState.clientId); wrap each branch body in
LaunchedEffect(key1 = uiState.id/clientId) and perform uploadImage, showSnackbar
and then call navigateToCreateLoanAccount from inside that coroutine so the
snackbar can complete before navigation and to prevent repeated navigation on
recomposition.
---
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 154-168: The route wrapper CreateNewClientScreenRoute currently
takes a parameter named navigateToClientDetails but passes it into the inner
composable CreateNewClientScreen as navigateToCreateLoanAccount, causing a
naming inconsistency; decide the true destination is loan account creation,
rename the parameter in CreateNewClientScreenRoute from navigateToClientDetails
to navigateToCreateLoanAccount (and update all call sites) and also rename the
parameter in CreateNewClientScreen signature if needed so both functions use the
same name (navigateToCreateLoanAccount) and type ((Int)->Unit) for clear,
consistent navigation intent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
8a56c4f to
542f611
Compare
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
Code Review
Summary
This PR enhances the client creation workflow by navigating to the loan account creation screen after a client is successfully created, enabling a streamlined "one-shot" flow for creating both client and loan.
Code Walkthrough
| File | Changes |
|---|---|
AuthenticatedNavbarNavigationScreen.kt |
Simplified onNavigateToSearch callback to method reference |
CreateNewClientScreen.kt |
Renamed to CreateNewClientScreenRoute, added navigation callback |
CreateNewClientUiState.kt |
Added clientId parameter to OnImageUploadSuccess |
CreateNewClientViewModel.kt |
Added delay before navigation, moved init to init block |
ClientNavigation.kt |
Wired navigation to client profile route |
Findings
Major
-
CreateNewClientScreen.kt:233-237 - Side effects called directly during composition
The navigation calls
navigateToClientDetails(uiState.id)anduploadImage(uiState.id)are invoked directly in thewhenbranch during composition. If recomposition occurs while the ViewModel emitsSetClientId, navigation fires multiple times.Suggested Fix
is CreateNewClientUiState.SetClientId -> { LaunchedEffect(uiState.id) { if (createClientWithImage) { uploadImage(uiState.id) } else { navigateToClientDetails(uiState.id) } } }
-
CreateNewClientScreen.kt:250-257 - Same recomposition issue in
OnImageUploadSuccessnavigateToClientDetails(uiState.clientId)is called outsideLaunchedEffect, causing potential duplicate navigation.Suggested Fix
is CreateNewClientUiState.OnImageUploadSuccess -> { LaunchedEffect(uiState.clientId) { snackbarHostState.showSnackbar( message = getString(Res.string.feature_client_Image_Upload_Successful), duration = SnackbarDuration.Long, ) navigateToClientDetails(uiState.clientId) } }
Minor
- CreateNewClientViewModel.kt:159 - Extract magic number
delay(1000)to a named constant likeNAVIGATION_DELAY_MS
Architecture Compliance
| Check | Status |
|---|---|
| MVI Pattern | ✅ Uses StateFlow + sealed UiState |
| Single Responsibility | ✅ Screen, ViewModel, Navigation separated |
Test Coverage
- No test changes included - consider adding ViewModel tests for new state transitions
Security
- No hardcoded secrets
- No injection vulnerabilities
Recommendation: Address the LaunchedEffect issues before merge to prevent navigation bugs on recomposition.
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
Code Review
Summary
This PR enhances the client creation flow by navigating directly to the newly created client's details page instead of just going back. It also moves the loadOfficeAndClientTemplate() call from LaunchedEffect to ViewModel's init block.
Code Walkthrough
-
CreateNewClientScreen.kt (UI Layer)
- Renamed
CreateNewClientScreen→CreateNewClientScreenRoutefor clarity - Added
navigateToClientDetails: (Int) -> Unitcallback - After client creation, navigates to client details instead of going back
- Removed
LaunchedEffectfor loading data (moved to ViewModel init)
- Renamed
-
CreateNewClientViewModel.kt (ViewModel)
- Moved
loadOfficeAndClientTemplate()toinitblock - Added
delay(1000)before transitioning toSetClientIdstate - Added
clientIdparameter toOnImageUploadSuccessstate
- Moved
-
CreateNewClientUiState.kt (State)
OnImageUploadSuccessnow includesclientId: Intfor navigation
-
ClientNavigation.kt (Navigation)
- Added
navigateToClientDetailsparameter tocreateClientRoute - Wired navigation to
navigateToClientDetailsProfileRoute
- Added
-
AuthenticatedNavbarNavigationScreen.kt (Minor)
- Simplified lambda to method reference
Findings Overview
Major [2 issues]
-
🟠 CreateNewClientScreen.kt:232-237 [85% confidence]
Navigation is called during composition in theSetClientIdwhen branch. This will cause navigation to be triggered on every recomposition. Should be wrapped inLaunchedEffect. -
🟠 CreateNewClientScreen.kt:249-257 [85% confidence]
Same issue inOnImageUploadSuccessbranch -navigateToClientDetails(uiState.clientId)is called during composition, not in a side-effect handler.
Minor [1 issue]
- 🟡 CreateNewClientViewModel.kt:159 [75% confidence]
Thedelay(1000)is a hardcoded magic number. Consider extracting to a named constant likeCLIENT_CREATION_SUCCESS_DELAY_MSfor clarity and maintainability.
Suggested Fix for Navigation Issue
// Current (problematic):
is CreateNewClientUiState.SetClientId -> {
if (createClientWithImage) {
uploadImage(uiState.id)
} else {
navigateToClientDetails(uiState.id) // ❌ Called during composition
}
}
// Recommended:
is CreateNewClientUiState.SetClientId -> {
LaunchedEffect(uiState.id) {
if (createClientWithImage) {
uploadImage(uiState.id)
} else {
navigateToClientDetails(uiState.id) // ✅ Called as side-effect
}
}
}Test Coverage
- Unit tests for navigation callback
- Test that
SetClientIdstate triggers navigation exactly once
Security
- No hardcoded secrets
- No injection vulnerabilities
- No sensitive data logging
Related Files Analyzed
| File | Purpose |
|---|---|
| ClientNavigation.kt | Verified navigation destination exists |
| CreateNewClientUiState.kt | Confirmed state changes |
| AuthenticatedNavbarNavigationScreen.kt | Verified navigation wiring |
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
Good improvements to the client creation flow! The navigation to client details instead of going back is a better UX.
A couple of things to fix:
-
Navigation during composition - The
navigateToClientDetails()calls in bothSetClientIdandOnImageUploadSuccessbranches are executing during composition. This will trigger on every recomposition - see my inline comments for the fix. -
Minor: The
delay(1000)in ViewModel could use a named constant likeCLIENT_CREATION_SUCCESS_DELAY_MS.
The ViewModel refactoring (moving loadOfficeAndClientTemplate() to init) is cleaner. Navigation wiring looks correct.
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
The changes look good overall - navigating to client details instead of just going back is a better UX.
Still needs fixing:
- The navigation calls at lines 236 and 256 are still being called during composition. I've left inline comments with the fix - wrap them in
LaunchedEffectto ensure they only fire once.
The ViewModel refactoring (moving loadOfficeAndClientTemplate() to init) is cleaner.
b12c08b to
216d3f4
Compare
|
|
@therajanmaurya this PR looks fine from a functional perspective! |
6 participants
Fixes - https://mifosforge.jira.com/browse/MIFOSAC-707
Summary by CodeRabbit