feat(loan): Loan Transaction Table#2603
feat(loan): Loan Transaction Table#2603sahilshivekar wants to merge 1 commit intoopenMF:developmentfrom
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:
📝 WalkthroughWalkthroughAdds a reusable composable MifosTableRow and size tokens, extends Transaction model, adds localization strings, and refactors the LoanTransactions screen, UI state, and ViewModel to render transactions in a multi-column, scrollable table with per-row actions and a bottom sheet. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as LoanTransactionsScreen
participant VM as LoanTransactionsViewModel
participant State as LoanTransactionsUiState
participant Sheet as TransactionActionsBottomSheet
User->>Screen: open transactions screen
Screen->>VM: load transactions
VM->>VM: map & format transactions → transactionsTableData
VM->>State: set transactionsTableData
State-->>Screen: render table
User->>Screen: tap row
Screen->>VM: onRowAction(row)
VM->>State: set selectedRow & isBottomSheetOpen=true
State-->>Sheet: show actions
User->>Sheet: choose action
Sheet->>VM: onActionSelected(action, id)
VM->>State: dismissBottomSheet()
VM->>VM: perform action handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 6
🤖 Fix all issues with AI agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt`:
- Line 33: The parameter cellsBackgroundColor on MifosTableRow is declared but
never applied to each cell's Box; update the cell rendering loop in
MifosTableRow to apply a background color per cell by reading
cellsBackgroundColor.getOrNull(index) ?: Color.Transparent and adding
Modifier.background(thatColor) (or the appropriate Compose background API) to
the existing Box modifier for each cell so callers' colors are actually
rendered; ensure indexes align with the cells List and preserve existing
modifiers like padding/weight.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Around line 418-424: The current TransactionActionsBottomSheet is routing user
selection by passing a human-readable displayName string to onAction which is
fragile; change the signature of TransactionActionsBottomSheet's onAction from
(String) -> Unit to (TransactionAction) -> Unit and pass the actual
TransactionAction value (instead of action.displayName) from the UI, then update
the caller site(s) and the ViewModel method onActionSelected to accept a
TransactionAction parameter rather than String so routing uses the enum
directly; adjust any related usages (including the other occurrence around lines
469-471) to compile with the new type.
- Around line 102-103: The inner composable LoanTransactionsScreen should not
receive the viewModel directly; hoist the remaining ViewModel interactions by
adding lambda parameters (e.g., onDismissBottomSheet: () -> Unit and
onActionSelected: (action: ActionType, id: String) -> Unit) to its signature and
invocation, remove the viewModel parameter usage in the call site, and replace
calls to viewModel?.dismissBottomSheet() and viewModel?.onActionSelected(...)
inside the inner composable with the new onDismissBottomSheet() and
onActionSelected(...) callbacks; update the outer composable to pass
viewModel::dismissBottomSheet and viewModel::onActionSelected (or equivalent
lambdas) so previews/tests can supply mocks.
- Around line 151-163: The bottom sheet is rendered even when
uiState.selectedRow can be null, causing TransactionActionsBottomSheet to
receive empty fallbacks and produce an action-less UI; update the conditional to
only render the sheet when both uiState.isBottomSheetOpen and
uiState.selectedRow != null (use the non-null selectedRow to supply
transactionType, manuallyReversed and id to TransactionActionsBottomSheet and to
viewModel?.onActionSelected) so the component never receives empty defaults and
always has a valid selectedRow to act on.
- Around line 362-393: The background logic in textCells uses Color.Unspecified
and a magic range 6..10; replace Color.Unspecified with Color.Transparent to
avoid using a non-drawable sentinel, and replace the hard-coded range with a
named constant or predicate (e.g., BREAKDOWN_COLUMN_INDICES or
isBreakdownColumn(index)) used inside the map so the intent is explicit and
resilient to column reorder; update the background condition in the textCells
mapping to check that named constant/predicate instead of the literal 6..10.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt`:
- Line 56: DateHelper.getDateAsString currently dereferences transaction.date
list indices directly which will throw IndexOutOfBoundsException when
transaction.date is empty or incomplete; update DateHelper.getDateAsString to
first validate the input list (check for null and size >= 3 or handle
variable-length dates) and return a safe fallback (e.g., empty string or
formatted partial date) when data is missing, and ensure callers like
LoanTransactionsViewModel when mapping transactionDate =
DateHelper.getDateAsString(transaction.date) will receive the safe value instead
of crashing.
🧹 Nitpick comments (6)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt (1)
50-63: Consider settingverticalAlignmenton theRow.The
Rowdefaults toAlignment.Top. For a table,Alignment.CenterVerticallyis typically more appropriate to keep cell content aligned when heights vary.Row( modifier = Modifier .horizontalScroll(scrollState), + verticalAlignment = Alignment.CenterVertically, ) {feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt (1)
20-25: Consider separating bottom sheet / selection state from data state.
selectedRowandisBottomSheetOpenare transient UI interaction state mixed into theShowLoanTransactiondata class. Every bottom sheet toggle creates a new copy of the state, potentially triggering broader recomposition. For a small screen this is acceptable, but if the screen grows in complexity, consider extracting these into a separate state holder.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt (1)
119-122:onActionSelectedis a no-op with a TODO.The method accepts raw
Stringparameters instead of leveraging theTransactionActionenum defined in the UI state file. When implementing the TODO, consider using the enum for type safety.-fun onActionSelected(action: String, id: String) { - // TODO: Handle the action based on action string and id - dismissBottomSheet() -} +fun onActionSelected(action: TransactionAction, id: String) { + when (action) { + TransactionAction.UNDO_TRANSACTION -> { /* TODO */ } + TransactionAction.VIEW_RECEIPTS -> { /* TODO */ } + TransactionAction.VIEW_JOURNAL_ENTRIES -> { /* TODO */ } + TransactionAction.VIEW_TRANSACTION -> { /* TODO */ } + } + dismissBottomSheet() +}Do you want me to open a follow-up issue to track the implementation of
onActionSelectedactions?feature/loan/src/commonMain/composeResources/values/strings.xml (1)
153-153: Consider adding string resources for bottom sheet action labels.The
TransactionActionenum inLoanTransactionsUiState.ktuses hardcoded English strings ("Undo Transaction", "View Receipts", etc.) for user-facing action labels. For i18n support, these should be defined as string resources here and resolved viastringResource()in the composable layer.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (2)
253-319: Header background color computed identically in three places.The
lerp(MaterialTheme.colorScheme.surface, MaterialTheme.colorScheme.primary, 0.3f)expression is repeated at lines 259, 302, and a similar one at line 369–372 (with different fraction). Extract the header color into a local variable at the top ofLoanTransactionsTableContent:Proposed fix
+ val headerBackground = lerp( + MaterialTheme.colorScheme.surface, + MaterialTheme.colorScheme.primary, + 0.3f, + )Then use
headerBackgroundin both headerBoxmodifiers instead of repeating thelerp(...)call.
507-508: Remove commented-out preview states.These commented-out entries (
ShowFetchingError,ShowProgressBar) should either be re-enabled to preview all UI states or removed to keep the code clean. Leaving them commented out adds no value and clutters the file.
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt
Outdated
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Outdated
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Outdated
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
...an/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt
Outdated
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/MIFOSAC-651-Loan-Transaction-Table
branch
2 times, most recently
from
0f87c40 to
a6fe084
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Line 139: The code force-unwraps uiState.transactionsTableData with "!!" in
the conditional (if (uiState.transactionsTableData!!.transactions.isEmpty())),
which will crash if transactionsTableData is null; update the check to safely
handle null by either (a) changing the UiState/ShowLoanTransaction so
transactionsTableData is non-null and remove the "!!" usage, or (b) perform a
safe-access/null-coalescing check (e.g., use
uiState.transactionsTableData?.transactions.isNullOrEmpty() or check for null
before accessing transactions) to avoid the force-unwrap; locate references to
transactionsTableData and the conditional in LoanTransactionsScreen.kt to
implement the chosen fix.
- Around line 462-468: The getTextForAction function currently returns
feature_loan_transaction_action_undo for every TransactionAction; update the
when branches in getTextForAction to return the correct stringResource for each
action instead of reusing the undo string—map TransactionAction.VIEW_TRANSACTION
-> feature_loan_transaction_action_view_transaction,
TransactionAction.UNDO_TRANSACTION ->
feature_loan_transaction_action_undo_transaction,
TransactionAction.VIEW_RECEIPTS ->
feature_loan_transaction_action_view_receipts, and
TransactionAction.VIEW_JOURNAL_ENTRIES ->
feature_loan_transaction_action_view_journal_entries (use the corresponding
existing resource names in your project if they differ).
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt`:
- Around line 18-22: Make transactionsTableData non-null to encode the invariant
instead of relying on !! in the UI: change the ShowLoanTransaction data class
field transactionsTableData from LoanTransactionsTableData? to non-null
LoanTransactionsTableData with a sensible default (e.g., an empty/initial
LoanTransactionsTableData instance) and update any callers constructing
ShowLoanTransaction (e.g., ViewModel places that set this state) to pass that
default when needed; alternatively, if you prefer guarding in the UI, remove the
!! in LoanTransactionsScreen and null-check or early-return when
transactionsTableData is null. Ensure references to ShowLoanTransaction and
LoanTransactionsScreen are updated accordingly.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt`:
- Around line 43-100: In LoanTransactionsViewModel inside the DataState.Success
branch, add null-safe fallbacks when reading loanWithAssociations.currency:
replace direct accesses to currency.code and currency.decimalPlaces with safe
values (e.g., val currencyCode = loanWithAssociations.currency.code ?:
"<fallback>" and val maxDigits = loanWithAssociations.currency.decimalPlaces ?:
<defaultInt>) before mapping transactions; ensure those fallback values are then
passed to CurrencyFormatter.format(...) so CurrencyFormatter.format is never
called with null currencyCode or null maxDigits. Reference symbols:
LoanTransactionsViewModel, loanWithAssociations, currency.code,
currency.decimalPlaces, CurrencyFormatter.format, _loanTransactionsUiState,
LoanTransactionsUiState.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt (1)
45-55:fromValuereturnsnull— prefer returningUNKNOWNto eliminate null-handling at every call site.Suggested fix
companion object { - fun fromValue(value: String): TransactionType? = entries.find { it.value == value } + fun fromValue(value: String): TransactionType = entries.find { it.value == value } ?: UNKNOWN }This also simplifies the call site in the ViewModel.
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
...loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt
Show resolved
Hide resolved
...an/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/MIFOSAC-651-Loan-Transaction-Table
branch
3 times, most recently
from
47e09e2 to
94471eb
Compare
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/Transaction.kt
Show resolved
Hide resolved
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt
Outdated
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/MIFOSAC-651-Loan-Transaction-Table
branch
from
94471eb to
f679ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Around line 164-167: The current onAction handler calls
uiState.selectedRow?.id?.let { id -> onTransactionActionClick(action,
id.toInt()) } which will throw NumberFormatException when id is the "-"
placeholder; change the conversion to use id.toIntOrNull() and guard the null
case (e.g., return early or skip calling onTransactionActionClick) so only valid
integers are passed to onTransactionActionClick; locate this in the onAction
lambda referencing uiState.selectedRow?.id and replace the toInt() conversion
with a safe toIntOrNull() check.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt`:
- Around line 51-53: The current mapping sets TransactionRowData.id to the
string "-" when transaction.id is null which later causes NumberFormatException
when parsed with toInt(); change the mapping to preserve the numeric nature by
storing id as Int? in
LoanTransactionsUiState.LoanTransactionsTableData.TransactionRowData (assign id
= transaction.id) and update all places that consume TransactionRowData.id
(notably the LoanTransactionsScreen consumer that calls id.toInt()) to handle
nullable Ints directly (or use safe null checks) instead of parsing strings;
alternatively, if you must keep id as String, update the consumer to use
toIntOrNull() and handle nulls safely—adjust the TransactionRowData declaration
and all call sites accordingly.
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt (1)
52-54:fromValuereturnsTransactionType?but callers always fall back toUNKNOWN.Since every call site uses
?: TransactionType.UNKNOWN, consider returningTransactionTypedirectly withUNKNOWNas the default. This eliminates unnecessary null handling at every call site.Proposed fix
companion object { - fun fromValue(value: String): TransactionType? = entries.find { it.value == value } + fun fromValue(value: String): TransactionType = entries.find { it.value == value } ?: UNKNOWN }feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (2)
224-339: Header construction relies on positional magic indices (index == 6, empty-string placeholders).The two-tier header uses positional index checks (line 275:
index == 6for center alignment) and a list of empty strings for non-breakdown columns in row 2 (lines 291-303). This is fragile — any column reorder will silently break alignment and styling. Consider defining a column descriptor (e.g., a data class withheader,subHeader,width,isBreakdown) and deriving both header rows and data rows from it.
498-502:dismissBottomSheetis called twice per action selection.
onAction(action)routes toviewModel.onActionSelected(...)which internally callsdismissBottomSheet(). Then line 501 callsonDismissRequest()which also callsdismissBottomSheet(). The state update is idempotent so this won't break, but the explicitonDismissRequest()call on line 501 is redundant.Proposed fix
.clickable { onAction(action) - onDismissRequest() }
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
...an/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
|
@sahilshivekar rest LGTM. 👍 |
|
@sahilshivekar I think it looks good, but I think you can make one more addition, you can add a spacer on the sides of the table so that it looks like a card that starts and also a spacer on the right side such that you can see the spacer after you scroll completely to the right and round the corners of the table. This way the heading won't look very close to the edge of the screen. |
I did exactly that initially but Rajan sir told me to remove it during daily standup |
|
@sahilshivekar No, I'm not talking about that, I'm talking about 2 spacer in a row with the table not padding such that it feels like a card sliding into frame. |
|
@sahilshivekar I think we can get rid of the Actions column, instead clicking anywhere in a row should bring up the Actions bottom sheet. |
I got you now, nice idea |
sahilshivekar
force-pushed
the
feature/MIFOSAC-651-Loan-Transaction-Table
branch
from
f679ca6 to
38dbf5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt`:
- Around line 63-84: The loop in MifosTableRow using cells.forEachIndexed reads
widths[index] directly which can throw IndexOutOfBounds; change to a safe lookup
(e.g., widths.getOrNull(index) with a sensible fallback width) and assign that
to cellWidth before creating the Box; also make HorizontalDivider conditional on
the same showBorders flag (so HorizontalDivider is only rendered when
showBorders is true), and keep the existing VerticalDivider behavior including
the trailing vertical divider when index == cells.lastIndex.
- Around line 71-77: The HorizontalDivider is currently stacked at the top
because Box stacks children; update the MifosTableRow cell implementation so the
divider is positioned at the bottom — either replace Box(modifier =
Modifier.width(cellWidth)) with a Column and place HorizontalDivider after
content(), or keep the Box and add a Modifier.align(Alignment.BottomCenter) (or
BottomStart) to the HorizontalDivider; reference symbols: HorizontalDivider,
Box, content(), cellWidth, borderColor to locate and apply the change.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Around line 187-215: columnWidths currently defines 13 widths (including a
trailing "actions" smallWidth) but TransactionRow and headersRow2 only render 12
cells, leaving the last width unused; fix by either removing the trailing
actions entry from columnWidths (delete the final smallWidth) or add a
corresponding actions cell in TransactionRow and headersRow2 (e.g., render an
action/composable at the same final index) so the widths and rendered cells
align; reference columnWidths, TransactionRow, and headersRow2 to locate and
apply the change.
- Around line 476-479: The click handler is calling two dismiss paths
redundantly: onAction(action) -> viewModel.onActionSelected(action, id) which
already calls dismissBottomSheet(), and then onDismissRequest() ->
viewModel.dismissBottomSheet() again; remove the redundant explicit dismiss by
deleting the onDismissRequest() call from the clickable lambda (or only call
onAction and not call onDismissRequest there) so the dismissal is performed once
via viewModel.onActionSelected; update the clickable block where
onAction(action) and onDismissRequest() are invoked to only invoke
onAction(action).
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt (1)
52-54:fromValuereturns nullable butUNKNOWNalready serves as the fallback.Since there's an
UNKNOWNenum value,fromValuecould return a non-nullTransactionTypedirectly, eliminating the need for callers to handle nullability.Proposed simplification
companion object { - fun fromValue(value: String): TransactionType? = entries.find { it.value == value } + fun fromValue(value: String): TransactionType = entries.find { it.value == value } ?: UNKNOWN }Then in the ViewModel (line 63-65):
-transactionType = transaction.type?.value?.let { - TransactionType.fromValue(it) -} ?: TransactionType.UNKNOWN, +transactionType = transaction.type?.value?.let { + TransactionType.fromValue(it) +} ?: TransactionType.UNKNOWN,feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (1)
96-99: AccessingviewModel.viewModelScopefrom the UI layer breaks encapsulation.The
onRetrylambda reaches into the ViewModel's coroutine scope. Consider makingloadLoanTransactionnon-suspend (launching internally) or exposing a dedicatedretry()method.Proposed approach — add a retry function in the ViewModel
In
LoanTransactionsViewModel:fun retry() { viewModelScope.launch { loadLoanTransaction() } }Then in the outer composable:
- onRetry = { - viewModel.viewModelScope.launch { - viewModel.loadLoanTransaction() - } - }, + onRetry = { viewModel.retry() },
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt
Show resolved
Hide resolved
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTableRow.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/MIFOSAC-651-Loan-Transaction-Table
branch
from
38dbf5e to
c53563f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt`:
- Around line 52-54: The companion object's fromValue currently returns null for
unrecognized strings; update TransactionType.fromValue to return the explicit
fallback TransactionType.UNKNOWN instead of null by falling back when
entries.find { it.value == value } yields null (e.g., use the elvis operator or
equivalent to return UNKNOWN).
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (2)
96-99: Inconsistent invocation ofloadLoanTransaction.On line 90,
loadLoanTransaction()is called directly insideLaunchedEffect(a coroutine scope). On line 97-99, it's wrapped inviewModel.viewModelScope.launch. If the function is asuspendfunction, theonRetrylambda (which is not a coroutine scope) needs thelaunchwrapper—but then the outer composable leaks ViewModel implementation details. If it's a regular function that launches internally, theviewModelScope.launchwrapper is redundant.Consider making
onRetryconsistent with theLaunchedEffectcall by hoisting the coroutine launch.
282-295: Second header row uses empty strings as spacers — consider using explicit empty composables.The
headersRow2list uses 6 empty strings and 5 header strings, relying on string content to determine background color (line 304). This couples visual styling to string emptiness, which is fragile. A more explicit approach (e.g., a sealed type distinguishing empty/header cells, or at minimum a named constant for the empty placeholder) would make the intent clearer.
...loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsUiState.kt
Show resolved
Hide resolved
@biplab1 I have removed the Actions column and now clicking on the row shows the bottom sheet |
@techsavvy185 I have implemented this, check it |
|
@sahilshivekar It actually looks really good to me. |
sahilshivekar
changed the title
5 participants
Jira-MIFOSAC-651
WhatsApp.Video.2026-02-11.at.19.13.24.mp4
Summary by CodeRabbit
New Features
Localization