Skip to content

fix(client): resolve create new client logical issues and migrate to BaseViewModel#2655

Open
revanthkumarJ wants to merge 14 commits into
openMF:devfrom
revanthkumarJ:revanth/create_client_revamp
Open

fix(client): resolve create new client logical issues and migrate to BaseViewModel#2655
revanthkumarJ wants to merge 14 commits into
openMF:devfrom
revanthkumarJ:revanth/create_client_revamp

Conversation

@revanthkumarJ

@revanthkumarJ revanthkumarJ commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Fixes - Jira-#754

Before:

screen-20260317-223819.mp4

After:

screen-20260318-234014.mp4

Summary by CodeRabbit

  • Refactor

    • Consolidated client-creation UI into a single state-driven, unidirectional flow and renamed the creation screen for a simpler, consistent experience.
  • New Features

    • Centralized event-driven handling for navigation, snackbars, and datatables callbacks.
    • Overlay progress indicator for long-running operations.
    • Helper to merge loading states from multiple data sources.
  • Bug Fixes

    • Date picker now restricts to past dates.
    • Improved image selection, preview, removal, and submit behavior.
    • Address validation runs only when address input is enabled.

@coderabbitai

coderabbitai Bot commented Mar 18, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaces sealed UI-state branching with a unidirectional state/action/event architecture for the Create New Client flow: removed CreateNewClientUiState, introduced CreateNewClientState, CreateNewClientAction, and CreateNewClientEvent; ViewModel moved to BaseViewModel pattern and composable now accepts state, snackbarHostState, and onAction.

Changes

Cohort / File(s) Summary
Screen / Composables
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt
Replaced legacy UI-state branching with CreateNewClientScreen(state, snackbarHostState, onAction) driven by state.screenState; moved to event-driven side effects via EventsEffect(viewModel.eventFlow); form inputs dispatch CreateNewClientAction.*; overlay/loading controlled by state.showOverLayProgressIndicator; date/image pickers and image removal now dispatch actions; image preview uses toImageBitmap/produceState.
ViewModel & State model
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt
Converted to BaseViewModel<CreateNewClientState, CreateNewClientEvent, CreateNewClientAction>; centralized initial loading with combineDataState(...); single CreateNewClientState container and handleAction updates; added validation, payload creation, submit flow, event emissions (snackbar, navigation, HasDatatables), and overlay-driven image upload flow.
Removed legacy sealed state
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt
Deleted sealed CreateNewClientUiState and all nested UI variants (progress, error, success, upload states).
Navigation API rename / call sites
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
Renamed composable/type CreateNewClientScreenRouteCreateNewClientScreen and updated NavGraphBuilder usage to call the new composable.
Utility added
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CombineDataState.kt
Added combineDataState helper to merge two Flow<DataState<...>> sources into a single DataState flow with precedence rules (Loading/Error) and a success transform.
Signatures, validation & previews
various files under feature/client/.../createNewClient
Validation now takes isAddressEnabled; address validation runs only when enabled; activation date/selectability logic uses formState.activationDate and utcTimeMillis <= now; previews updated to use CreateNewClientState.ScreenState.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant Screen as CreateNewClientScreen
    participant VM as CreateNewClientViewModel
    participant Repo as Repository/API/DB
    participant Nav as Navigation

    User->>Screen: fill form / pick date / pick image / submit
    Screen->>VM: onAction(UpdateField / TogglePicker / SelectImage / CreateClient)
    VM->>Repo: fetch templates/offices OR createClient(payload) OR uploadImage
    Repo-->>VM: template/offices OR create response / upload result
    VM-->>Screen: emit event (ShowSnackBar / NavigateToClientDetails / HasDatatables)
    Screen->>Screen: show snackbar via SnackbarHostState
    Screen->>Nav: perform navigation on Navigate event
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • biplab1
  • Nagarjuna0033

Poem

🐰 I hopped through fields both old and new,

Actions guide the form in steady queue.
Events now chatter where states once sprawled,
Snackbars sing and uploads stand tall.
A rabbit cheers — the flow's refreshed and true!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: fixes for logical issues in the create new client feature and migration to BaseViewModel architecture.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@revanthkumarJ revanthkumarJ marked this pull request as ready for review March 18, 2026 18:16
@revanthkumarJ revanthkumarJ requested a review from a team March 18, 2026 18:16
@revanthkumarJ revanthkumarJ changed the title Revanth/create client revamp [Fix]: Create New Client Logical issues and migrate to BaseViewModel Mar 18, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

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)

261-275: ⚠️ Potential issue | 🟠 Major

Avoid implicit office side effects on first composition.

This block fetches staff for officeList[0] before the form actually selects an office, and the next effect immediately shows the empty-staff snackbar because staffInOffices starts empty. Since the dropdown later uses sortedOffices in Line 475, the implicit office may not even match the first visible option. Either initialize selectedOffice / selectedOfficeId to the same office before loading staff, or wait until the user explicitly picks one.

🤖 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 261 - 275, The current LaunchedEffect implicitly loads staff for
officeList[0] and triggers the empty-staff snackbar because staffInOffices
starts empty; modify CreateNewClientScreen.kt so you either (A) initialize the
form's selected office state (e.g., selectedOffice or selectedOfficeId) to match
officeList[0] (or sortedOffices[0] if dropdown uses sortedOffices) before
calling onAction(CreateNewClientAction.LoadStaffInOffices(...)), or (B) postpone
calling onAction(CreateNewClientAction.LoadStaffInOffices(...)) until the user
explicitly selects an office from the dropdown; also ensure the snackbar logic
that checks staffInOffices only runs after an office selection has been
attempted (so the snackbarHostState.showSnackbar and
onAction(CreateNewClientAction.UpdateStaff("", 0)) are not triggered on initial
composition).
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (2)

1109-1117: Seed the Success preview with actual success data.

With screenState = Success but clientsTemplate = null, this preview renders an empty container instead of the success UI. Populate the preview state with a minimal template/offices payload so the preview exercises the form.

🤖 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 1109 - 1117, The Success preview currently uses screenState =
Success but leaves clientsTemplate null, so update PreviewCreateNewClientScreen
to supply a minimal clientsTemplate and offices payload inside the
CreateNewClientState used for the preview: construct a small ClientsTemplate
(with at least required fields like name, savingsProduct options, or minimal
field definitions) and a non-empty offices list, attach them to
CreateNewClientState (and/or the CreateNewClientScreenPreviewProvider) so
CreateNewClientScreen receives a populated clientsTemplate and offices when
screenState == Success; locate PreviewCreateNewClientScreen and
CreateNewClientState/clientsTemplate to add this mock data for the preview.

953-1095: Remove the old screen-side validators.

Validation now lives in CreateNewClientViewModel.validateForm(), so these helpers are dead code. Keeping both paths around invites rule drift and leaves the extra scope plumbing with no purpose.

🤖 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 953 - 1095, The screen-side validators (isAllFieldsValid,
isFirstNameValid, isMiddleNameValid, isLastNameValid, isAddressTypeIdValid) are
dead now that validation is centralized in
CreateNewClientViewModel.validateForm(); remove these functions and any calls to
them (e.g., isAllFieldsValid(...)) from CreateNewClientScreen, and eliminate the
now-unused CoroutineScope and SnackbarHostState plumbing/parameters that existed
solely to show snackbars from those helpers; ensure callers use
CreateNewClientViewModel.validateForm() (or the ViewModel's validation flow)
instead and remove any unused imports or parameters left behind.
🤖 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/createNewClient/CreateNewClientViewModel.kt`:
- Around line 304-323: The loadStaffInOffices function starts a new collector
each call and can be overwritten by slower prior responses; change it to cancel
any previous collector and only keep the latest result — either use the
latest-only operator by calling
repository.getStaffInOffice(officeId).collectLatest { ... } inside
viewModelScope.launch, or store a single Job (e.g., loadStaffJob) on the
ViewModel and cancel/replace it before launching a new collector so only the
most recent staffInOffices update (and errors sent via sendEvent) can apply.
- Around line 430-490: validateForm currently misses validating the office
selection (selectedOfficeId defaults to 0 and is later written to officeId), so
add a check in validateForm to reject submissions when form.selectedOfficeId <=
0 (or null/undefined equivalent) and send the same
CreateNewClientEvent.ShowSnackBar with an appropriate error message resource
(e.g., feature_client_error_office_is_required) before returning false; update
the condition near other field checks (firstName/lastName/address) inside
validateForm to reference form.selectedOfficeId and ensure the UI-enforced
mandatory office selection cannot be submitted as 0 and prevents building the
payload with an invalid officeId.
- Around line 370-397: The uploadImage function currently starts its own
coroutine (viewModelScope.launch) so createClient clears the overlay before
compression/upload/navigation finish; make uploadImage a suspend function
(remove viewModelScope.launch) so compression, multipart creation,
repository.uploadClientImage, and
sendEvent(CreateNewClientEvent.NavigateToClientDetails(id)) run to completion
before returning, and update the createClient caller to call/await
uploadImage(id) (instead of launching it) so the UI overlay remains until the
upload completes; keep the same error handling using MFErrorParser.errorMessage
and sendEvent(CreateNewClientEvent.ShowSnackBar(err)) inside uploadImage.
- Around line 230-255: The current combine block in CreateNewClientViewModel
checks for Loading before Error, which causes an Error from one source to be
masked by Loading from the other; modify the conditional ordering to check if
clientResult or officeResult is DataState.Error first and update
mutableStateFlow to CreateNewClientState.ScreenState.Error(...) using the
failing result's message (clientResult.message or officeResult.message) before
handling DataState.Loading for either source so failures surface immediately.
- Around line 163-168: When handling CreateNewClientAction.UpdateOffice in
CreateNewClientViewModel (inside the updateForm { copy(...) } block), clear the
dependent staff state: set staff to an empty list and selectedStaffId to null
(or empty string as the codebase uses), in addition to updating selectedOffice
and selectedOfficeId so stale staff IDs cannot survive and be accepted by
createClientPayload.

---

Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 261-275: The current LaunchedEffect implicitly loads staff for
officeList[0] and triggers the empty-staff snackbar because staffInOffices
starts empty; modify CreateNewClientScreen.kt so you either (A) initialize the
form's selected office state (e.g., selectedOffice or selectedOfficeId) to match
officeList[0] (or sortedOffices[0] if dropdown uses sortedOffices) before
calling onAction(CreateNewClientAction.LoadStaffInOffices(...)), or (B) postpone
calling onAction(CreateNewClientAction.LoadStaffInOffices(...)) until the user
explicitly selects an office from the dropdown; also ensure the snackbar logic
that checks staffInOffices only runs after an office selection has been
attempted (so the snackbarHostState.showSnackbar and
onAction(CreateNewClientAction.UpdateStaff("", 0)) are not triggered on initial
composition).

---

Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 1109-1117: The Success preview currently uses screenState =
Success but leaves clientsTemplate null, so update PreviewCreateNewClientScreen
to supply a minimal clientsTemplate and offices payload inside the
CreateNewClientState used for the preview: construct a small ClientsTemplate
(with at least required fields like name, savingsProduct options, or minimal
field definitions) and a non-empty offices list, attach them to
CreateNewClientState (and/or the CreateNewClientScreenPreviewProvider) so
CreateNewClientScreen receives a populated clientsTemplate and offices when
screenState == Success; locate PreviewCreateNewClientScreen and
CreateNewClientState/clientsTemplate to add this mock data for the preview.
- Around line 953-1095: The screen-side validators (isAllFieldsValid,
isFirstNameValid, isMiddleNameValid, isLastNameValid, isAddressTypeIdValid) are
dead now that validation is centralized in
CreateNewClientViewModel.validateForm(); remove these functions and any calls to
them (e.g., isAllFieldsValid(...)) from CreateNewClientScreen, and eliminate the
now-unused CoroutineScope and SnackbarHostState plumbing/parameters that existed
solely to show snackbars from those helpers; ensure callers use
CreateNewClientViewModel.validateForm() (or the ViewModel's validation flow)
instead and remove any unused imports or parameters left behind.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22b41f7f-53b9-438a-a606-d19117e2fb47

📥 Commits

Reviewing files that changed from the base of the PR and between 120053a and 52c08eb.

📒 Files selected for processing (4)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
💤 Files with no reviewable changes (1)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientUiState.kt

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (2)

284-291: ⚠️ Potential issue | 🟡 Minor

Initialize the DOB picker from the current form state.

The text field reads from formState.dateOfBirth, but the picker always starts with null. Reopening a partially completed form shows a blank selection instead of the saved DOB.

💡 Proposed fix
     val dateOfBirthDatePickerState = rememberDatePickerState(
-        initialSelectedDateMillis = null,
+        initialSelectedDateMillis = formState.dateOfBirth,
         selectableDates = object : SelectableDates {

Also applies to: 418-429

🤖 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 284 - 291, The date picker state is initialized with null instead
of the current form value, so update the rememberDatePickerState calls (e.g.,
the dateOfBirthDatePickerState) to use formState.dateOfBirth (converted to epoch
millis) as initialSelectedDateMillis when present; keep the selectableDates
logic unchanged and handle null safely by falling back to null if
formState.dateOfBirth is empty—apply the same change to the other
rememberDatePickerState occurrence that also reads from formState.dateOfBirth.

259-273: ⚠️ Potential issue | 🟠 Major

Drive staff loading from the selected office, not officeList[0].

This effect fetches staff for the first raw office, while the dropdown later sorts offices and binds its value from formState.selectedOffice. The next effect also treats the initial empty staffInOffices as “no staff”, so first render can show a false snackbar and clear staff for an office the user never selected. Trigger the lookup from the selected office id and only emit the empty-state snackbar after that request completes.

Also applies to: 473-491

🤖 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 259 - 273, The current LaunchedEffect uses officeList[0].id to load
staff which causes incorrect fetches and a premature empty snackbar; change both
LaunchedEffect blocks to drive staff loading from the selected office value
(formState.selectedOffice or its id) instead of officeList[0], invoking
CreateNewClientAction.LoadStaffInOffices(selectedOfficeId) and only show
snackbarHostState.showSnackbar(...) and dispatch
CreateNewClientAction.UpdateStaff("", 0) after the fetch for that selected
office has completed (use the selectedOffice as the LaunchedEffect key and guard
against null/empty selection) so the empty-state snackbar is only emitted for
the actual selected office.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)

650-656: Remove the leftover UI-side validator chain.

Submit now sends CreateClient directly, so isAllFieldsValid is unreachable from this screen. Keeping the old validator path here makes it unclear whether validation lives in the UI or the ViewModel and invites the two to drift.

Also applies to: 951-983

🤖 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 650 - 656, Remove the leftover UI-side validator chain and any
unreachable isAllFieldsValid checks so the button directly dispatches the
ViewModel action; specifically, in the MifosButton onClick that calls
onAction(CreateNewClientAction.CreateClient) remove any surrounding UI
validation logic/variables named isAllFieldsValid and the validator chain blocks
(same cleanup for the other occurrence around lines that contain the MifosButton
and the validator code referenced in the comment), leaving the submit path to
only invoke CreateNewClientAction.CreateClient and letting the ViewModel own
validation.
🤖 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/createNewClient/CreateNewClientScreen.kt`:
- Around line 222-223: The overlay boolean state
(state.showOverLayProgressIndicator) is not preventing duplicate submissions;
either wire it into the button or guard the ViewModel: add enabled =
!state.showOverLayProgressIndicator to the MifosButton call site (the
MifosButton invocation that currently triggers CreateClient) so the button
becomes disabled while the overlay is shown, or alternatively add an in-flight
guard inside submitClient() (e.g., a Boolean like isSubmitting or a Job
reference stored on the ViewModel) that returns early if a submission is already
active and sets/clears that flag around the CreateClient dispatch.

---

Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 284-291: The date picker state is initialized with null instead of
the current form value, so update the rememberDatePickerState calls (e.g., the
dateOfBirthDatePickerState) to use formState.dateOfBirth (converted to epoch
millis) as initialSelectedDateMillis when present; keep the selectableDates
logic unchanged and handle null safely by falling back to null if
formState.dateOfBirth is empty—apply the same change to the other
rememberDatePickerState occurrence that also reads from formState.dateOfBirth.
- Around line 259-273: The current LaunchedEffect uses officeList[0].id to load
staff which causes incorrect fetches and a premature empty snackbar; change both
LaunchedEffect blocks to drive staff loading from the selected office value
(formState.selectedOffice or its id) instead of officeList[0], invoking
CreateNewClientAction.LoadStaffInOffices(selectedOfficeId) and only show
snackbarHostState.showSnackbar(...) and dispatch
CreateNewClientAction.UpdateStaff("", 0) after the fetch for that selected
office has completed (use the selectedOffice as the LaunchedEffect key and guard
against null/empty selection) so the empty-state snackbar is only emitted for
the actual selected office.

---

Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 650-656: Remove the leftover UI-side validator chain and any
unreachable isAllFieldsValid checks so the button directly dispatches the
ViewModel action; specifically, in the MifosButton onClick that calls
onAction(CreateNewClientAction.CreateClient) remove any surrounding UI
validation logic/variables named isAllFieldsValid and the validator chain blocks
(same cleanup for the other occurrence around lines that contain the MifosButton
and the validator code referenced in the comment), leaving the submit path to
only invoke CreateNewClientAction.CreateClient and letting the ViewModel own
validation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1439ad71-68da-4c52-9c64-a69ee4929e44

📥 Commits

Reviewing files that changed from the base of the PR and between 52c08eb and 68a4519.

📒 Files selected for processing (1)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt

@biplab1

biplab1 commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

@revanthkumarJ Please resolve the merge conflicts.

# Conflicts:
#	feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt
#	feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt
@revanthkumarJ

Copy link
Copy Markdown
Contributor Author

@revanthkumarJ Please resolve the merge conflicts.

done

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt (5)

229-253: ⚠️ Potential issue | 🟠 Major

Fail fast on Error before showing Loading.

With combine, an Error from one source plus Loading from the other still returns early as Loading, so the screen can keep spinning instead of surfacing the failure.

🤖 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 229 - 253, The current collect block in CreateNewClientViewModel
checks for Loading before Error which can hide an Error when the other stream is
Loading; reorder the checks so you first test if clientResult is DataState.Error
or officeResult is DataState.Error (set mutableStateFlow to
CreateNewClientState.ScreenState.Error with the appropriate message and return),
then check for Loading (set Loading state and return), and finally handle the
success case; use the existing symbols clientResult, officeResult,
DataState.Error, DataState.Loading, and mutableStateFlow to locate and update
the logic.

429-489: ⚠️ Potential issue | 🟠 Major

Require a valid office before building the payload.

selectedOfficeId starts at 0 on Line 611 and is copied straight into officeId at Line 508, but validateForm() never rejects that untouched state. This pushes a required-field error to the backend instead of failing fast in the form.

Also applies to: 505-508, 610-611

🤖 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 429 - 489, validateForm currently never checks selectedOfficeId so
a default 0 gets copied into officeId and causes a backend required-field error;
update validateForm (in CreateNewClientViewModel) to validate
form.selectedOfficeId (or CreateNewClientState.ClientFormState.selectedOfficeId)
and return false/send a CreateNewClientEvent.ShowSnackBar when selectedOfficeId
<= 0 so the form fails fast before building the payload (the same validation
must prevent officeId being set to 0 when constructing the payload that uses
officeId).

303-322: ⚠️ Potential issue | 🟠 Major

Only keep the latest office-staff request alive.

Each office change launches another collector. A slower response from an older office can still overwrite staffInOffices for the current selection, and stale requests can also emit irrelevant snackbar errors later.

🤖 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 303 - 322, The current loadStaffInOffices launches a new collector
every call and old collectors can overwrite staffInOffices or emit snackbar
errors; fix by tracking and cancelling the previous job before starting a new
one: add a nullable Job property (e.g., staffInOfficesJob) on the ViewModel, at
start of loadStaffInOffices call cancel staffInOfficesJob?.cancel(), then set
staffInOfficesJob = viewModelScope.launch {
repository.getStaffInOffice(officeId).collect { ... } } so only the latest
repository.getStaffInOffice collector can update mutableStateFlow.staffInOffices
or send CreateNewClientEvent.ShowSnackBar.

335-367: ⚠️ Potential issue | 🟠 Major

Run submit/create/upload as one coroutine.

Line 351 turns the overlay off right after uploadImage() is launched, and the extra launch layers also let a second submit start before the first one finishes. Make createClient() / uploadImage() suspend and guard the whole flow with one in-flight job.

Also applies to: 369-397, 399-425

🤖 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 335 - 367, The createClient flow launches uploadImage
asynchronously and clears the overlay immediately, allowing concurrent
submissions; change uploadImage to be a suspend function and call it
sequentially from createClient (remove the extra viewModelScope.launch around
uploadImage) so the overlay (showOverLayProgressIndicator in mutableStateFlow)
stays true until uploadImage returns or throws; additionally add an in-flight
guard (e.g., a Job field like currentCreateJob or an isSubmitting flag in state)
checked/updated at the start of createClient to prevent a second createClient
invocation while the first is running, and ensure all error paths and the
finally-like block reset showOverLayProgressIndicator and clear the guard.

162-168: ⚠️ Potential issue | 🟠 Major

Reset dependent staff state when the office changes.

staff, selectedStaffId, and staffInOffices survive an office switch here. If the next office also has staff, Lines 552-554 can still serialize the stale ID into the payload.

Minimal fix
             is CreateNewClientAction.UpdateOffice -> {
-                updateForm {
-                    copy(
-                        selectedOffice = action.name,
-                        selectedOfficeId = action.id,
-                    )
-                }
+                mutableStateFlow.update { current ->
+                    current.copy(
+                        staffInOffices = emptyList(),
+                        formState = current.formState.copy(
+                            selectedOffice = action.name,
+                            selectedOfficeId = action.id,
+                            staff = "",
+                            selectedStaffId = 0,
+                        ),
+                    )
+                }
             }

Also applies to: 552-554

🤖 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 162 - 168, When handling CreateNewClientAction.UpdateOffice in
CreateNewClientViewModel, reset dependent staff state to avoid carrying a stale
staff selection: inside the updateForm { copy(...) } call set staff =
emptyList(), selectedStaffId = null (or empty string if type requires), and
staffInOffices = emptyMap() so the form reflects the new office; also ensure the
code path that builds/serializes the request payload (the logic that reads
selectedStaffId to include staff in the payload) either reads these cleared
fields or guards/clears selectedStaffId before serializing so no stale staff ID
is emitted.
🤖 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/createNewClient/CreateNewClientViewModel.kt`:
- Around line 283-286: In each of the four methods (loadAddressConfiguration,
loadAddressTemplate, createClient, uploadImage) replace the broad catch (e:
Exception) handling so that CancellationException is rethrown immediately
instead of being parsed as an error: either add a specific catch (e:
CancellationException) { throw e } before the generic catch, or inside the
existing catch check if (e is CancellationException) throw e, then continue to
call MFErrorParser.errorMessage(e) and send the ShowSnackBar event; update the
catch blocks in those methods accordingly so coroutine cancellations are not
swallowed.

---

Duplicate comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt`:
- Around line 229-253: The current collect block in CreateNewClientViewModel
checks for Loading before Error which can hide an Error when the other stream is
Loading; reorder the checks so you first test if clientResult is DataState.Error
or officeResult is DataState.Error (set mutableStateFlow to
CreateNewClientState.ScreenState.Error with the appropriate message and return),
then check for Loading (set Loading state and return), and finally handle the
success case; use the existing symbols clientResult, officeResult,
DataState.Error, DataState.Loading, and mutableStateFlow to locate and update
the logic.
- Around line 429-489: validateForm currently never checks selectedOfficeId so a
default 0 gets copied into officeId and causes a backend required-field error;
update validateForm (in CreateNewClientViewModel) to validate
form.selectedOfficeId (or CreateNewClientState.ClientFormState.selectedOfficeId)
and return false/send a CreateNewClientEvent.ShowSnackBar when selectedOfficeId
<= 0 so the form fails fast before building the payload (the same validation
must prevent officeId being set to 0 when constructing the payload that uses
officeId).
- Around line 303-322: The current loadStaffInOffices launches a new collector
every call and old collectors can overwrite staffInOffices or emit snackbar
errors; fix by tracking and cancelling the previous job before starting a new
one: add a nullable Job property (e.g., staffInOfficesJob) on the ViewModel, at
start of loadStaffInOffices call cancel staffInOfficesJob?.cancel(), then set
staffInOfficesJob = viewModelScope.launch {
repository.getStaffInOffice(officeId).collect { ... } } so only the latest
repository.getStaffInOffice collector can update mutableStateFlow.staffInOffices
or send CreateNewClientEvent.ShowSnackBar.
- Around line 335-367: The createClient flow launches uploadImage asynchronously
and clears the overlay immediately, allowing concurrent submissions; change
uploadImage to be a suspend function and call it sequentially from createClient
(remove the extra viewModelScope.launch around uploadImage) so the overlay
(showOverLayProgressIndicator in mutableStateFlow) stays true until uploadImage
returns or throws; additionally add an in-flight guard (e.g., a Job field like
currentCreateJob or an isSubmitting flag in state) checked/updated at the start
of createClient to prevent a second createClient invocation while the first is
running, and ensure all error paths and the finally-like block reset
showOverLayProgressIndicator and clear the guard.
- Around line 162-168: When handling CreateNewClientAction.UpdateOffice in
CreateNewClientViewModel, reset dependent staff state to avoid carrying a stale
staff selection: inside the updateForm { copy(...) } call set staff =
emptyList(), selectedStaffId = null (or empty string if type requires), and
staffInOffices = emptyMap() so the form reflects the new office; also ensure the
code path that builds/serializes the request payload (the logic that reads
selectedStaffId to include staff in the payload) either reads these cleared
fields or guards/clears selectedStaffId before serializing so no stale staff ID
is emitted.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b6704ebe-44cd-4feb-8acf-0635d8c24884

📥 Commits

Reviewing files that changed from the base of the PR and between 68a4519 and 02b91d0.

📒 Files selected for processing (1)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt

private suspend fun validateForm(
form: CreateNewClientState.ClientFormState,
isAddressEnabled: Boolean,
): Boolean {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use when block to check

is DataState.Error ->
_createNewClientUiState.value =
CreateNewClientUiState.ShowError(Res.string.feature_client_failed_to_fetch_staffs)
combine(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use combineDataState including kotlin flow operators to check the state properly and can manage it easily and won't need a separate if block

)
}

LaunchedEffect(key1 = Unit) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we still using this? update the required logic directly in ViewModel as when user selects an office load its staff

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

737-743: ⚠️ Potential issue | 🟡 Minor

Add an accessibility label to the photo control.

This Image is clickable, but contentDescription = null gives assistive tech no meaningful name for the action.

🤖 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 737 - 743, The Image composable currently sets contentDescription =
null which prevents screen readers from describing the clickable photo control;
update the Image in CreateNewClientScreen to provide a meaningful accessibility
label (e.g., a string resource like stringResource(R.string.add_client_photo) or
a passed-in description) instead of null, so the clickable Image that invokes
onImageClick() can be announced to assistive technologies; ensure the label
accurately describes the action (e.g., "Add client photo" or "Edit client
photo") and keep the existing Modifier (.align, .clickable, .border) and
onImageClick reference.
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)

643-649: ⚠️ Potential issue | 🟠 Major

Disable submit while the create request is running.

state.showOverLayProgressIndicator only drives the overlay here; this button still stays tappable. That leaves a double-tap window where CreateClient can be dispatched more than once.

🤖 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 643 - 649, The submit MifosButton remains tappable during the
in-flight create request; disable it while state.showOverLayProgressIndicator is
true to prevent duplicate dispatches. Update the MifosButton invocation (the one
calling onAction(CreateNewClientAction.CreateClient)) to pass an enabled flag
tied to the inverse of state.showOverLayProgressIndicator (or equivalent in your
UI API) so the button is not clickable while the overlay/progress is visible.
🧹 Nitpick comments (2)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CombineDataState.kt (1)

20-27: Consider removing the unreachable else branch.

Since DataState is a sealed class with exactly three variants (Loading, Success, Error), all possible state combinations are already covered by the preceding conditions. The else -> DataState.Loading branch on line 26 is unreachable dead code.

You could refactor to an exhaustive when on explicit pairs or simply remove the else:

♻️ Suggested refactor
 fun <T1, T2, R> combineDataState(
     flow1: Flow<DataState<T1>>,
     flow2: Flow<DataState<T2>>,
     transform: (T1, T2) -> R,
 ): Flow<DataState<R>> = combine(flow1, flow2) { state1, state2 ->
     when {
         state1 is DataState.Loading || state2 is DataState.Loading -> DataState.Loading
         state1 is DataState.Error -> DataState.Error(state1.exception)
         state2 is DataState.Error -> DataState.Error(state2.exception)
-        state1 is DataState.Success && state2 is DataState.Success ->
-            DataState.Success(transform(state1.data, state2.data))
-        else -> DataState.Loading
+        else -> DataState.Success(transform((state1 as DataState.Success).data, (state2 as DataState.Success).data))
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CombineDataState.kt`
around lines 20 - 27, The when expression combining state1 and state2 contains
an unreachable else branch; remove the else -> DataState.Loading branch and make
the when exhaustive by matching the explicit combinations of the sealed
DataState variants (e.g., check Loading, Error, and Success pairs) in the
combine logic (the expression that uses state1, state2 and transform) so the
compiler understands all cases are handled without a default.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)

944-976: Keep validation in one place.

isAllFieldsValid() and the field validators are now private to this screen, but the submit path no longer calls them. Either remove these helpers or keep the same rules entirely in CreateNewClientViewModel so validation does not drift in two places.

🤖 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 944 - 976, isAllFieldsValid and the helper validators
(isFirstNameValid, isMiddleNameValid, isLastNameValid, isAddressTypeIdValid)
live privately in CreateNewClientScreen but the submit flow in
CreateNewClientViewModel no longer uses them, causing duplicated/ drifting
validation logic; fix by centralizing validation in one place: either remove the
private helpers from CreateNewClientScreen and call the ViewModel's validation
from the UI submit path, or move these helper implementations into
CreateNewClientViewModel (expose a single validateAll or isAllFieldsValid
function) and update the submit handler to call that ViewModel method, ensuring
the same validation rules are used for submission and UI feedback.
🤖 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 737-743: The Image composable currently sets contentDescription =
null which prevents screen readers from describing the clickable photo control;
update the Image in CreateNewClientScreen to provide a meaningful accessibility
label (e.g., a string resource like stringResource(R.string.add_client_photo) or
a passed-in description) instead of null, so the clickable Image that invokes
onImageClick() can be announced to assistive technologies; ensure the label
accurately describes the action (e.g., "Add client photo" or "Edit client
photo") and keep the existing Modifier (.align, .clickable, .border) and
onImageClick reference.

---

Duplicate comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 643-649: The submit MifosButton remains tappable during the
in-flight create request; disable it while state.showOverLayProgressIndicator is
true to prevent duplicate dispatches. Update the MifosButton invocation (the one
calling onAction(CreateNewClientAction.CreateClient)) to pass an enabled flag
tied to the inverse of state.showOverLayProgressIndicator (or equivalent in your
UI API) so the button is not clickable while the overlay/progress is visible.

---

Nitpick comments:
In
`@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CombineDataState.kt`:
- Around line 20-27: The when expression combining state1 and state2 contains an
unreachable else branch; remove the else -> DataState.Loading branch and make
the when exhaustive by matching the explicit combinations of the sealed
DataState variants (e.g., check Loading, Error, and Success pairs) in the
combine logic (the expression that uses state1, state2 and transform) so the
compiler understands all cases are handled without a default.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 944-976: isAllFieldsValid and the helper validators
(isFirstNameValid, isMiddleNameValid, isLastNameValid, isAddressTypeIdValid)
live privately in CreateNewClientScreen but the submit flow in
CreateNewClientViewModel no longer uses them, causing duplicated/ drifting
validation logic; fix by centralizing validation in one place: either remove the
private helpers from CreateNewClientScreen and call the ViewModel's validation
from the UI submit path, or move these helper implementations into
CreateNewClientViewModel (expose a single validateAll or isAllFieldsValid
function) and update the submit handler to call that ViewModel method, ensuring
the same validation rules are used for submission and UI feedback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: baf9d69c-a8ca-4dfd-8446-bf8b69d58ced

📥 Commits

Reviewing files that changed from the base of the PR and between 02b91d0 and 0cfa126.

📒 Files selected for processing (4)
  • cmp-ios/iosApp.xcodeproj/project.xcworkspace/xcuserdata/pronaysarker.xcuserdatad/UserInterfaceState.xcuserstate
  • core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CombineDataState.kt
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we already have a combineResults function in https://github.com/openMF/mifos-x-field-officer-app/blame/development/core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DataStateExtensions.kt, do we still require this helper function?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsPronay use that one

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also build is failing can you check that too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alr, sending

@sonarqubecloud

sonarqubecloud Bot commented Apr 8, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt (5)

162-168: ⚠️ Potential issue | 🟠 Major

Reset dependent staff state when the office changes.

Line 163 only updates the office fields. The previous staffInOffices, staff, and selectedStaffId survive the switch, so a stale staff ID can still be submitted once the next office loads.

Minimal fix
             is CreateNewClientAction.UpdateOffice -> {
-                updateForm {
-                    copy(
-                        selectedOffice = action.name,
-                        selectedOfficeId = action.id,
-                    )
-                }
+                mutableStateFlow.update { current ->
+                    current.copy(
+                        staffInOffices = emptyList(),
+                        formState = current.formState.copy(
+                            selectedOffice = action.name,
+                            selectedOfficeId = action.id,
+                            staff = "",
+                            selectedStaffId = 0,
+                        ),
+                    )
+                }
             }
🤖 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 162 - 168, When handling CreateNewClientAction.UpdateOffice in
CreateNewClientViewModel, reset dependent staff state so stale staff IDs aren’t
submitted: inside the updateForm { copy(...) } for the UpdateOffice branch
(where selectedOffice and selectedOfficeId are set), also clear staffInOffices,
staff, and selectedStaffId (e.g., set staffInOffices and staff to empty lists
and selectedStaffId to null/empty as appropriate for their types) so the view
model reflects the new office selection.

424-443: ⚠️ Potential issue | 🟠 Major

Reject submits with no office selected.

selectedOfficeId still defaults to 0/null, gets written straight to officeId at Line 470, and validateForm() never blocks that state. Users only find out from the backend even though office is mandatory in the UI.

Minimal fix
+import androidclient.feature.client.generated.resources.feature_client_error_office_is_required
...
         val errorMessage = when {
             form.firstName.isBlank() ->
                 getString(Res.string.feature_client_error_first_name_can_not_be_empty)
@@
             form.middleName.isNotBlank() && form.middleName.contains("[^a-zA-Z ]".toRegex()) ->
                 getString(Res.string.feature_client_error_middle_name_should_contain_only_alphabets)
+
+            form.selectedOfficeId == null || form.selectedOfficeId <= 0 ->
+                getString(Res.string.feature_client_error_office_is_required)
 
             isAddressEnabled && form.selectedAddressTypeId <= 0 ->
                 getString(Res.string.feature_client_error_address_type_is_required)

Also applies to: 467-470

🤖 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 424 - 443, The form validation (validateForm) currently misses
checking that an office is selected so selectedOfficeId can be 0/null and get
written to officeId; update validateForm in CreateNewClientViewModel to add a
branch that rejects when selectedOfficeId <= 0 (or null depending on type) and
return the appropriate error string (e.g.,
feature_client_error_office_is_required) before the method falls through to
success; reference the selectedOfficeId field and the code path that assigns
officeId around where officeId is written to ensure the check prevents
submission when no office is chosen.

327-356: ⚠️ Potential issue | 🟠 Major

Keep image upload inside the submit coroutine.

Line 342 clears showOverLayProgressIndicator right after calling uploadImage, but Line 361 starts a second coroutine, so compression/upload/navigation can still be running while the form is re-enabled.

Minimal fix
     fun createClient(clientPayload: ClientPayloadEntity) {
         viewModelScope.launch {
             try {
                 mutableStateFlow.update {
                     it.copy(
                         showOverLayProgressIndicator = true,
                     )
                 }
                 val clientId = repository.createClient(clientPayload)

                 clientId?.let {
                     uploadImage(it)
                 } ?: run {
                     sendEvent(CreateNewClientEvent.ShowSnackBar(getString(Res.string.feature_client_waiting_for_checker_approval)))
                 }
-
-                mutableStateFlow.update {
-                    it.copy(
-                        showOverLayProgressIndicator = false,
-                    )
-                }
             } catch (e: Exception) {
                 val err = MFErrorParser.errorMessage(e)
                 sendEvent(CreateNewClientEvent.ShowSnackBar(err))
-
+            } finally {
                 mutableStateFlow.update {
                     it.copy(
                         showOverLayProgressIndicator = false,
                     )
                 }
             }
         }
     }

-    fun uploadImage(id: Int) {
-        viewModelScope.launch {
-            try {
+    private suspend fun uploadImage(id: Int) {
+        try {
             val selectedImage = state.formState.selectedImage
             if (selectedImage == null) {
                 delay(500)
                 sendEvent(CreateNewClientEvent.NavigateToClientDetails(id))
             } else {
@@
                 repository.uploadClientImage(id, requestFile)
                 sendEvent(CreateNewClientEvent.NavigateToClientDetails(id))
             }
-            } catch (e: Exception) {
+        } catch (e: Exception) {
             val err = MFErrorParser.errorMessage(e)
             sendEvent(CreateNewClientEvent.ShowSnackBar(err))
-            }
         }
     }

Also applies to: 360-387

🤖 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 327 - 356, The submit coroutine clears showOverLayProgressIndicator
immediately after calling uploadImage but uploadImage launches its own coroutine
so compression/upload/navigation may still be running; modify uploadImage (or
its usage) so it performs upload work synchronously from the submit coroutine
(make uploadImage a suspend that does compression/upload and returns result, or
have submit await its Deferred) and only update
mutableStateFlow.showOverLayProgressIndicator = false and navigate/send events
after uploadImage completes (use the existing repository.createClient,
uploadImage, sendEvent and mutableStateFlow.update symbols to locate and change
the logic).

274-276: ⚠️ Potential issue | 🟠 Major

Rethrow CancellationException in these coroutine catch blocks.

These catch (e: Exception) handlers still treat coroutine cancellation as a user-visible failure. That can break structured concurrency and emit bogus snackbars during scope teardown.

Minimal fix pattern
+import kotlinx.coroutines.CancellationException
...
-        } catch (e: Exception) {
+        } catch (e: CancellationException) {
+            throw e
+        } catch (e: Exception) {
             val err = MFErrorParser.errorMessage(e)
             sendEvent(CreateNewClientEvent.ShowSnackBar(err))
         }

Also applies to: 288-290, 347-349, 383-385

🤖 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 274 - 276, The catch blocks in CreateNewClientViewModel that catch
Exception and call MFErrorParser.errorMessage(e) then
sendEvent(CreateNewClientEvent.ShowSnackBar(err)) should rethrow coroutine
CancellationException instead of treating it as a user error; update each such
handler to check for and rethrow if e is CancellationException (or use Kotlin's
is CancellationException) before parsing/logging/emit snackbars (e.g., in the
catch surrounding MFErrorParser.errorMessage and
sendEvent(CreateNewClientEvent.ShowSnackBar)), and apply the same change to the
other similar catch sites in this class so cancellation propagates correctly.

294-313: ⚠️ Potential issue | 🟠 Major

Cancel superseded staff loads.

Every LoadStaffInOffices action starts a new collector. Rapid office changes can race, and a slower response from the old office can overwrite staffInOffices for the current selection.

Minimal fix
+    private var loadStaffJob: Job? = null
+
     fun loadStaffInOffices(officeId: Int) {
-        viewModelScope.launch {
+        loadStaffJob?.cancel()
+        loadStaffJob = viewModelScope.launch {
             repository.getStaffInOffice(officeId).collect { result ->
                 when (result) {
                     is DataState.Error -> {
🤖 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 294 - 313, loadStaffInOffices starts a new collector each call and
can let older slower results overwrite state; add a cancellable Job (e.g., a
property staffLoadJob: Job?) and cancel it before launching the new coroutine.
Concretely: add staffLoadJob to the ViewModel, call staffLoadJob?.cancel() at
the start of loadStaffInOffices, then assign staffLoadJob =
viewModelScope.launch { repository.getStaffInOffice(officeId).collect { ... } }
so only the latest load's collector is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt`:
- Around line 162-168: When handling CreateNewClientAction.UpdateOffice in
CreateNewClientViewModel, reset dependent staff state so stale staff IDs aren’t
submitted: inside the updateForm { copy(...) } for the UpdateOffice branch
(where selectedOffice and selectedOfficeId are set), also clear staffInOffices,
staff, and selectedStaffId (e.g., set staffInOffices and staff to empty lists
and selectedStaffId to null/empty as appropriate for their types) so the view
model reflects the new office selection.
- Around line 424-443: The form validation (validateForm) currently misses
checking that an office is selected so selectedOfficeId can be 0/null and get
written to officeId; update validateForm in CreateNewClientViewModel to add a
branch that rejects when selectedOfficeId <= 0 (or null depending on type) and
return the appropriate error string (e.g.,
feature_client_error_office_is_required) before the method falls through to
success; reference the selectedOfficeId field and the code path that assigns
officeId around where officeId is written to ensure the check prevents
submission when no office is chosen.
- Around line 327-356: The submit coroutine clears showOverLayProgressIndicator
immediately after calling uploadImage but uploadImage launches its own coroutine
so compression/upload/navigation may still be running; modify uploadImage (or
its usage) so it performs upload work synchronously from the submit coroutine
(make uploadImage a suspend that does compression/upload and returns result, or
have submit await its Deferred) and only update
mutableStateFlow.showOverLayProgressIndicator = false and navigate/send events
after uploadImage completes (use the existing repository.createClient,
uploadImage, sendEvent and mutableStateFlow.update symbols to locate and change
the logic).
- Around line 274-276: The catch blocks in CreateNewClientViewModel that catch
Exception and call MFErrorParser.errorMessage(e) then
sendEvent(CreateNewClientEvent.ShowSnackBar(err)) should rethrow coroutine
CancellationException instead of treating it as a user error; update each such
handler to check for and rethrow if e is CancellationException (or use Kotlin's
is CancellationException) before parsing/logging/emit snackbars (e.g., in the
catch surrounding MFErrorParser.errorMessage and
sendEvent(CreateNewClientEvent.ShowSnackBar)), and apply the same change to the
other similar catch sites in this class so cancellation propagates correctly.
- Around line 294-313: loadStaffInOffices starts a new collector each call and
can let older slower results overwrite state; add a cancellable Job (e.g., a
property staffLoadJob: Job?) and cancel it before launching the new coroutine.
Concretely: add staffLoadJob to the ViewModel, call staffLoadJob?.cancel() at
the start of loadStaffInOffices, then assign staffLoadJob =
viewModelScope.launch { repository.getStaffInOffice(officeId).collect { ... } }
so only the latest load's collector is active.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3323d7ca-d55f-43e3-9ada-0c5490da30d7

📥 Commits

Reviewing files that changed from the base of the PR and between 0cfa126 and 63a0306.

📒 Files selected for processing (1)
  • feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientViewModel.kt

@biplab1

biplab1 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai Improve PR title based on github.com/openMF/mobile-wallet/wiki/Commit-style-guide. Give us just the title.

@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@biplab1: Based on the Commit Style Guide, here is the improved PR title:

fix(client): resolve create new client logical issues and migrate to BaseViewModel

@biplab1 biplab1 changed the title [Fix]: Create New Client Logical issues and migrate to BaseViewModel fix(client): resolve create new client logical issues and migrate to BaseViewModel Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants