feat(loan): Implementation of Export PDF functionality.#2622
Open
techsavvy185 wants to merge 8 commits intoopenMF:developmentfrom
Open
feat(loan): Implementation of Export PDF functionality.#2622techsavvy185 wants to merge 8 commits intoopenMF:developmentfrom
techsavvy185 wants to merge 8 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughIntroduces a multiplatform PDF export pipeline (common API, platform implementations), refactors loan repayment schedule into a state-driven UI with PDF export, adds HTML templating for PDFs, updates design tokens/table components/icons, adds UI strings and a details card, and adds build/dependency changes for kotlinx-html and OpenHTMLToPDF. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Screen as LoanRepaymentScreen
participant VM as ViewModel
participant Generator as HtmlGenerator
participant Pdf as PdfGenerator
participant Platform as OS/Browser
User->>Screen: Tap "Export to PDF"
Screen->>VM: send ExportToPdf action
VM->>Generator: request HTML (tableData + strings)
Generator-->>VM: return HTML content
VM->>Pdf: generateAndSharePdf(html, fileName, pageConfig)
Pdf->>Platform: platform-specific render/share (WebView/PrintManager, OpenHTMLToPDF+Dialog, iframe+print)
Platform-->>User: show save/print UI or error
Pdf-->>VM: success or error
VM->>Screen: emit event (ExportPdf or PdfExportError)
Screen-->>User: display result dialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
techsavvy185
force-pushed
the
loanPdfExport
branch
from
8c768cf to
d175b2c
Compare
Contributor
Author
|
The code in this PR is exactly the same as the one implemented in Mifos Mobile. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt (2)
41-44: Minor: Remove the space before the colon in the heading format.The string
"$heading :"has an unconventional space before the colon. Standard formatting would be"$heading:"or"$heading: "(space after).✏️ Suggested fix
Text( - text = "$heading :", + text = "$heading:", style = MifosTypography.labelMediumEmphasized, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt` around lines 41 - 44, Update the Text composable in MifosDetailsCard where the heading string is built as "$heading :"; change it to remove the space before the colon so it reads "$heading:" (or "$heading: " if you prefer a trailing space) to match conventional punctuation; locate the Text call that uses MifosTypography.labelMediumEmphasized and adjust the interpolated string accordingly.
33-51: Consider documenting that map insertion order is preserved.The
forEachiteration relies on the map's iteration order. If callers pass aLinkedHashMap(or usemapOf()which returns one in Kotlin), order is preserved. However, if a plainHashMapis passed, the display order becomes unpredictable. This is typically fine sincemapOf()is the idiomatic Kotlin approach, but consider adding a KDoc comment noting that insertion order is respected for display.📝 Suggested KDoc addition
+/** + * Displays a card with key-value detail rows. + * + * `@param` details Map of heading to value pairs. Iteration order determines display order, + * so use [mapOf] or [LinkedHashMap] to preserve insertion order. + * `@param` modifier Modifier to be applied to the card. + */ `@Composable` fun MifosDetailsCard( details: Map<String, String?>, modifier: Modifier = Modifier, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt` around lines 33 - 51, Add a KDoc comment to MifosDetailsCard above its declaration indicating that the component iterates the provided details map in its natural iteration order (so insertion order is preserved when callers pass a LinkedHashMap or use mapOf()), and that using a plain HashMap will produce unpredictable display order; mention callers should use LinkedHashMap/mapOf if a stable order is required. Reference the MifosDetailsCard component and its details parameter in the KDoc so readers know this behavior applies to the forEach-driven rendering.gradle.properties (1)
19-19: Prefer safer default daemon memory limits for broader dev/CI compatibility.Hard-coding
-Xmx8gplus-XX:MaxMetaspaceSize=1gcan make builds brittle on lower-memory environments. Consider a more conservative default here and let high-memory machines override locally.Proposed adjustment
-org.gradle.jvmargs=-Xmx8g -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC -XX:MaxMetaspaceSize=1g +org.gradle.jvmargs=-Xmx6g -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle.properties` at line 19, The org.gradle.jvmargs setting currently hard-codes high memory (-Xmx8g and -XX:MaxMetaspaceSize=1g); change this to more conservative defaults (for example -Xmx1g or -Xmx2g and remove or reduce MaxMetaspaceSize) so CI and low-memory dev machines aren’t forced to allocate 8GB, and document/howto override locally (e.g., via GRADLE_OPTS or a local gradle.properties) so machines that need larger heaps can opt-in; update the org.gradle.jvmargs line and add a brief comment explaining how to override.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (3)
494-503: Preview uses empty default state which won't render meaningful content.The preview uses
LoanRepaymentScheduleState()which hasscreenState = ScreenState.Loadingby default. Consider providing a sampleSuccessstate with mock data for more useful previews.🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 494 - 503, The preview PreviewLoanRepaymentSchedule currently builds LoanRepaymentScheduleState() which defaults to ScreenState.Loading and shows nothing; update PreviewLoanRepaymentSchedule to construct and pass a meaningful LoanRepaymentScheduleState representing a ScreenState.Success with mocked loan repayment data (e.g., sample schedule items, totals, and any flags used by LoanRepaymentScheduleScreenContent) so the composable renders useful content; locate PreviewLoanRepaymentSchedule and replace the default state with a fabricated success state instance matching the state model used by LoanRepaymentScheduleScreenContent.
186-193: Redundant null check onrepaymentScheduleTableDatain Success state.When
screenStateisSuccess, therepaymentScheduleTableDatashould be guaranteed non-null based on the ViewModel logic (line 172 in ViewModel sets both together). Consider making this invariant explicit in the state model or using!!with a comment explaining the guarantee.🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 186 - 193, The null-check around repaymentScheduleTableData in the LoanRepaymentScheduleState.ScreenState.Success branch is redundant; when the ViewModel emits Success it guarantees repaymentScheduleTableData is non-null. Replace the safe-let with a direct use of the non-null value (e.g., pass state.repaymentScheduleTableData!! to LoanRepaymentScheduleContent) and add a short comment referencing the ViewModel invariant, or better yet update the state model so Success carries a non-null tableData (change LoanRepaymentScheduleState.ScreenState.Success to include a non-null repaymentScheduleTableData property) and then call LoanRepaymentScheduleContent(tableData = state.repaymentScheduleTableData, basicDetails = state.basicDetails) without a null check.
103-135: PDF export error handling could be more robust.A few observations:
- Line 122:
e.printStackTrace()is not ideal for production logging- Lines 121-128: Catching generic
Exceptionmay hide the actual error type- The
coroutineScope.launchinsideLaunchedEffectcollection may outlive the composable if navigation happens during PDF generation💡 Consider structured error handling
coroutineScope.launch { try { val pdfStrings = createPdfStrings() val htmlGenerator = RepaymentScheduleHtmlGenerator(data, pdfStrings) val htmlContent = htmlGenerator.generateHtml() val fileName = "repayment_schedule_${data.accountNo}" val pageConfig = PageConfig( size = PageSize.A4, orientation = Orientation.LANDSCAPE, marginMm = 8, ) pdfGenerator.generateAndSharePdf(htmlContent, fileName, pageConfig) } catch (e: Exception) { - e.printStackTrace() + // Consider using a proper logging framework viewModel.trySendAction( LoanRepaymentScheduleAction.PdfExportError( title = getString(Res.string.feature_loan_export_pdf_error_title), - message = getString(Res.string.feature_loan_export_pdf_error), + message = e.message ?: getString(Res.string.feature_loan_export_pdf_error), ), ) } }🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 103 - 135, The PDF export handling is fragile: it uses e.printStackTrace(), catches a generic Exception, and launches a coroutine via coroutineScope.launch inside LaunchedEffect which can outlive the composable; fix by moving the PDF generation out of the UI effect into a lifecycle-safe place (e.g., add a viewModel.exportRepaymentSchedulePdf(data) or a suspend viewModel.generateAndSharePdf(...) that calls createPdfStrings, RepaymentScheduleHtmlGenerator, and pdfGenerator.generateAndSharePdf), replace coroutineScope.launch with a call to that viewModel suspend function from LaunchedEffect (or use launch tied to the LaunchedEffect coroutine scope) so cancellation follows the composable lifecycle, catch and handle specific exceptions (e.g., IOException, PdfGenerationException) instead of Exception, and replace e.printStackTrace() with structured logging or include e.message/stack in the LoanRepaymentScheduleAction.PdfExportError payload when calling viewModel.trySendAction; reference functions/classes: LaunchedEffect, viewModel.eventFlow, coroutineScope.launch, createPdfStrings, RepaymentScheduleHtmlGenerator, pdfGenerator.generateAndSharePdf, and LoanRepaymentScheduleAction.PdfExportError.core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.kt (1)
88-90: Consider using structured logging instead ofprintln.Using
printlnfor error logging is not ideal for production. Consider using a proper logging framework or at minimumconsole.errorfor JS targets to ensure errors are visible in browser dev tools.💡 Use console.error for JS logging
} catch (e: Exception) { - println("Error generating PDF on Web: ${e.message}") + console.error("Error generating PDF on Web: ${e.message}") throw e } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.kt` around lines 88 - 90, The catch block in PdfGenerator (the exception handler shown in PdfGenerator.js.kt) currently calls println("Error generating PDF on Web: ${e.message}"); replace this with a JS-appropriate structured log call—use console.error with the error and message (or integrate the project's logging utility if available) so errors appear in browser devtools; update the catch in the PDF generation function to call console.error("Error generating PDF on Web:", e) or route through the existing logger instead of println and keep rethrowing the exception.core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/HtmlTemplateGenerator.kt (1)
142-151: Date format is hardcoded and may not match user locale expectations.The date format
${currentDate.day}/${currentDate.month.number}/${currentDate.year}producesd/M/yyyyformat, which differs from many locales (e.g., US usesM/d/yyyy, ISO usesyyyy-MM-dd). For a PDF document, consider using a consistent ISO format or the locale-aware formatting.💡 Consider using ISO date format for consistency
`@OptIn`(ExperimentalTime::class) protected suspend fun getGenerationDateText(): String { val currentDate = kotlin.time.Clock.System.now() .toLocalDateTime(kotlinx.datetime.TimeZone.currentSystemDefault()) .date - val dateString = "${currentDate.day}/${currentDate.month.number}/${currentDate.year}" + val dateString = "${currentDate.year}-${currentDate.month.number.toString().padStart(2, '0')}-${currentDate.day.toString().padStart(2, '0')}" val generationDateLabel = getString(Res.string.feature_pdf_generation_date) return "$generationDateLabel: $dateString" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/HtmlTemplateGenerator.kt` around lines 142 - 151, The date formatting in getGenerationDateText is hardcoded as "${currentDate.day}/${currentDate.month.number}/${currentDate.year}" which is not locale-safe; change it to produce a consistent ISO-8601 date (yyyy-MM-dd) or use a locale-aware formatter instead. Update getGenerationDateText to derive the date string from the LocalDate's ISO representation (or, if you need locale-specific presentation, use a platform DateTimeFormatter with the current locale) and keep the rest of the method (generationDateLabel assembly) unchanged so the label from getString(Res.string.feature_pdf_generation_date) is preserved.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt (1)
104-123: Consider potential state race with nested coroutine launch.In
handleNetworkStatus, there's aviewModelScope.launchthat updates state based on network status. If network status changes rapidly, multiple coroutines could be launched and compete to update state. Whileupdateis thread-safe, the logic flow could become unpredictable.💡 Consider removing unnecessary coroutine wrapper
The operations inside this launch block don't require suspension. Consider removing the nested launch:
private fun handleNetworkStatus(isOnline: Boolean) { mutableStateFlow.update { it.copy(networkStatus = isOnline) } - viewModelScope.launch { - if (!isOnline) { - mutableStateFlow.update { current -> - if (current.screenState is LoanRepaymentScheduleState.ScreenState.Loading || - current.screenState is LoanRepaymentScheduleState.ScreenState.Error || - current.screenState is LoanRepaymentScheduleState.ScreenState.Network - ) { - current.copy(screenState = LoanRepaymentScheduleState.ScreenState.Network) - } else { - current - } + if (!isOnline) { + mutableStateFlow.update { current -> + if (current.screenState is LoanRepaymentScheduleState.ScreenState.Loading || + current.screenState is LoanRepaymentScheduleState.ScreenState.Error || + current.screenState is LoanRepaymentScheduleState.ScreenState.Network + ) { + current.copy(screenState = LoanRepaymentScheduleState.ScreenState.Network) + } else { + current } - } else { - loadLoanRepaySchedule() } + } else { + loadLoanRepaySchedule() } }🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt` around lines 104 - 123, In handleNetworkStatus, remove the unnecessary viewModelScope.launch wrapper for the non-suspending offline branch to avoid racing coroutines: perform the mutableStateFlow.update synchronously when !isOnline (use the existing conditional update using LoanRepaymentScheduleState.ScreenState checks) and only start a coroutine (viewModelScope.launch) for the path that needs suspension by calling loadLoanRepaySchedule(); keep references: handleNetworkStatus, mutableStateFlow.update, LoanRepaymentScheduleState.ScreenState, viewModelScope.launch, and loadLoanRepaySchedule().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt`:
- Around line 41-46: The default borderColor in MifosTable.kt currently equals
backgroundColor (KptTheme.colorScheme.surfaceVariant) making dividers invisible;
update the default for the borderColor parameter in the MifosTable function
signature to a higher‑contrast token (for example KptTheme.colorScheme.outline
or KptTheme.colorScheme.onSurfaceVariant with a low alpha) so row separators are
visible against surfaceVariant, and ensure any usages that rely on the old
default still render correctly.
In `@core/ui/build.gradle.kts`:
- Around line 52-57: Update the openhtmltopdf dependencies used in desktopMain
to the current 1.1.37 release: replace the existing references for
openhtmltopdf.pdfbox and openhtmltopdf.svg.support (the identifiers used in the
build file) to point at version 1.1.37 in your version catalog (or hardcode
1.1.37 if you manage versions inline); ensure the artifacts referenced as
implementation(libs.openhtmltopdf.pdfbox) and
implementation(libs.openhtmltopdf.svg.support) resolve to 1.1.37 so the JVM-only
desktopMain uses the updated openhtmltopdf 1.1.37 artifacts.
In `@core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PageConfig.kt`:
- Around line 31-35: PageConfig currently allows negative marginMm which can
produce invalid `@page` CSS; add validation in the PageConfig constructor (e.g.,
an init block or require call) to reject negative values by throwing an
IllegalArgumentException with a clear message referencing marginMm. Locate the
data class PageConfig (alongside PageSize and Orientation) and add
require(marginMm >= 0) (or equivalent) to enforce non‑negative margins and
update any callers/tests that pass negative margins.
In `@core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.kt`:
- Around line 65-85: The onload handler for the iframe is being set after
writing/closing the frame document which can miss a synchronously fired load
event; move the iframe.onload assignment (the lambda that calls
iframe.contentWindow?.let { it.print(); completionSignal.complete(Unit) } or
completes exceptionally) so it is attached before calling
frameDoc.write(finalHtml) and frameDoc.close(), ensuring the handler is in place
prior to writing/closing the document and keeping use of completionSignal for
success/failure as-is.
In
`@core/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.native.kt`:
- Around line 21-27: The native iOS implementation of generateAndSharePdf
currently calls TODO(...) which throws at runtime; replace this with a safe
interim fallback: in PdfGenerator.native.kt implement actual suspend fun
generateAndSharePdf(htmlContent: String, fileName: String, pageConfig:
PageConfig) to either (a) return early after logging a clear warning/error that
PDF export is unsupported on Native/iOS yet, or (b) throw a documented
UnsupportedOperationException that callers can catch; reference the
generateAndSharePdf function and PageConfig to locate the code and ensure the
fallback does not perform a hard TODO() crash and is gated so feature paths on
Native/iOS fail gracefully until a full
UIPrintPageRenderer/UIMarkupTextPrintFormatter implementation is added.
In
`@core/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.kt`:
- Around line 67-87: The await on completionSignal (completionSignal.await())
can hang if the iframe never calls its onload/print path; modify the
PdfGenerator wasmJs flow to add a timeout and explicit iframe error path: wrap
the wait in a cancellable timeout (e.g., use withTimeout or start a timer) and
on timeout completeExceptionally the same CompletableDeferred used by the iframe
handler, add iframe.onerror/onabort handlers that call
completionSignal.completeExceptionally with a descriptive exception, and ensure
all handlers/timers are cleaned up (remove onload/onerror and cancel timer)
after completion to avoid leaks; update the logic around iframe.onload, the
setTimeout callback, and any caller of completionSignal to handle the timeout
exception.
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 284: The string resource named "principle_paid_off" contains a typo in
its value ("Principle Paid Off"); update the value to the correct financial term
"Principal Paid Off" in the <string name="principle_paid_off"> entry, and search
for any other occurrences of the misspelled word in resource files or UI text to
correct them as well.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt`:
- Around line 181-211: mapToTableData uses loan.currency.code which is nullable;
ensure you replace uses of currencyCode with a non-null fallback (e.g.
currencyCode ?: "USD") before calling CurrencyFormatter.format and before
passing into mapPeriodsData and calculateTotals to avoid NPEs. Update
LoanRepaymentScheduleViewModel.mapToTableData so that the local currency
variable is the guarded value and use that guarded variable in all calls to
CurrencyFormatter.format, mapPeriodsData, and calculateTotals.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt`:
- Around line 218-233: The h1 CSS inside getAdditionalStyles() contains an
invalid fragment "solid `#1976d2`" (missing property name and width); update the
h1 rule in RepaymentScheduleHtmlGenerator.getAdditionalStyles() to use a
complete declaration such as "border-bottom: 1px solid `#1976d2`;" (or remove the
fragment) so the CSS is valid and the method returns a correct stylesheet
string.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Line 24: The table header currently imports and uses
feature_loan_table_header_principal_due in LoanTransactionsScreen.kt which
labels the transaction column as “Principal Due” even though the column renders
transaction values (row.principal); replace that import/use with a
transaction-specific header (e.g.,
feature_loan_table_header_principal_transaction or similar string key) and
update the header reference wherever LoanTransactionsScreen (and its column
rendering row.principal) sets the column label; if that transaction-oriented
string key was removed, restore it in
feature/loan/src/commonMain/composeResources/values/strings.xml with an
appropriate key/name for transaction principal header so the UI uses the correct
string.
In `@gradle/libs.versions.toml`:
- Around line 112-113: Update the openhtmltopdf dependency entry in
libs.versions.toml by changing the value for the openhtmltopdf key from "1.0.10"
to "1.1.37" so the project uses the latest stable release; locate the
openhtmltopdf = "1.0.10" line and replace the version string with "1.1.37".
---
Nitpick comments:
In
`@core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt`:
- Around line 41-44: Update the Text composable in MifosDetailsCard where the
heading string is built as "$heading :"; change it to remove the space before
the colon so it reads "$heading:" (or "$heading: " if you prefer a trailing
space) to match conventional punctuation; locate the Text call that uses
MifosTypography.labelMediumEmphasized and adjust the interpolated string
accordingly.
- Around line 33-51: Add a KDoc comment to MifosDetailsCard above its
declaration indicating that the component iterates the provided details map in
its natural iteration order (so insertion order is preserved when callers pass a
LinkedHashMap or use mapOf()), and that using a plain HashMap will produce
unpredictable display order; mention callers should use LinkedHashMap/mapOf if a
stable order is required. Reference the MifosDetailsCard component and its
details parameter in the KDoc so readers know this behavior applies to the
forEach-driven rendering.
In
`@core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/HtmlTemplateGenerator.kt`:
- Around line 142-151: The date formatting in getGenerationDateText is hardcoded
as "${currentDate.day}/${currentDate.month.number}/${currentDate.year}" which is
not locale-safe; change it to produce a consistent ISO-8601 date (yyyy-MM-dd) or
use a locale-aware formatter instead. Update getGenerationDateText to derive the
date string from the LocalDate's ISO representation (or, if you need
locale-specific presentation, use a platform DateTimeFormatter with the current
locale) and keep the rest of the method (generationDateLabel assembly) unchanged
so the label from getString(Res.string.feature_pdf_generation_date) is
preserved.
In `@core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.kt`:
- Around line 88-90: The catch block in PdfGenerator (the exception handler
shown in PdfGenerator.js.kt) currently calls println("Error generating PDF on
Web: ${e.message}"); replace this with a JS-appropriate structured log call—use
console.error with the error and message (or integrate the project's logging
utility if available) so errors appear in browser devtools; update the catch in
the PDF generation function to call console.error("Error generating PDF on
Web:", e) or route through the existing logger instead of println and keep
rethrowing the exception.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`:
- Around line 494-503: The preview PreviewLoanRepaymentSchedule currently builds
LoanRepaymentScheduleState() which defaults to ScreenState.Loading and shows
nothing; update PreviewLoanRepaymentSchedule to construct and pass a meaningful
LoanRepaymentScheduleState representing a ScreenState.Success with mocked loan
repayment data (e.g., sample schedule items, totals, and any flags used by
LoanRepaymentScheduleScreenContent) so the composable renders useful content;
locate PreviewLoanRepaymentSchedule and replace the default state with a
fabricated success state instance matching the state model used by
LoanRepaymentScheduleScreenContent.
- Around line 186-193: The null-check around repaymentScheduleTableData in the
LoanRepaymentScheduleState.ScreenState.Success branch is redundant; when the
ViewModel emits Success it guarantees repaymentScheduleTableData is non-null.
Replace the safe-let with a direct use of the non-null value (e.g., pass
state.repaymentScheduleTableData!! to LoanRepaymentScheduleContent) and add a
short comment referencing the ViewModel invariant, or better yet update the
state model so Success carries a non-null tableData (change
LoanRepaymentScheduleState.ScreenState.Success to include a non-null
repaymentScheduleTableData property) and then call
LoanRepaymentScheduleContent(tableData = state.repaymentScheduleTableData,
basicDetails = state.basicDetails) without a null check.
- Around line 103-135: The PDF export handling is fragile: it uses
e.printStackTrace(), catches a generic Exception, and launches a coroutine via
coroutineScope.launch inside LaunchedEffect which can outlive the composable;
fix by moving the PDF generation out of the UI effect into a lifecycle-safe
place (e.g., add a viewModel.exportRepaymentSchedulePdf(data) or a suspend
viewModel.generateAndSharePdf(...) that calls createPdfStrings,
RepaymentScheduleHtmlGenerator, and pdfGenerator.generateAndSharePdf), replace
coroutineScope.launch with a call to that viewModel suspend function from
LaunchedEffect (or use launch tied to the LaunchedEffect coroutine scope) so
cancellation follows the composable lifecycle, catch and handle specific
exceptions (e.g., IOException, PdfGenerationException) instead of Exception, and
replace e.printStackTrace() with structured logging or include e.message/stack
in the LoanRepaymentScheduleAction.PdfExportError payload when calling
viewModel.trySendAction; reference functions/classes: LaunchedEffect,
viewModel.eventFlow, coroutineScope.launch, createPdfStrings,
RepaymentScheduleHtmlGenerator, pdfGenerator.generateAndSharePdf, and
LoanRepaymentScheduleAction.PdfExportError.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt`:
- Around line 104-123: In handleNetworkStatus, remove the unnecessary
viewModelScope.launch wrapper for the non-suspending offline branch to avoid
racing coroutines: perform the mutableStateFlow.update synchronously when
!isOnline (use the existing conditional update using
LoanRepaymentScheduleState.ScreenState checks) and only start a coroutine
(viewModelScope.launch) for the path that needs suspension by calling
loadLoanRepaySchedule(); keep references: handleNetworkStatus,
mutableStateFlow.update, LoanRepaymentScheduleState.ScreenState,
viewModelScope.launch, and loadLoanRepaySchedule().
In `@gradle.properties`:
- Line 19: The org.gradle.jvmargs setting currently hard-codes high memory
(-Xmx8g and -XX:MaxMetaspaceSize=1g); change this to more conservative defaults
(for example -Xmx1g or -Xmx2g and remove or reduce MaxMetaspaceSize) so CI and
low-memory dev machines aren’t forced to allocate 8GB, and document/howto
override locally (e.g., via GRADLE_OPTS or a local gradle.properties) so
machines that need larger heaps can opt-in; update the org.gradle.jvmargs line
and add a brief comment explaining how to override.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/ui/src/commonMain/composeResources/drawable/ic_icon_mifos_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (33)
cmp-android/dependencies/demoDebugRuntimeClasspath.txtcmp-android/dependencies/demoReleaseRuntimeClasspath.txtcmp-android/dependencies/prodDebugRuntimeClasspath.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/ui/build.gradle.ktscore/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.android.ktcore/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.android.ktcore/ui/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/HtmlTemplateGenerator.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PageConfig.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.ktcore/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.desktop.ktcore/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.desktop.ktcore/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.ktcore/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.js.ktcore/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.native.ktcore/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.native.ktcore/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.ktcore/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.wasmJs.ktfeature/loan/build.gradle.ktsfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleUiState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.ktgradle.propertiesgradle/libs.versions.toml
💤 Files with no reviewable changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleUiState.kt
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt
Outdated
Show resolved
Hide resolved
core/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.native.kt
Show resolved
Hide resolved
feature/loan/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
...onMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt
Show resolved
Hide resolved
...in/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
Contributor
|
@techsavvy185 Please resolve the merge conflicts. |
techsavvy185
force-pushed
the
loanPdfExport
branch
from
d175b2c to
a5c2092
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (1)
24-24:⚠️ Potential issue | 🟡 MinorUse a transaction-specific principal label here.
This column renders
row.principal, so “Principal Due” is misleading on the loan transactions table.💡 Suggested fix
-import androidclient.feature.loan.generated.resources.feature_loan_table_header_principal_due +import androidclient.feature.loan.generated.resources.feature_loan_table_header_principal ... - stringResource(Res.string.feature_loan_table_header_principal_due), + stringResource(Res.string.feature_loan_table_header_principal),Also applies to: 255-255
🤖 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/loanTransaction/LoanTransactionsScreen.kt` at line 24, The import and header resource feature_loan_table_header_principal_due used in LoanTransactionsScreen is misleading because the column renders row.principal (transaction principal) — replace the import and any usage of feature_loan_table_header_principal_due with a transaction-specific label (e.g., a new resource like feature_loan_transactions_table_header_principal or the existing transaction-principal resource), update the import statement and the column header reference in LoanTransactionsScreen (and the duplicate usage at the other occurrence) so the UI reads a transaction-specific "Principal" label rather than "Principal Due".core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt (1)
41-46:⚠️ Potential issue | 🟠 MajorUse a contrasting default for
borderColor.Line 46 still defaults
borderColorto the same token asbackgroundColor, so the row dividers disappear unless every caller overrides it.💡 Proposed fix
- borderColor: Color = KptTheme.colorScheme.surfaceVariant, + borderColor: Color = KptTheme.colorScheme.outlineVariant,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt` around lines 41 - 46, The default borderColor currently equals backgroundColor (backgroundColor: KptTheme.colorScheme.surfaceVariant, borderColor: KptTheme.colorScheme.surfaceVariant) which hides row dividers; change the default for the borderColor parameter in MifosTable (the function/constructor using backgroundColor, cornerShape, showTopBorder, showBottomBorder, showSideBorders) to a contrasting token such as KptTheme.colorScheme.outline or KptTheme.colorScheme.onSurfaceVariant so dividers are visible by default, keeping other parameter names (backgroundColor, borderColor) unchanged so callers that override still work.feature/loan/src/commonMain/composeResources/values/strings.xml (1)
326-326:⚠️ Potential issue | 🟡 MinorFix the financial term typo.
“Principle Paid Off” should be “Principal Paid Off”. This shared resource is surfaced in the repayment summary and the generated PDF.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/composeResources/values/strings.xml` at line 326, The string resource with name "principle_paid_off" contains a typo in its value; update the value for the resource string "principle_paid_off" to read "Principal Paid Off" (capitalization preserved) so the repayment summary and generated PDF display the correct financial term.core/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.kt (1)
67-87:⚠️ Potential issue | 🟠 MajorGuard the iframe completion path with timeout/error handlers.
If the iframe never reaches the load/print path,
completionSignal.await()never completes and the export flow hangs indefinitely. Addonerror/onabortcompletion plus a timeout around the await.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.kt` around lines 67 - 87, The iframe print completion currently only resolves via iframe.onload and then completionSignal.await(), which can hang if load/print never occur; update the logic around completionSignal, iframe.onload, and frameDoc.close to add iframe.onerror and iframe.onabort handlers that completeExceptionally the completionSignal, and wrap the await with a cancellable timeout (e.g., schedule a timer that completesExceptionally on timeout and is cancelled on success) so that any error, abort, or timeout will unblock completionSignal.await(); ensure handlers check completionSignal.isCompleted before completing to avoid races.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt (1)
181-210:⚠️ Potential issue | 🔴 CriticalNormalize
currencyCodebefore passing it downstream.
loan.currency.codeis nullable here, but it is forwarded straight intoCurrencyFormatter.format()and intomapPeriodsData/calculateTotals. A loan missing currency metadata can still crash this screen and the PDF path on platforms whose formatter expects a non-null ISO code. Normalize once inmapToTableDataand pass the guarded value through the rest of the mapping. Based on learnings: When a Kotlin function parameter is nullable and downstream calls require a non-null value, add null-safety handling in all implementations.🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt` around lines 181 - 210, mapToTableData currently reads a nullable loan.currency.code into currencyCode and forwards it into CurrencyFormatter.format, mapPeriodsData, and calculateTotals risking NPEs; normalize currencyCode at the top of mapToTableData (e.g., fallback to a safe default like an empty string or "USD" per app conventions) and then pass that non-null guarded value into CurrencyFormatter.format, mapPeriodsData(periods, currencyCode, maxDigits), and calculateTotals(periods, currencyCode, maxDigits) so all downstream calls receive a non-null ISO code; update any local variable name usage in mapToTableData to use the normalized value.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt (1)
218-223:⚠️ Potential issue | 🟡 MinorFix the invalid
h1CSS declaration.
solid#1976d2`` is not a valid CSS property, so that part of the heading style is ignored in the generated HTML. Replace it with a complete declaration such asborder-bottom: 1px solid#1976d2`;`.🤖 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/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt` around lines 218 - 223, In getAdditionalStyles() of RepaymentScheduleHtmlGenerator the h1 rule contains an invalid token "solid `#1976d2`" which is ignored; replace that fragment with a proper declaration such as "border-bottom: 1px solid `#1976d2`;" so the heading gets the intended bottom border (update the h1 declaration inside the returned CSS string in getAdditionalStyles()).
🧹 Nitpick comments (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt (1)
26-27: Consider usingLinkedHashMapor documenting iteration order requirement.The
Map<String, String?>parameter doesn't guarantee iteration order. If display order matters (which it typically does for detail cards), callers must pass an ordered map implementation likeLinkedHashMap(whichmapOf()returns). Consider either:
- Documenting this expectation in a KDoc comment, or
- Changing the parameter type to
List<Pair<String, String?>>to make ordering explicit.📝 Option: Use List for explicit ordering
`@Composable` fun MifosDetailsCard( - details: Map<String, String?>, + details: List<Pair<String, String?>>, modifier: Modifier = Modifier, ) { MifosCard( modifier = modifier.fillMaxWidth(), ) { Column(modifier = Modifier.padding(KptTheme.spacing.md)) { - details.forEach { (heading, value) -> + details.forEach { (heading, value) -> Row(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt` around lines 26 - 27, The MifosDetailsCard composable accepts details: Map<String, String?> which does not guarantee iteration order; update the API so ordering is explicit: either change the parameter to details: List<Pair<String, String?>> (and update all call sites to pass an ordered list) or constrain it to an ordered map type (e.g., LinkedHashMap) and update the parameter type accordingly, or add a KDoc to the MifosDetailsCard declaration stating that callers must pass an ordered map (such as LinkedHashMap or the mapOf() result) if display order matters; reference the MifosDetailsCard function and its details parameter when making the change so all usages are adjusted.gradle.properties (2)
19-19: High memory allocation may fail on developer machines with limited RAM.Allocating 8GB to the Gradle daemon is aggressive and could cause issues for developers with less available memory. Consider whether 4-6GB would suffice, or document the minimum RAM requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle.properties` at line 19, The org.gradle.jvmargs setting currently reserves -Xmx8g which can cause failures on machines with limited RAM; change the value in the org.gradle.jvmargs property to a lower default (e.g., -Xmx4g or -Xmx6g) or make it configurable via an environment variable/CI override, and update accompanying project documentation to state the minimum recommended RAM for development so contributors know requirements.
30-38: Consider whether these Kotlin/Native settings are necessary for this PR.This PR implements PDF export functionality, but introduces substantial build configuration changes (Kotlin/Native cache disabled, high memory settings, experimental flags). If these settings aren't required for PDF export, consider splitting them into a separate infrastructure PR to isolate concerns and simplify review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle.properties` around lines 30 - 38, This change bundles broad Kotlin/Native build flags (org.jetbrains.compose.experimental.jscanvas.enabled, kotlin.native.cacheKind, kotlin.native.jvmArgs, kotlin.native.disableCompilerDaemon, kotlin.kmp.unresolvedDependenciesDiagnostic, kotlin.apple.xcodeCompatibility.nowarn) with the PDF export feature; revert or extract those settings into a separate infrastructure/configuration commit or PR and keep this PR focused on PDF export-only changes. Specifically, remove or rollback the kotlin.native.* and org.jetbrains.compose.* lines from this PR, create a dedicated commit/PR that documents why each flag is needed (with benchmarks or reproduction steps), and update this PR’s description to state it only contains PDF export logic so reviewers can evaluate functionality without unrelated build-system changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt`:
- Line 47: The onClick parameter in MifosTable (currently declared as onClick:
() -> Unit = {}) should be changed to a nullable callback (() -> Unit)? = null
to avoid adding click semantics to static/header rows; update the MifosTable
parameter signature accordingly and only apply the Modifier.clickable(...) when
onClick is non-null (e.g., check onClick != null before adding clickable) so
ripple, semantics and accessibility are only present for actionable rows.
In `@core/ui/build.gradle.kts`:
- Around line 55-56: The kotlinx-html dependency is currently declared as
implementation, so downstream modules extending HtmlTemplateGenerator (which
exposes the BODY type in its protected abstract fun BODY.generateBody()) cannot
see the BODY type; change the dependency declaration in core/ui's
build.gradle.kts from implementation(libs.kotlinx.html) to
api(libs.kotlinx.html) so kotlinx-html is exported on the compile classpath
(this will allow classes like RepaymentScheduleHtmlGenerator to compile without
re-declaring kotlinx-html).
In
`@core/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.android.kt`:
- Around line 81-83: The current code resumes the coroutine and defers WebView
cleanup immediately after calling printManager.print(fileName, nativeAdapter,
attributes), which can destroy the WebView before the PrintDocumentAdapter
callbacks finish; change this to capture the PrintJob returned by
printManager.print(...), then wait for the PrintJob to reach a terminal state
(via PrintJob.isCompleted/isFailed/isCancelled or a PrintJobStateChange
listener) before resuming the continuation and destroying the WebView (do not
rely on post()); update both places using
printManager.print/nativeAdapter/continuation to poll or register a completion
callback and only then resume Unit and call webView.destroy().
In
`@core/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.desktop.kt`:
- Around line 48-59: Replace the pre-check using file.canWrite() with an actual
write attempt and map failures to the user dialog: try to open the target file
for write (e.g., RandomAccessFile(file, "rw") or FileOutputStream(file).use { }
/ file.outputStream().use { } ) inside a try block and close it immediately; if
that open fails with IOException, FileNotFoundException, or
OverlappingFileLockException, show the same JOptionPane.showMessageDialog
message and return null, otherwise return file. Update the code around the file
variable (where canWrite() is currently used) to catch those exceptions and
display the permission/locked-file dialog instead of relying on canWrite().
In `@core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.kt`:
- Around line 67-73: Inside the iframe.onload handler around the call to
iframe.contentWindow?.let { it.print(); completionSignal.complete(Unit) },
replace the current flow so that you attach an "afterprint" event listener to
iframe.contentWindow that will complete completionSignal and perform cleanup
(remove the listener and the iframe) when fired; still start the existing
2-second fallback timeout but only as a backup if "afterprint" doesn't fire, and
ensure you clear the fallback timeout when the "afterprint" handler runs to
avoid double cleanup.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt`:
- Around line 137-178: The loadLoanRepaySchedule function can spawn overlapping
collectors; fix it by tracking and cancelling the previous load job before
starting a new one (add a Job property like loadLoanRepayJob on the ViewModel),
or alternatively use Flow.collectLatest on repository.getLoanRepaySchedule to
ensure only the latest emission is handled; in either approach cancel/replace
the previous job or switch to collectLatest inside loadLoanRepaySchedule so
repository.getLoanRepaySchedule(state.loanId) no longer produces concurrent
collectors that race updates to mutableStateFlow and fields like
repaymentScheduleTableData and screenState.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt`:
- Around line 155-167: In RepaymentScheduleHtmlGenerator.kt, the
disbursement-row currently merges columns using large colspans which shifts
values left; update the tr(classes = "disbursement-row") block to emit one td
per table column (no large colspan grouping) so tableData.disbursementDate and
tableData.loanAmount are placed in the specific Date and Loan Balance column
cells respectively (fill other columns with empty td entries as needed), or
alternatively adjust the colspan values so the disbursementDate cell aligns
under the Date column and the loanAmount cell aligns under the Loan Balance
column; locate the tr(classes = "disbursement-row") and replace the grouped
td(colspan=...) usage around tableData.disbursementDate and tableData.loanAmount
accordingly.
---
Duplicate comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt`:
- Around line 41-46: The default borderColor currently equals backgroundColor
(backgroundColor: KptTheme.colorScheme.surfaceVariant, borderColor:
KptTheme.colorScheme.surfaceVariant) which hides row dividers; change the
default for the borderColor parameter in MifosTable (the function/constructor
using backgroundColor, cornerShape, showTopBorder, showBottomBorder,
showSideBorders) to a contrasting token such as KptTheme.colorScheme.outline or
KptTheme.colorScheme.onSurfaceVariant so dividers are visible by default,
keeping other parameter names (backgroundColor, borderColor) unchanged so
callers that override still work.
In
`@core/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.kt`:
- Around line 67-87: The iframe print completion currently only resolves via
iframe.onload and then completionSignal.await(), which can hang if load/print
never occur; update the logic around completionSignal, iframe.onload, and
frameDoc.close to add iframe.onerror and iframe.onabort handlers that
completeExceptionally the completionSignal, and wrap the await with a
cancellable timeout (e.g., schedule a timer that completesExceptionally on
timeout and is cancelled on success) so that any error, abort, or timeout will
unblock completionSignal.await(); ensure handlers check
completionSignal.isCompleted before completing to avoid races.
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 326: The string resource with name "principle_paid_off" contains a typo
in its value; update the value for the resource string "principle_paid_off" to
read "Principal Paid Off" (capitalization preserved) so the repayment summary
and generated PDF display the correct financial term.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt`:
- Around line 181-210: mapToTableData currently reads a nullable
loan.currency.code into currencyCode and forwards it into
CurrencyFormatter.format, mapPeriodsData, and calculateTotals risking NPEs;
normalize currencyCode at the top of mapToTableData (e.g., fallback to a safe
default like an empty string or "USD" per app conventions) and then pass that
non-null guarded value into CurrencyFormatter.format, mapPeriodsData(periods,
currencyCode, maxDigits), and calculateTotals(periods, currencyCode, maxDigits)
so all downstream calls receive a non-null ISO code; update any local variable
name usage in mapToTableData to use the normalized value.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt`:
- Around line 218-223: In getAdditionalStyles() of
RepaymentScheduleHtmlGenerator the h1 rule contains an invalid token "solid
`#1976d2`" which is ignored; replace that fragment with a proper declaration such
as "border-bottom: 1px solid `#1976d2`;" so the heading gets the intended bottom
border (update the h1 declaration inside the returned CSS string in
getAdditionalStyles()).
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Line 24: The import and header resource
feature_loan_table_header_principal_due used in LoanTransactionsScreen is
misleading because the column renders row.principal (transaction principal) —
replace the import and any usage of feature_loan_table_header_principal_due with
a transaction-specific label (e.g., a new resource like
feature_loan_transactions_table_header_principal or the existing
transaction-principal resource), update the import statement and the column
header reference in LoanTransactionsScreen (and the duplicate usage at the other
occurrence) so the UI reads a transaction-specific "Principal" label rather than
"Principal Due".
---
Nitpick comments:
In
`@core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.kt`:
- Around line 26-27: The MifosDetailsCard composable accepts details:
Map<String, String?> which does not guarantee iteration order; update the API so
ordering is explicit: either change the parameter to details: List<Pair<String,
String?>> (and update all call sites to pass an ordered list) or constrain it to
an ordered map type (e.g., LinkedHashMap) and update the parameter type
accordingly, or add a KDoc to the MifosDetailsCard declaration stating that
callers must pass an ordered map (such as LinkedHashMap or the mapOf() result)
if display order matters; reference the MifosDetailsCard function and its
details parameter when making the change so all usages are adjusted.
In `@gradle.properties`:
- Line 19: The org.gradle.jvmargs setting currently reserves -Xmx8g which can
cause failures on machines with limited RAM; change the value in the
org.gradle.jvmargs property to a lower default (e.g., -Xmx4g or -Xmx6g) or make
it configurable via an environment variable/CI override, and update accompanying
project documentation to state the minimum recommended RAM for development so
contributors know requirements.
- Around line 30-38: This change bundles broad Kotlin/Native build flags
(org.jetbrains.compose.experimental.jscanvas.enabled, kotlin.native.cacheKind,
kotlin.native.jvmArgs, kotlin.native.disableCompilerDaemon,
kotlin.kmp.unresolvedDependenciesDiagnostic,
kotlin.apple.xcodeCompatibility.nowarn) with the PDF export feature; revert or
extract those settings into a separate infrastructure/configuration commit or PR
and keep this PR focused on PDF export-only changes. Specifically, remove or
rollback the kotlin.native.* and org.jetbrains.compose.* lines from this PR,
create a dedicated commit/PR that documents why each flag is needed (with
benchmarks or reproduction steps), and update this PR’s description to state it
only contains PDF export logic so reviewers can evaluate functionality without
unrelated build-system changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d290179-94a3-4e49-9751-49b1405dbce5
⛔ Files ignored due to path filters (1)
core/ui/src/commonMain/composeResources/drawable/ic_icon_mifos_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (36)
cmp-android/dependencies/demoDebugRuntimeClasspath.txtcmp-android/dependencies/demoReleaseRuntimeClasspath.txtcmp-android/dependencies/prodDebugRuntimeClasspath.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/ui/build.gradle.ktscore/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.android.ktcore/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.android.ktcore/ui/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosDetailsCard.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/HtmlTemplateGenerator.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PageConfig.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.ktcore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.ktcore/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.desktop.ktcore/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.desktop.ktcore/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.js.ktcore/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.js.ktcore/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.native.ktcore/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.native.ktcore/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.wasmJs.ktcore/ui/src/wasmJsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.wasmJs.ktfeature/loan/build.gradle.ktsfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/component/RepaymentPeriodCard.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleUiState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.ktgradle.propertiesgradle/libs.versions.toml
💤 Files with no reviewable changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleUiState.kt
🚧 Files skipped from review as they are similar to previous changes (12)
- cmp-android/dependencies/prodReleaseRuntimeClasspath.txt
- cmp-android/dependencies/demoReleaseRuntimeClasspath.txt
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
- core/ui/src/nativeMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.native.kt
- core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.kt
- core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PageConfig.kt
- core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.js.kt
- cmp-android/dependencies/demoDebugRuntimeClasspath.txt
- feature/loan/build.gradle.kts
- cmp-android/dependencies/prodDebugRuntimeClasspath.txt
- core/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGeneratorHelper.desktop.kt
- core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.kt
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt
Show resolved
Hide resolved
core/ui/src/androidMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.android.kt
Show resolved
Hide resolved
core/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/pdf/PdfGenerator.desktop.kt
Show resolved
Hide resolved
...onMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt
Show resolved
Hide resolved
...in/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/pdf/RepaymentScheduleHtmlGenerator.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (2)
370-388: Consider extracting column count to avoid magic number duplication.The column count
14is duplicated across multiple composables (columnWidths,headers,List(14)here,periodValues,footerValues). If the table structure changes, this could lead to inconsistencies.♻️ Optional refactor to derive count from headers
private const val COLUMN_COUNT = 14 // Or derive from headers list in a shared location🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 370 - 388, The code uses a hardcoded column count (List(14)) in the cells creation which is duplicated across columnWidths, headers, periodValues, and footerValues; replace the magic number by introducing a single shared constant or deriving the count from an authoritative source (e.g., headers.size) and use that constant/derived value wherever List(14) and other column-related lists are constructed (update the cells variable, columnWidths, headers, periodValues, footerValues to reference the single COLUMN_COUNT or headers.size to keep them consistent).
162-169: Export button is clickable even when data is unavailable.The export button is always enabled regardless of the data state. When clicked during
LoadingorErrorstates, the export action will silently fail sincerepaymentScheduleTableDatais null. Consider conditionally enabling the button.♻️ Suggested approach
actions = { - IconButton(onClick = { onAction(LoanRepaymentScheduleAction.ExportToPdf) }) { + IconButton( + onClick = { onAction(LoanRepaymentScheduleAction.ExportToPdf) }, + enabled = state.repaymentScheduleTableData != null, + ) { Icon( imageVector = MifosIcons.Export, contentDescription = stringResource(Res.string.feature_loan_export_to_pdf), ) } },🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 162 - 169, The export IconButton is always enabled and can trigger onAction(LoanRepaymentScheduleAction.ExportToPdf) when repaymentScheduleTableData is null (e.g., UI states Loading or Error); update the composable so IconButton is enabled only when repaymentScheduleTableData is non-null and state indicates success (check the UI state enum/case, e.g., not Loading/Error), by adding an enabled = repaymentScheduleTableData != null (or equivalent isSuccess check) to the IconButton and guard the onClick to no-op unless repaymentScheduleTableData is present to prevent silent failures.
🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`:
- Around line 108-133: The ExportPdf handler currently no-ops when
state.repaymentScheduleTableData is null and uses e.printStackTrace(); change it
so when repaymentScheduleTableData is null you immediately send a user-facing
error action (use viewModel.trySendAction with
LoanRepaymentScheduleAction.PdfExportError) instead of silently returning, and
replace e.printStackTrace() with a proper log call that includes the exception
(e.g., logger.error or processLogger.error) and include the exception
message/details when sending the PdfExportError so users get feedback if
pdfGenerator.generateAndSharePdf or RepaymentScheduleHtmlGenerator fails.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`:
- Around line 370-388: The code uses a hardcoded column count (List(14)) in the
cells creation which is duplicated across columnWidths, headers, periodValues,
and footerValues; replace the magic number by introducing a single shared
constant or deriving the count from an authoritative source (e.g., headers.size)
and use that constant/derived value wherever List(14) and other column-related
lists are constructed (update the cells variable, columnWidths, headers,
periodValues, footerValues to reference the single COLUMN_COUNT or headers.size
to keep them consistent).
- Around line 162-169: The export IconButton is always enabled and can trigger
onAction(LoanRepaymentScheduleAction.ExportToPdf) when
repaymentScheduleTableData is null (e.g., UI states Loading or Error); update
the composable so IconButton is enabled only when repaymentScheduleTableData is
non-null and state indicates success (check the UI state enum/case, e.g., not
Loading/Error), by adding an enabled = repaymentScheduleTableData != null (or
equivalent isSuccess check) to the IconButton and guard the onClick to no-op
unless repaymentScheduleTableData is present to prevent silent failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b95ed568-9e59-414b-bca7-b7aa029d0470
📒 Files selected for processing (6)
cmp-android/dependencies/demoDebugRuntimeClasspath.txtcmp-android/dependencies/demoReleaseRuntimeClasspath.txtcmp-android/dependencies/prodDebugRuntimeClasspath.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- cmp-android/dependencies/demoReleaseRuntimeClasspath.txt
- cmp-android/dependencies/demoDebugRuntimeClasspath.txt
- cmp-android/dependencies/prodReleaseRuntimeClasspath.txt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt
...ommonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt
Show resolved
Hide resolved
Contributor
|
@techsavvy185 Please address the @coderabbitai suggestions relevant to this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 116712
🔴 Critical / High Priority
🟡 Minor / Nitpick
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the CodeRabbit review comments on this PR first. 🏁 Script executed: Length of output: 39767 Let me fetch the inline review comments specifically. 🏁 Script executed: Length of output: 13 🏁 Script executed: Length of output: 242 🏁 Script executed: Length of output: 133678
🔴 Critical1.
|
| File | Issue |
|---|---|
MifosDetailsCard.kt L41 |
Remove the space before colon: "$heading :" → "$heading:" |
MifosDetailsCard.kt L33 |
Add KDoc noting that map iteration order determines display order |
HtmlTemplateGenerator.kt L142 |
Date format d/M/yyyy is locale-dependent; use ISO-8601 yyyy-MM-dd |
LoanRepaymentScheduleScreen.kt L494 |
Preview defaults to ScreenState.Loading; provide mock Success state for useful previews |
LoanRepaymentScheduleScreen.kt L186 |
Redundant null check in ScreenState.Success branch; repaymentScheduleTableData is guaranteed non-null in that state |
LoanRepaymentScheduleScreen.kt L370 |
Hardcoded column count 14 duplicated across composables; extract as const val COLUMN_COUNT = 14 |
PdfGenerator.js.kt L67 |
Use afterprint event for iframe cleanup instead of a fixed 2-second timeout |
gradle.properties L19 |
-Xmx8g is aggressive; consider -Xmx6g or making it configurable |
gradle.properties L30–38 |
Kotlin/Native build flags are unrelated to PDF export; consider splitting into a separate infrastructure PR |
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt (2)
183-211:⚠️ Potential issue | 🔴 CriticalNormalize
currencyCodebefore formatting.
loan.currency.codeis still nullable here, but the formatter path on some targets expects a non-null code. Passing it throughloanAmount,principalPaid,mapPeriodsData, andcalculateTotalskeeps the NPE risk in place. Normalize it once in common code and store the guarded value in state.Suggested fix
- val currencyCode = loan.currency.code + val currencyCode = loan.currency.code ?: "USD" val maxDigits = loan.currency.decimalPlacesBased on learnings: when a Kotlin parameter like
currencyCodeis nullable, normalize it before calling downstream currency formatters that require a non-null value.🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt` around lines 183 - 211, In mapToTableData (LoanRepaymentScheduleViewModel), guard the nullable loan.currency.code before any formatting by normalizing it (e.g., val currencyCode = loan.currency.code ?: "") and use that non-null/default value when calling CurrencyFormatter.format, mapPeriodsData, calculateTotals and when storing currencyCode in the returned LoanRepaymentScheduleState.RepaymentScheduleTableData so downstream functions receive a non-null string; update references to currencyCode in mapToTableData accordingly and ensure any signatures/uses of mapPeriodsData and calculateTotals expect a non-null String if needed.
139-180:⚠️ Potential issue | 🟠 MajorKeep a single repayment-schedule collector alive.
Every
Retry/ online transition starts a freshcollectwithout cancelling the previous one, so multiple repository streams can racedataState,basicDetails, andrepaymentScheduleTableData.Suggested fix
+import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.collectLatest + class LoanRepaymentScheduleViewModel( private val repository: LoanRepaymentScheduleRepository, private val networkMonitor: NetworkMonitor, savedStateHandle: SavedStateHandle, ) : BaseViewModel<LoanRepaymentScheduleState, LoanRepaymentScheduleEvent, LoanRepaymentScheduleAction>( @@ ) { + private var loadLoanRepayScheduleJob: Job? = null + init { observeNetwork() } @@ private fun loadLoanRepaySchedule() { mutableStateFlow.update { it.copy(dataState = DataState.Loading) } - viewModelScope.launch { - repository.getLoanRepaySchedule(state.loanId).collect { dataState -> + loadLoanRepayScheduleJob?.cancel() + loadLoanRepayScheduleJob = viewModelScope.launch { + repository.getLoanRepaySchedule(state.loanId).collectLatest { dataState -> when (dataState) {🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt` around lines 139 - 180, loadLoanRepaySchedule currently starts a new collector on repository.getLoanRepaySchedule every time (via viewModelScope.launch { ... collect { ... } }), which allows multiple concurrent collectors to race updating mutableStateFlow; fix by ensuring only one active collector: either replace collect with collectLatest on repository.getLoanRepaySchedule(...) inside loadLoanRepaySchedule so new emissions/calls cancel the previous handling, or implement a Job property (e.g., private var repayScheduleJob: Job?) and before launching call repayScheduleJob?.cancel() then assign repayScheduleJob = viewModelScope.launch { repository.getLoanRepaySchedule(state.loanId).collect { ... } } so previous coroutine is cancelled before starting a new one. Use the symbols loadLoanRepaySchedule, viewModelScope, repository.getLoanRepaySchedule, collect/collectLatest, and mutableStateFlow in your change.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (1)
100-132:⚠️ Potential issue | 🟠 MajorUse the latest screen state inside the export collector.
LaunchedEffect(Unit)keeps the initialstatein its closure. After the schedule loads, this handler can still seerepaymentScheduleTableData == nulland dropExportPdfas a no-op. Please read the latest state viarememberUpdatedState(or move the export payload into the event/ViewModel), and turn the null-data path intoPdfExportErrorinstead of silently returning. While you're here,printStackTrace()should be replaced with the app logger and the exception message should be forwarded to the dialog.Suggested fix
+import androidx.compose.runtime.rememberUpdatedState + `@Composable` internal fun LoanRepaymentScheduleScreen( viewModel: LoanRepaymentScheduleViewModel = koinViewModel(), navigateBack: () -> Unit, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() + val currentState by rememberUpdatedState(state) val coroutineScope = rememberCoroutineScope() val pdfGenerator = rememberPdfGenerator() @@ LoanRepaymentScheduleEvent.NavigateBack -> navigateBack() LoanRepaymentScheduleEvent.ExportPdf -> { - state.repaymentScheduleTableData?.let { data -> + val data = currentState.repaymentScheduleTableData + ?: run { + viewModel.trySendAction( + LoanRepaymentScheduleAction.PdfExportError( + title = getString(Res.string.feature_loan_export_pdf_error_title), + message = getString(Res.string.feature_loan_export_pdf_error), + ), + ) + return@collect + } coroutineScope.launch { try { @@ } catch (e: Exception) { - e.printStackTrace() viewModel.trySendAction( LoanRepaymentScheduleAction.PdfExportError( title = getString(Res.string.feature_loan_export_pdf_error_title), - message = getString(Res.string.feature_loan_export_pdf_error), + message = e.message ?: getString(Res.string.feature_loan_export_pdf_error), ), ) } } - } } } } }🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt` around lines 100 - 132, The ExportPdf collector inside LaunchedEffect captures the initial state and may see repaymentScheduleTableData as null; update the code to read the latest state via rememberUpdatedState (e.g., val latestState by rememberUpdatedState(state)) or move the payload into the ExportPdf event so the handler always has current data; change the null branch to send LoanRepaymentScheduleAction.PdfExportError via viewModel.trySendAction instead of silently returning; replace e.printStackTrace() with the app logger and include e.message (or the exception) in the PdfExportError dialog payload; keep the existing flow body that calls createPdfStrings, RepaymentScheduleHtmlGenerator(...).generateHtml(), and pdfGenerator.generateAndSharePdf(...) when data is present.
🧹 Nitpick comments (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt (1)
68-69: Consider adding validation whencellsandwidthssizes differ.The
getOrElsefallback on line 69 silently handles mismatched list sizes, which is defensive but could mask programming errors at call sites. Since the relevant call sites (RepaymentTableHeader, RepaymentPeriodRow, RepaymentTableFooter) all create exactly 14 cells and expect 14 widths, a mismatch would indicate a bug.Consider adding a
requirecheck or debug assertion at the start of the function:require(cells.size == widths.size) { "cells (${cells.size}) and widths (${widths.size}) must have the same size" }Alternatively, keep the fallback but log a warning in debug builds to aid troubleshooting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt` around lines 68 - 69, Add a size validation for the cells vs widths lists in the MifosTable rendering logic: in the function containing the cells.forEachIndexed loop (the MifosTable component), assert that cells.size == widths.size using a require or debug-only assertion and provide a clear message like "cells (${cells.size}) and widths (${widths.size}) must have the same size"; alternatively, if you want to preserve the fallback behavior, emit a debug/log warning when sizes differ so mismatches originating from callers such as RepaymentTableHeader, RepaymentPeriodRow, and RepaymentTableFooter are visible during development.feature/loan/src/commonMain/composeResources/values/strings.xml (1)
332-334: Minor capitalization inconsistency.Line 333 uses "Current balance" (lowercase 'b'), while line 179 (
feature_loan_profile_label_balance) uses "Current Balance" (title case). Consider aligning the capitalization for consistency across the UI, unless the different casing is intentional for distinct contexts.✏️ Optional fix for consistency
<string name="feature_loan_client_name_label">Client Name</string> - <string name="feature_loan_current_balance_label">Current balance</string> + <string name="feature_loan_current_balance_label">Current Balance</string> <string name="feature_loan_arrears_label">Arrears</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/composeResources/values/strings.xml` around lines 332 - 334, The string resource feature_loan_current_balance_label currently uses "Current balance" (lowercase 'b') which is inconsistent with feature_loan_profile_label_balance ("Current Balance"); update feature_loan_current_balance_label to "Current Balance" to match title case across the UI (or, if the lowercase is intentional for a different context, verify and document that distinction), ensuring the change is made to the string value in the resource entry.
🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`:
- Around line 162-168: The export IconButton in LoanRepaymentScheduleScreen
should be disabled until repayment data exists; change the IconButton (the one
invoking onAction(LoanRepaymentScheduleAction.ExportToPdf)) to set its enabled
property based on repaymentScheduleTableData availability (e.g., enabled =
repaymentScheduleTableData != null or combined with state check such as state
!is Loading && state !is Error) so users cannot trigger ExportToPdf when
repaymentScheduleTableData is missing.
- Around line 216-245: The productNameLabel is populated with the screen title
in createPdfStrings(), causing the PDF to show “Loan Repayment Schedule” instead
of the loan product name; update productNameLabel in createPdfStrings() to use
the actual product-name source (e.g., replace
getString(Res.string.feature_loan_loan_repayment_schedule) with the product name
resource such as getString(Res.string.feature_loan_product_name) if it exists,
or pull the product name from the screen state/viewModel like
viewModel.loanProductName or a passed-in parameter) so productNameLabel contains
the real product name rather than the screen title.
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`:
- Around line 100-132: The ExportPdf collector inside LaunchedEffect captures
the initial state and may see repaymentScheduleTableData as null; update the
code to read the latest state via rememberUpdatedState (e.g., val latestState by
rememberUpdatedState(state)) or move the payload into the ExportPdf event so the
handler always has current data; change the null branch to send
LoanRepaymentScheduleAction.PdfExportError via viewModel.trySendAction instead
of silently returning; replace e.printStackTrace() with the app logger and
include e.message (or the exception) in the PdfExportError dialog payload; keep
the existing flow body that calls createPdfStrings,
RepaymentScheduleHtmlGenerator(...).generateHtml(), and
pdfGenerator.generateAndSharePdf(...) when data is present.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.kt`:
- Around line 183-211: In mapToTableData (LoanRepaymentScheduleViewModel), guard
the nullable loan.currency.code before any formatting by normalizing it (e.g.,
val currencyCode = loan.currency.code ?: "") and use that non-null/default value
when calling CurrencyFormatter.format, mapPeriodsData, calculateTotals and when
storing currencyCode in the returned
LoanRepaymentScheduleState.RepaymentScheduleTableData so downstream functions
receive a non-null string; update references to currencyCode in mapToTableData
accordingly and ensure any signatures/uses of mapPeriodsData and calculateTotals
expect a non-null String if needed.
- Around line 139-180: loadLoanRepaySchedule currently starts a new collector on
repository.getLoanRepaySchedule every time (via viewModelScope.launch { ...
collect { ... } }), which allows multiple concurrent collectors to race updating
mutableStateFlow; fix by ensuring only one active collector: either replace
collect with collectLatest on repository.getLoanRepaySchedule(...) inside
loadLoanRepaySchedule so new emissions/calls cancel the previous handling, or
implement a Job property (e.g., private var repayScheduleJob: Job?) and before
launching call repayScheduleJob?.cancel() then assign repayScheduleJob =
viewModelScope.launch { repository.getLoanRepaySchedule(state.loanId).collect {
... } } so previous coroutine is cancelled before starting a new one. Use the
symbols loadLoanRepaySchedule, viewModelScope, repository.getLoanRepaySchedule,
collect/collectLatest, and mutableStateFlow in your change.
---
Nitpick comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.kt`:
- Around line 68-69: Add a size validation for the cells vs widths lists in the
MifosTable rendering logic: in the function containing the cells.forEachIndexed
loop (the MifosTable component), assert that cells.size == widths.size using a
require or debug-only assertion and provide a clear message like "cells
(${cells.size}) and widths (${widths.size}) must have the same size";
alternatively, if you want to preserve the fallback behavior, emit a debug/log
warning when sizes differ so mismatches originating from callers such as
RepaymentTableHeader, RepaymentPeriodRow, and RepaymentTableFooter are visible
during development.
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Around line 332-334: The string resource feature_loan_current_balance_label
currently uses "Current balance" (lowercase 'b') which is inconsistent with
feature_loan_profile_label_balance ("Current Balance"); update
feature_loan_current_balance_label to "Current Balance" to match title case
across the UI (or, if the lowercase is intentional for a different context,
verify and document that distinction), ensuring the change is made to the string
value in the resource entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5442b398-f415-422e-b6fa-4011eba7e28b
📒 Files selected for processing (5)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTable.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
Comment on lines
+162
to
168
| actions = { | ||
| IconButton(onClick = { onAction(LoanRepaymentScheduleAction.ExportToPdf) }) { | ||
| Icon( | ||
| imageVector = MifosIcons.Export, | ||
| contentDescription = stringResource(Res.string.feature_loan_export_to_pdf), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Disable export until repayment data is ready.
The action is enabled even in Loading / Error states, so users can trigger export before repaymentScheduleTableData exists. Wiring enabled to data availability will avoid dead taps and duplicate error dialogs.
🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`
around lines 162 - 168, The export IconButton in LoanRepaymentScheduleScreen
should be disabled until repayment data exists; change the IconButton (the one
invoking onAction(LoanRepaymentScheduleAction.ExportToPdf)) to set its enabled
property based on repaymentScheduleTableData availability (e.g., enabled =
repaymentScheduleTableData != null or combined with state check such as state
!is Loading && state !is Error) so users cannot trigger ExportToPdf when
repaymentScheduleTableData is missing.
Comment on lines
+216
to
+245
| private suspend fun createPdfStrings(): RepaymentSchedulePdfStrings { | ||
| return RepaymentSchedulePdfStrings( | ||
| title = getString(Res.string.feature_loan_repayment_schedule_pdf_title), | ||
| clientNameLabel = getString(Res.string.feature_loan_client_name_label), | ||
| accountNumberLabel = getString(Res.string.feature_loan_account_number), | ||
| productNameLabel = getString(Res.string.feature_loan_loan_repayment_schedule), | ||
| disbursementDateLabel = getString(Res.string.feature_loan_disbursed_date), | ||
| installmentsLabel = getString(Res.string.total_installments), | ||
| paidLabel = getString(Res.string.feature_loan_paid_label), | ||
| totalLabel = getString(Res.string.feature_loan_total_label), | ||
| principalPaidLabel = getString(Res.string.principal_paid_off), | ||
| periodDetailsHeader = getString(Res.string.feature_loan_period_details), | ||
| loanAmountBalanceHeader = getString(Res.string.feature_loan_amount_and_balance), | ||
| totalCostLoanHeader = getString(Res.string.feature_loan_total_cost_of_loan), | ||
| installmentTotalsHeader = getString(Res.string.feature_loan_installment_totals), | ||
| hNo = getString(Res.string.feature_loan_table_header_number), | ||
| hDays = getString(Res.string.feature_loan_table_header_days), | ||
| hDate = getString(Res.string.feature_loan_table_header_date), | ||
| hPaidDate = getString(Res.string.feature_loan_table_header_paid_date), | ||
| hBalance = getString(Res.string.feature_loan_table_header_loan_balance), | ||
| hPrincipal = getString(Res.string.feature_loan_table_header_principal_due), | ||
| hInterest = getString(Res.string.feature_loan_table_header_interest), | ||
| hFees = getString(Res.string.feature_loan_table_header_fees), | ||
| hPenalties = getString(Res.string.feature_loan_table_header_penalties), | ||
| hDue = getString(Res.string.feature_loan_table_header_due), | ||
| hPaid = getString(Res.string.feature_loan_table_header_paid), | ||
| hInAdvance = getString(Res.string.feature_loan_table_header_in_advance), | ||
| hLate = getString(Res.string.feature_loan_table_header_late), | ||
| hOutstanding = getString(Res.string.feature_loan_table_header_outstanding), | ||
| ) |
There was a problem hiding this comment.
Populate productNameLabel with a product-name string, not the screen title.
productNameLabel currently uses feature_loan_loan_repayment_schedule, so the generated PDF will label that field as “Loan Repayment Schedule” instead of “Product Name”.
🤖 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/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt`
around lines 216 - 245, The productNameLabel is populated with the screen title
in createPdfStrings(), causing the PDF to show “Loan Repayment Schedule” instead
of the loan product name; update productNameLabel in createPdfStrings() to use
the actual product-name source (e.g., replace
getString(Res.string.feature_loan_loan_repayment_schedule) with the product name
resource such as getString(Res.string.feature_loan_product_name) if it exists,
or pull the product name from the screen state/viewModel like
viewModel.loanProductName or a passed-in parameter) so productNameLabel contains
the real product name rather than the screen title.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#649
Android
Screen_recording_20260310_003021.webm
Desktop
Screen.Recording.2026-03-10.at.12.33.33.AM.mov
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Chores