chore: add missing files from kmp-project-template#1951
chore: add missing files from kmp-project-template#1951biplab1 wants to merge 6 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive keystore management infrastructure, multi-platform HTTP client configuration, a navigation system for Compose-based mobile apps, shared UI components for bottom navigation and scaffolding, and authentication state modeling. It includes new Gradle convention plugins, reorganizes network monitoring code into a repository package, and adds extensive test suites for keystore operations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App as SharedApp
participant RootNav as RootNavScreen
participant UserRepo as UserDataRepository
participant RootVM as RootNavViewModel
participant AuthNav as Auth/Onboarding/etc
User->>App: Launch App
App->>App: initKoin()
App->>RootNav: Create RootNavScreen
RootNav->>RootVM: Initialize ViewModel
RootVM->>UserRepo: Observe userData
UserRepo-->>RootVM: Emit user state
RootVM->>RootVM: Map to RootNavState
alt First Time User
RootVM->>RootNav: ShowOnboarding
RootNav->>AuthNav: Navigate to Onboarding
else Not Authenticated
RootVM->>RootNav: Auth
RootNav->>AuthNav: Navigate to Auth
else Passcode Empty
RootVM->>RootNav: UserLocked
RootNav->>AuthNav: Navigate to Passcode
else Unlocked
RootVM->>RootNav: UserUnlocked(userId)
RootNav->>AuthNav: Navigate to AuthenticatedGraph
end
AuthNav-->>User: Render Screen
sequenceDiagram
participant Task as KeystoreGenerationTask
participant Validator as ConfigurationValidator
participant Keytool as keytool (CLI)
participant Config as KeystoreConfig
participant Secrets as SecretsEnvUpdateTask
Task->>Validator: validateConfiguration()
Validator-->>Task: isValid
Task->>Config: Build keytool command
Task->>Keytool: Execute keytool
Keytool-->>Task: Keystore files created
Task->>Secrets: Pass keystorePath
Secrets->>Secrets: Encode keystores to Base64
Secrets->>Secrets: Parse existing secrets.env
Secrets->>Secrets: Merge with Base64 data
Secrets->>Secrets: Write updated secrets.env
Secrets-->>Task: secrets.env updated
Task-->>Task: Print summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
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 |
biplab1
changed the title
biplab1
changed the title
biplab1
force-pushed
the
MW-328-add-missing-template-modules-files
branch
from
010c032 to
4bc40f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI Agents
In @build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt:
- Around line 36-40: The Windows branch is using an incorrect attrib command to
set executable bits; instead, make the permission-setting step only run on Unix
by removing or replacing the else branch that calls commandLine("cmd", "/c",
"attrib", ...). Update the isLinuxOrMacOs() conditional so that the Exec task
invokes commandLine("chmod", "-R", "+x", ".git/hooks/") only when true and
otherwise either do nothing or perform a harmless verification (e.g., check/copy
files exist), removing the incorrect attrib call; adjust the Exec task
configuration where commandLine(...) is used to reflect this change.
- Around line 49-55: The current project.afterEvaluate block makes many build
tasks (including clean) always depend on installGitHooks; change this to only
wire installGitHooks to preBuild (remove "clean" and the broad list) and/or make
the installGitHooks task itself incremental by adding an up-to-date check
(compare existing hooks vs desired content or a marker file) so it no-ops when
nothing changed; update the matching/configureEach invocation to reference only
"preBuild" (or make the dependency opt-in via a project property) and implement
the up-to-date logic inside the installGitHooks task implementation to avoid
re-installing hooks on every build or in CI.
- Line 38: In GitHooksConventionPlugin (locate the occurrence of the stray
"}else {" token), fix the spacing around the else keyword by replacing "}else {"
with "} else {" so the else has a leading space after the closing brace and
proper spacing before the opening brace.
In
@build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt:
- Around line 40-43: The code eagerly realizes the generation task by calling
generateKeystoresTask.get() inside the registration of updateSecretsEnv; change
this to wire the dependency lazily by passing the TaskProvider (or a mapped
Provider) instead of calling .get() — e.g., remove .get() and use
generateKeystoresTask or generateKeystoresTask.flatMap { ... } (and likewise
avoid forcing keystoreExtension.secretsConfig.get() if present) when calling
SecretsEnvUpdateTask.createFromKeystoreGeneration so the updateSecretsEnv task
remains registered without realizing the generateKeystoresTask during
configuration.
- Around line 24-31: The task registration eagerly calls
keystoreExtension.secretsConfig.get() via
KeystoreGenerationTask.createWithSecretsConfig(this,
keystoreExtension.secretsConfig.get()), which forces provider evaluation during
configuration; instead remove that .get() call and either (a) wire the task
properties lazily by passing the Provider into the task (use
keystoreConfig.set(keystoreExtension.keystoreConfig) and
secretsConfig.set(keystoreExtension.secretsConfig) as you already do) and move
the createWithSecretsConfig logic into KeystoreGenerationTask's @TaskAction so
it reads secretsConfig.get() at execution time, or (b) defer execution with
generateKeystoresTask.configure { it.doFirst {
KeystoreGenerationTask.createWithSecretsConfig(it,
keystoreExtension.secretsConfig.get()) } } so the provider is resolved at
runtime; ensure all references use the task property names (keystoreConfig,
secretsConfig) and remove any direct .get() in the configuration block.
In @build-logic/convention/src/main/kotlin/org/convention/AndroidCompose.kt:
- Around line 46-61: The helper relativeToRootProject is using flatMap and
captures the provider value `it` but never uses it, so replace flatMap with map
(or otherwise stop capturing `it`) and compute the directory directly: change
Provider<*>.relativeToRootProject(dir: String) to use map {
rootProject.layout.buildDirectory.dir(projectDir.toRelativeString(rootDir)).dir(dir)
} (or document that the input provider is intentionally discarded), and remove
the unused `it` parameter to avoid the misleading capture.
In @build-logic/convention/src/main/kotlin/org/convention/Badging.kt:
- Around line 116-118: Replace the artifact reference used when wiring APK
outputs: change the call that sets apk via
variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE) to use SingleArtifact.APK
instead so apk.set(...) points to regular APK outputs; update the reference in
the apk.set(...) line where variant.artifacts.get(...) is used to use
SingleArtifact.APK in place of SingleArtifact.APK_FROM_BUNDLE.
- Around line 30-36: Finish the incomplete KDoc sentence in Badging.kt to
explain the incremental behavior of the task: update the trailing sentence so it
reads something like "This task is also incremental, meaning that if only some
inputs have changed, the task will run only for those changes rather than
reprocessing everything," ensuring the documentation for the Badging task fully
describes incremental execution semantics.
In @build-logic/convention/src/main/kotlin/org/convention/Detekt.kt:
- Around line 17-19: The hard-coded project path in
tasks.named<Detekt>("detekt") calling
mustRunAfter(":cmp-android:dependencyGuard") makes the convention plugin
non-reusable; replace the direct path with a safe conditional lookup (e.g., use
tasks.findByPath for ":cmp-android:dependencyGuard" and only call mustRunAfter
when that task exists) or make the dependency task name configurable via a
plugin extension and use that value instead of the literal; update the block
around tasks.named<Detekt>("detekt") to perform the conditional check (or read
the extension property) before invoking mustRunAfter so the plugin no longer
assumes a specific project structure.
In @build-logic/convention/src/main/kotlin/org/convention/Jacoco.kt:
- Around line 61-66: The Project.configureJacoco code calls
libs.findVersion("jacoco").get() which will throw if the version key is missing;
add a "jacoco" entry to the version catalog (gradle/libs.versions.toml) under
the [versions] section (e.g. jacoco = "0.8.8" or your desired version) so
libs.findVersion("jacoco") returns a value for configure<JacocoPluginExtension>
in configureJacoco.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/BaseKeystoreTask.kt:
- Around line 81-96: checkKeytoolAvailable currently starts a
ProcessBuilder("keytool", "-help") then calls waitFor() without consuming
stdout/stderr or applying a timeout, which can hang; fix by consuming or
redirecting the process streams and using a timed wait: in
checkKeytoolAvailable, after starting the process (ProcessBuilder.start()),
either call process.inputStream and process.errorStream readers on background
threads or use processBuilder.redirectOutput(ProcessBuilder.Redirect.DISCARD)
and redirectError(ProcessBuilder.Redirect.DISCARD) before start(), then call
process.waitFor(timeoutMillis, TimeUnit.MILLISECONDS) (or waitFor with a
reasonable timeout) and if it returns false destroy the process and log an
error; also catch InterruptedException and ensure streams are closed and
process.destroyForcibly() is called when needed, and include the exit code in
the log when available.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt:
- Around line 156-184: Add the fastlane-config directory to .gitignore to
prevent committing plaintext credentials: update the repo .gitignore to include
"fastlane-config/" (or "fastlane-config/**") so files written by
createNewFastlaneConfig (which writes the Android Fastlane config via
configFile.writeText) are ignored; commit the .gitignore change and verify that
fastlane-config/android_config.rb is no longer tracked (remove from git index if
already committed using git rm --cached) before merging.
- Around line 167-172: The FIREBASE_CONFIG map in ConfigurationFileUpdatesTask
(symbol: FIREBASE_CONFIG) contains hardcoded Firebase app IDs that look real;
change those values to clearly fake placeholders (e.g.,
"YOUR_FIREBASE_APP_ID_HERE" and "YOUR_FIREBASE_DEMO_APP_ID_HERE") or wire them
to configurable sources (project properties / environment variables) so the task
reads keys instead of embedding real IDs; update the map entries
firebase_prod_app_id and firebase_demo_app_id accordingly and add a brief
comment explaining they must be provided via configuration when running the
task.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt:
- Around line 214-218: validateFilePaths currently has a side effect: it calls
config.keystoreDir.mkdirs(), which creates directories; change it to a pure
read-only validation by removing the mkdirs() call and instead check existence
and writability only (e.g., use config.keystoreDir.exists() and
config.keystoreDir.isDirectory(), or
java.nio.file.Files.exists/Files.isDirectory and Files.isWritable on the path or
its nearest existing parent) and add an error if the directory doesn't exist or
cannot be written to, keeping the checks confined to the validateFilePaths
function and referencing config.keystoreDir and validateFilePaths when updating
the logic.
- Around line 42-48: The guard on config.validity contains an unreachable
branch: after checking config.validity <= 0 the subsequent else if
(config.validity < 1) can never be true for integers; update
ConfigurationValidator to replace that unreachable condition with a meaningful
short-validity threshold (for example use config.validity <= 2 to warn about 1–2
year validity), keeping the existing errors.add and warnings.add flows
(references: config.validity, errors, warnings in ConfigurationValidator).
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt:
- Around line 296-303: The constants in EnvironmentOverrideHandler
(ORIGINAL_KEYSTORE_PASSWORD, ORIGINAL_KEYSTORE_ALIAS,
ORIGINAL_KEYSTORE_ALIAS_PASSWORD, UPLOAD_KEYSTORE_PASSWORD,
UPLOAD_KEYSTORE_ALIAS, UPLOAD_KEYSTORE_ALIAS_PASSWORD) have mismatched
identifier names vs. string values (some values include "_FILE_"). Fix by making
the identifiers and their string values consistent: either rename the constants
to include "FILE" (e.g., ORIGINAL_KEYSTORE_FILE_PASSWORD,
UPLOAD_KEYSTORE_FILE_PASSWORD) or change the string values to remove "_FILE_" so
they match the current constant names; update all referenced usages to the new
names to keep callers consistent.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt:
- Around line 10-12: KeystoreConfig currently uses weak hardcoded defaults for
keystorePassword and keyPassword ("android"); remove those defaults so callers
must supply secure values (i.e., change the parameters keystorePassword and
keyPassword in KeystoreConfig to have no default), and optionally keyAlias if
you want to avoid any implicit value; additionally add a runtime check in
KeystoreConfig initialization that detects if either password equals the legacy
debug value ("android") and either logs a prominent warning or throws an error
to prevent accidental use in production (use the class/constructor
KeystoreConfig and the properties keystorePassword, keyPassword, keyAlias to
locate where to change and add the check).
- Line 51: KeystoreConfig currently flags keySize < 1024 as an error but
ConfigurationValidator.validateKeyAlgorithm() requires RSA keys to be >= 2048;
update KeystoreConfig's validation (the keySize check in KeystoreConfig) to
enforce the same minimum (use 2048) or make the check algorithm-aware (e.g., if
algorithm is "RSA" require keySize >= 2048) so both validations are consistent
with ConfigurationValidator.validateKeyAlgorithm().
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt:
- Around line 196-199: The log currently prints the full command (including
sensitive `-storepass`/`-keypass` values) in executeKeytoolCommand; change the
logging to mask sensitive arguments by constructing a maskedCommand from the
command List<String> before logInfo: treat both "--flag=value" and separate
flag/value forms, replace the password value with a fixed mask (e.g. "****")
whenever an element startsWith("-storepass") or startsWith("-keypass") or
immediately follows those flags, and then call logInfo with the maskedCommand
instead of the raw command to avoid exposing keystore passwords.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt:
- Around line 96-98: The else branch that adds "Invalid multiline syntax" is
unreachable because when line.contains("<<") is true, split("<<", limit = 2)
always produces parts.size == 2; remove the dead else branch inside the
multiline handling in SecretsEnvParser (the block that checks
line.contains("<<") and assigns parts) and instead validate the second part
(parts[1]) for being empty or only whitespace — if parts[1].isBlank() then call
errors.add with a clear message; update any related tests to expect this
validation instead of relying on the unreachable branch.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvUpdateTask.kt:
- Around line 396-406: The header-write logic currently checks
secretsFile.exists() after calling secretsFile.printWriter() which
creates/truncates the file, so the header condition never triggers; move the
existence check before opening the writer (i.e., evaluate !secretsFile.exists()
|| secrets.comments.isEmpty() using secretsFile.exists() prior to calling
secretsFile.printWriter().use { writer -> ... }) and only then open the writer
to write the header and body; keep the rest of the write logic (the
printWriter().use block and the header lines) unchanged.
In @build-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt:
- Around line 28-34: Update the misleading comment above the compileOptions
block: it currently references "Up to Java 11 APIs" but the code sets
sourceCompatibility and targetCompatibility to JavaVersion.VERSION_17 and
enables isCoreLibraryDesugaringEnabled; change the comment to state that Java 17
language level and desugaring are configured (and clarify what desugaring covers
for Java 17, if needed) so it accurately reflects the settings in
compileOptions.
- Around line 48-53: Update the misleading comment above the
extensions.configure<JavaPluginExtension> block: remove the Android/desugaring
reference and state that sourceCompatibility and targetCompatibility are set to
Java 17 (JavaVersion.VERSION_17) for non-Android JVM projects; ensure the
comment succinctly reflects Java 17 configuration and its applicability (e.g.,
"Configure Java 17 (sourceCompatibility and targetCompatibility) for JVM
projects — not related to Android desugaring").
In @build-logic/convention/src/main/kotlin/org/convention/PrintTestApks.kt:
- Around line 88-91: Update the RuntimeException messages thrown after loading
APKs to include contextual details: when
builtArtifactsLoader.get().load(apkFolder.get()) returns null, throw an
exception that includes the apkFolder path (e.g.,
apkFolder.get().asFile.toString()) and the variant name if available (e.g.,
variantName or variant.toString()); similarly, when checking
builtArtifacts.elements.size != 1, include the actual size, the apkFolder path
and the variant identifier in the error message so the exceptions provide
actionable debugging info; modify the two throw RuntimeException(...) calls
accordingly near builtArtifactsLoader.get().load(...) and the size check.
In @build-logic/convention/src/main/kotlin/org/convention/Spotless.kt:
- Line 6: Update the ktlint version constant to remove the vulnerable old
release: change the const val ktlintVersion (in Spotless.kt) from "1.0.1" to
"1.8.0", then refresh dependencies/lockfiles and run the build/spotless checks
to ensure compatibility and resolve any formatter rule changes from the upgrade.
In @cmp-android/prodRelease-badging.txt:
- Line 1: The manifest badging shows versionCode='1' while
versionName='2026.1.1-beta.0.6'; update the integer versionCode to an
incremented value that reflects this new release (ensure versionCode is a
monotonically increasing integer used by the Play Store), e.g. bump versionCode
from 1 to an appropriate higher integer that corresponds to this release, and
make the same change in the canonical source of truth for app versions (e.g.,
the module's build.gradle / android.defaultConfig versionCode or wherever
versionCode is injected) so versionCode and versionName remain consistent across
builds.
In @cmp-desktop/src/jvmMain/kotlin/main.kt:
- Around line 31-34: Move the initKoin() call out of the application composable
scope so Koin is started exactly once at app startup: call initKoin() at the
start of main() before entering the application { ... } block (not inside any
composable or UI lambda), keeping the application { ... } block purely for UI;
update main() to invoke initKoin() first and then call application { /* UI */ }
to avoid reinitialization.
- Line 43: The window title is a placeholder ("DesktopApp") in main.kt; update
the title assignment (title = "DesktopApp") to a descriptive app name (e.g.,
"MifosPay" or "Mifos Mobile Wallet") so the UI shows a meaningful name; locate
the title property in the main window creation code (the title = ... line) and
replace the string accordingly.
In
@cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme:
- Around line 1-32: The scheme file for the iosApp (LiteDo.app, BlueprintName
"iosApp") is located under xcuserdata which is user-specific; either make the
scheme shared so it moves to xcshareddata by opening Xcode, selecting the iosApp
scheme, choosing "Manage Schemes..." and checking "Shared" (this will relocate
the scheme out of xcuserdata), or remove this xcuserdata/iosApp.xcscheme from
version control and add the appropriate xcuserdata/ path to .gitignore if the
scheme is truly user-specific.
- Line 11: Update the Xcode scheme's BuildableName entries from "LiteDo.app" to
"MifosPay.app": find the two occurrences of BuildableName = "LiteDo.app" in the
iosApp.xcscheme and replace each with BuildableName = "MifosPay.app" so the
scheme matches the product name configured in Config.xcconfig and the
org.mifospay bundle identifier.
In
@cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist:
- Around line 1-14: Update .gitignore to explicitly ignore developer xcuserdata
by adding patterns for cmp-ios/iosApp.xcodeproj/xcuserdata/ and
cmp-ios/iosApp.xcworkspace/xcuserdata/ so these per-developer Xcode files aren’t
tracked; then remove the currently tracked xcuserdata files (those under
cmp-ios/.../xcuserdata, e.g., entries for skniyajali, kapilmaurya,
hekmatullahamin) from git’s index and commit that change to stop them from
appearing in future commits; optionally, if historical removal is required,
purge those paths from the repository history using your repo’s history-rewrite
tool.
In
@cmp-ios/iosApp.xcworkspace/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist:
- Around line 1-5: The committed user-specific Xcode workspace file
xcschememanagement.plist located under xcuserdata/skniyajali.xcuserdatad should
be removed and excluded: remove the file from the repo index (e.g., git rm
--cached) and commit the deletion, then update .gitignore to ignore xcuserdata
directories for both .xcodeproj and .xcworkspace (add patterns like
*.xcodeproj/xcuserdata/ and *.xcworkspace/xcuserdata/ or equivalent), verify no
other xcuserdata files remain tracked, and commit the .gitignore change.
In
@core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt:
- Line 80: In ResultSuspendConverterFactory (locate the println calls in the
suspend conversion logic inside the class/method handling Result wrapping),
replace all println("Failure: " + result.throwable.message) uses with a proper
logger call from your chosen logging abstraction (e.g., kermit or napier);
obtain or inject a logger instance (e.g., a private val logger or constructor
parameter) and call logger.error/diagnostic with a descriptive message and
include the throwable so the stacktrace is captured (for example:
logger.error("Failure converting Result", throwable) or equivalent in your
logging API), and apply the same replacement for the other occurrences
referenced in the file.
- Line 88: In ResultSuspendConverterFactory's status handling (the when on
status), the HTTP success range currently uses 200..209 which incorrectly treats
2xx codes >=210 as errors; change the branch to use 200..299 so all RFC 7231
success codes (e.g., 201, 204, 206) are handled by the success path (i.e.,
update the condition from in 200..209 to in 200..299 and keep the existing body
that reads result.response.body(successType) and returns NetworkResult.Success
or maps serialization exceptions to NetworkError.SERIALIZATION).
- Around line 57-58: Update the KDoc on ResultSuspendConverterFactory so it
references NetworkResult (the actual type checked by the implementation) instead
of Result; change the sentence that currently says "`Result`" to
"`NetworkResult`" (and adjust surrounding wording/backticks as needed) so the
doc matches the converter's behavior and types used by suspendResponseConverter.
- Around line 40-46: The example in the KDoc uses the wrong generic types;
update the sample ApiService declaration so the suspend function getUsers()
returns NetworkResult<List<User>, NetworkError> instead of Result<List<User>,
RemoteError>, i.e., replace the type names in the documentation example to match
the codebase's NetworkResult and NetworkError types.
In
@core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt:
- Around line 118-126: Digest auth configuration is missing proactive credential
sending; update the digest auth block (where install(Auth) { digest {
credentials { digestCredentialsProvider() } } } ) to include a
sendWithoutRequest { request -> request.url.host in authRequiredUrl } clause
similar to the Bearer/Basic providers so digest credentials are sent proactively
to hosts in authRequiredUrl.
In
@core-base/network/src/jsMain/kotlin/template/core/base/network/KtorHttpClient.js.kt:
- Line 10: The package declaration in KtorHttpClient.js.kt uses the placeholder
package template.core.base.network; rename it to org.mifospay.core.base.network
to match the project's package structure, and apply the same change across all
files in the core-base module (ui, platform, network) so package declarations
consistently use org.mifospay.core.base.*; ensure any imports, references, and
module metadata that rely on the old package are updated accordingly (e.g.,
KtorHttpClient and other classes in core-base).
🟠 Major comments (21)
cmp-android/prodRelease-badging.txt-1-1 (1)
1-1: Address the versionCode/versionName mismatch.The versionCode is set to '1' while versionName is '2026.1.1-beta.0.6'. This inconsistency suggests version tracking may not be properly maintained. Typically, versionCode increments with each release. If this is a production build, versionCode should be incremented accordingly (e.g., to match release numbers).
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsEvent.kt-258-279 (1)
258-279: Documentation contradicts implementation: validation vs. sanitization.The implementation silently sanitizes invalid inputs instead of throwing exceptions as documented:
- Blank keys → replaced with
"unknown_param"(line 272)- Keys > 40 chars → truncated (line 271)
- Values > 100 chars → truncated (line 274)
However, the KDoc at lines 238, 249-253 explicitly states:
"@throws IllegalArgumentException if validation constraints are violated"
Impact:
- Developers expecting exceptions won't receive them, potentially masking bugs
- Blank keys silently becoming
"unknown_param"obscures data quality issues- Silent truncation may cause unnoticed data loss in analytics
Recommendation: Choose one approach consistently:
- Match documentation (throw exceptions for invalid input) — better for catching bugs early, OR
- Match implementation (update docs to reflect sanitization) — better for resilience but risks hiding issues
Option 1: Throw exceptions to match documentation
operator fun invoke(key: String, value: String): Param { - val safeKey = key - .takeIf { it.isNotBlank() } - ?.take(MAX_KEY_LENGTH) - ?: FALLBACK_KEY - - val safeValue = value.take(MAX_VALUE_LENGTH) - - return Param(safeKey, safeValue) + require(key.isNotBlank()) { "Parameter key must not be blank" } + require(key.length <= MAX_KEY_LENGTH) { + "Parameter key exceeds maximum length of $MAX_KEY_LENGTH characters" + } + require(value.length <= MAX_VALUE_LENGTH) { + "Parameter value exceeds maximum length of $MAX_VALUE_LENGTH characters" + } + + return Param(key, value) }Option 2: Update documentation to reflect sanitization behavior
Update the KDoc to remove
@throws IllegalArgumentExceptionreferences (lines 238, 249-253) and add:/** * Represents a validated analytics parameter with automatic constraint sanitization. * * This data class encapsulates a key-value pair for analytics events with built-in * sanitization that enforces analytics platform constraints. The sanitization occurs * during object construction to ensure data integrity. * * **Sanitization Rules:** * - Blank keys are replaced with "unknown_param" * - Keys are truncated to ≤ 40 characters (Firebase Analytics constraint) * - Values are truncated to ≤ 100 characters (Firebase Analytics constraint) * ... */Also update withParam/withParams documentation at lines 70, 97, 128 to remove exception throwing claims.
cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme-1-32 (1)
1-32: Move scheme to shared location or remove from version control.This scheme file is in the
xcuserdata/directory, making it user-specific. If this scheme needs to be shared across the team:
- In Xcode, select the scheme from the scheme dropdown
- Choose "Manage Schemes..."
- Check the "Shared" checkbox for the iosApp scheme
This will move the scheme file to
xcshareddata/instead ofxcuserdata/, making it appropriate for version control. If the scheme is truly user-specific and doesn't need to be shared, it should be excluded from the repository via.gitignore.cmp-ios/iosApp.xcworkspace/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist-1-5 (1)
1-5: Remove user-specific Xcode workspace files from version control.This file is also located in a
xcuserdata/skniyajali.xcuserdatad/directory, making it user-specific configuration that should not be committed. Even though it contains only an empty dictionary, it will still cause unnecessary noise and potential conflicts in the repository.Ensure that
xcuserdata/patterns are properly excluded in.gitignorefor both.xcodeprojand.xcworkspacedirectories.cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme-11-11 (1)
11-11: Update BuildableName from "LiteDo.app" to "MifosPay.app" in the Xcode scheme.The
BuildableNameis set to "LiteDo.app" (lines 11 and 26), but the project's actual app name is configured as "MifosPay" incmp-ios/Configuration/Config.xcconfigand the bundle identifier isorg.mifospay. The scheme file was copied from the kmp-project-template without proper customization. Update both occurrences to"MifosPay.app"to match the configured product name.cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist-1-14 (1)
1-14: Remove user-specific Xcode files from version control.Multiple xcuserdata files are currently tracked in git (from developers skniyajali, kapilmaurya, and hekmatullahamin), which should be excluded. These files:
- Are specific to each developer's machine
- Cause merge conflicts when multiple developers commit their preferences
- Clutter the repository with personal Xcode configuration
The root cause is that
.gitignorecontains patterns foriosApp/(e.g.,iosApp/iosApp.xcodeproj/*), but the actual iOS project is located incmp-ios/. Update.gitignoreto excludecmp-ios/iosApp.xcodeproj/xcuserdata/andcmp-ios/iosApp.xcworkspace/xcuserdata/, then remove the tracked xcuserdata files from git history.core-base/network/src/jsMain/kotlin/template/core/base/network/KtorHttpClient.js.kt-10-10 (1)
10-10: Rename package to match project structure across all core-base modules.The package name
template.core.base.networkis a placeholder that doesn't align with the project's actual package structure. The codebase consistently usesorg.mifospay.*for packages (see feature modules), but the entirecore-basemodule (ui, platform, network) uses thetemplate.*prefix. Rename this toorg.mifospay.core.base.networkand apply the same fix to all other files in thecore-basemodule.core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-141-157 (1)
141-157: Logging configuration has two issues.
httpLoggerparameter is ignored: Line 142 assignslogger = httpLogger, but lines 152-156 immediately overwrite it with a Kermit-based logger. ThehttpLoggerparameter becomes dead code.Empty
loggableHostsdisables all logging: The filter on lines 144-148 usesloggableHosts.any { ... }. With the default empty list, this always returnsfalse, meaning no requests are logged by default.🔎 Proposed fix
Choose either the custom Kermit logger or the configurable
httpLogger, and fix the filter logic:install(Logging) { - logger = httpLogger level = httpLogLevel - filter { request -> - loggableHosts.any { host -> - request.url.host.contains(host) + if (loggableHosts.isNotEmpty()) { + filter { request -> + loggableHosts.any { host -> + request.url.host.contains(host) + } } } sanitizeHeader { header -> header in sensitiveHeaders } logger = object : Logger { override fun log(message: String) { KermitLogger.d(tag = "KtorClient", messageString = message) } } }Alternatively, if
httpLoggershould be used instead of the hardcoded Kermit logger, remove lines 152-156.core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-80-80 (1)
80-80: Replace println with proper logging framework.Using
printlnfor logging is not production-ready. It lacks log levels, structured output, and configurability. Consider using a proper logging framework (e.g.,kermit,napier, or platform-specific loggers).🔎 Example refactor using a logging abstraction
Assuming you have a logger instance available:
is KtorfitResult.Failure -> { - println("Failure: " + result.throwable.message) + logger.error("Network request failed: ${result.throwable.message}", result.throwable) NetworkResult.Error(NetworkError.UNKNOWN) }} catch (e: SerializationException) { - println("Serialization error: ${e.message}") + logger.error("Failed to deserialize response: ${e.message}", e) NetworkResult.Error(NetworkError.SERIALIZATION) }else -> { - println("Status code $status") + logger.warn("Unhandled HTTP status code: $status") NetworkResult.Error(NetworkError.UNKNOWN) }Also applies to: 95-95, 107-107
core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-88-88 (1)
88-88: Change the HTTP success range from200..209to200..299.The current range only covers status codes 200-209, leaving codes 210-299 (201 Created, 204 No Content, 206 Partial Content, etc.) to fall through to the
elsebranch where they are incorrectly treated as UNKNOWN errors. HTTP success codes span the full 2xx range (200-299) per RFC 7231.Code context
when (status) { in 200..209 -> { try { val data = result.response.body(successType) as Any NetworkResult.Success(data) } catch (e: NoTransformationFoundException) { NetworkResult.Error(NetworkError.SERIALIZATION) } catch (e: SerializationException) { println("Serialization error: ${e.message}") NetworkResult.Error(NetworkError.SERIALIZATION) } } // ... other branches ... else -> { println("Status code $status") NetworkResult.Error(NetworkError.UNKNOWN) } }build-logic/convention/src/main/kotlin/org/convention/PrintTestApks.kt-88-91 (1)
88-91: Enhance error messages for better debugging.The error messages are generic and don't provide helpful context when things go wrong. Including the variant name and potentially the APK folder path would significantly improve debuggability.
🔎 Proposed improvement with descriptive error messages
- val builtArtifacts = builtArtifactsLoader.get().load(apkFolder.get()) - ?: throw RuntimeException("Cannot load APKs") - if (builtArtifacts.elements.size != 1) - throw RuntimeException("Expected one APK !") + val builtArtifacts = builtArtifactsLoader.get().load(apkFolder.get()) + ?: throw RuntimeException("Cannot load APKs for variant '${variantName.get()}' from ${apkFolder.get()}") + if (builtArtifacts.elements.size != 1) { + throw RuntimeException( + "Expected exactly one APK for variant '${variantName.get()}', but found ${builtArtifacts.elements.size}" + ) + } val apk = File(builtArtifacts.elements.single().outputFile).toPath()cmp-desktop/src/jvmMain/kotlin/main.kt-31-34 (1)
31-34: Move Koin initialization outside the composable scope.Calling
initKoin()inside theapplicationblock is problematic. Whileapplicationdoesn't recompose frequently, Koin'sstartKoinshould be called exactly once at application startup, outside any composable scope. This prevents potential reinitialization issues and follows DI best practices.🔎 Proposed fix
fun main() { + // Initialize Koin before entering composable scope + initKoin() + application { - // Initializes the Koin dependency injection framework. - initKoin() - // Creates a window state to manage the window's state. val windowState = rememberWindowState()build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt-49-55 (1)
49-55: Reconsider aggressive task dependency on installGitHooks.Making all build-related tasks depend on
installGitHookswill force hook installation on every build operation, includingclean. This could:
- Slow down builds unnecessarily
- Cause issues in CI environments where
.gitmight not be available or writable- Re-install hooks even when they haven't changed
Consider:
- Running hook installation only once (check if hooks already exist and are up-to-date)
- Removing
cleanfrom the dependency list- Making this opt-in rather than automatic, or only depending on
preBuild🔎 Suggested improvements
Option 1: Only depend on preBuild:
project.afterEvaluate { project.tasks.matching { - it.name in listOf("preBuild", "build", "assembleDebug", "assembleRelease", "installDebug", "installRelease", "clean") + it.name == "preBuild" }.configureEach { dependsOn(project.tasks.named("installGitHooks")) } }Option 2: Add up-to-date checks to installGitHooks to skip when hooks are already installed.
build-logic/convention/src/main/kotlin/org/convention/Detekt.kt-17-19 (1)
17-19: Hard-coded project path breaks reusability.Line 18 hard-codes the project path
:cmp-android:dependencyGuard, which assumes a specific project structure. If this convention plugin is intended to be reusable across different projects or if the project structure changes, this will break.Consider one of these approaches:
- Make the dependency task name configurable via the plugin extension
- Use a conditional check:
tasks.findByPath(":cmp-android:dependencyGuard")?.let { mustRunAfter(it) }- Remove this constraint if the ordering isn't critical
🔎 Proposed fix using conditional
tasks.named<Detekt>("detekt") { - mustRunAfter(":cmp-android:dependencyGuard") + tasks.findByPath(":cmp-android:dependencyGuard")?.let { mustRunAfter(it) } jvmTarget = "17"build-logic/convention/src/main/kotlin/KtlintConventionPlugin.kt-7-18 (1)
7-18: Plugin ID is correct, but configuration implementation is missing compared to similar convention plugins.The plugin ID
"org.jlleitschuh.gradle.ktlint"correctly matches the version catalog declaration (version 12.1.1). However, unlikeMifosSpotlessConventionPluginandMifosDetektConventionPluginwhich both call configuration functions after applying their plugins,KtlintConventionPluginapplies the plugin without any additional configuration. NoconfigureKtlintfunction orKtlint.ktconfiguration file exists in the codebase, breaking the established pattern. Either add configuration support consistent with the other convention plugins, or document why ktlint intentionally uses defaults.build-logic/convention/src/main/kotlin/org/convention/GradleManagedDevices.kt-27-57 (1)
27-57: Add API levels 35 and 36 to device configurations.The current API levels 30–31 are still valid (API 30 is required for ATDs, and API 31 is supported). However, as of January 2026, the Google Play target API requirement is now 35+, and API 36 (Android 16) is the latest platform. Add test configurations for these levels to ensure compatibility with Play requirements and catch forward-compatibility issues.
Optionally, document why Pixel 6 (API 31) is excluded from CI if this is intentional for performance reasons.
build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt-10-12 (1)
10-12: Default passwords are weak and may pose a security risk if used inadvertently in production.The default values
"android"forkeystorePasswordandkeyPasswordare common debug defaults. Consider either:
- Requiring explicit password configuration (no defaults)
- Adding prominent documentation that these are debug-only defaults
- Logging a warning when defaults are used
build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt-40-43 (1)
40-43: Calling.get()on a task provider during configuration realizes the task eagerly.
generateKeystoresTask.get()on line 42 forces the task to be created immediately, defeating the purpose oftasks.register(). UseflatMapor wire dependencies lazily.🔎 Suggested approach
val updateSecretsEnvTask = tasks.register("updateSecretsEnv", SecretsEnvUpdateTask::class.java) { - SecretsEnvUpdateTask.createFromKeystoreGeneration(this, generateKeystoresTask.get(), keystoreExtension.secretsConfig.get()) + dependsOn(generateKeystoresTask) + // Wire outputs lazily or defer configuration to doFirst/TaskAction }Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt-24-31 (1)
24-31: Avoid calling.get()on providers during task configuration phase.Calling
keystoreExtension.secretsConfig.get()at configuration time (line 30) forces eager evaluation and breaks Gradle's lazy configuration model. This can cause ordering issues if the extension hasn't been configured yet by the build script.🔎 Suggested approach
Wire task properties lazily using provider APIs:
val generateKeystoresTask = tasks.register("generateKeystores", KeystoreGenerationTask::class.java) { keystoreConfig.set(keystoreExtension.keystoreConfig) secretsConfig.set(keystoreExtension.secretsConfig) - - // Load configuration from secrets.env if it exists - KeystoreGenerationTask.createWithSecretsConfig(this, keystoreExtension.secretsConfig.get()) }Move the
createWithSecretsConfiglogic into the task's@TaskActionmethod or usedoFirstto defer execution to runtime.Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt-214-218 (1)
214-218: Validation method has side effects: it creates directories.
validateFilePathscallsconfig.keystoreDir.mkdirs()on line 216, which modifies the filesystem. Validation should be a pure, read-only operation to avoid unexpected behavior when called multiple times or in dry-run scenarios.🔎 Suggested fix
Check only if the directory exists or is creatable without actually creating it:
private fun validateFilePaths(config: KeystoreConfig, errors: MutableList<String>, warnings: MutableList<String>) { // Check keystore directory - if (!config.keystoreDir.exists() && !config.keystoreDir.mkdirs()) { - errors.add("Cannot create keystore directory: ${config.keystoreDir.absolutePath}") + if (!config.keystoreDir.exists()) { + val parent = config.keystoreDir.parentFile + if (parent != null && !parent.canWrite()) { + errors.add("Cannot create keystore directory (no write permission): ${config.keystoreDir.absolutePath}") + } else { + // Directory will be created during task execution + } }build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvUpdateTask.kt-396-406 (1)
396-406: Logic issue: File existence check occurs after file creation.
printWriter().use { writer -> ... }creates/truncates the file before the existence check on line 399. The condition!secretsFile.exists()will always befalseat this point, so the header is never written for new files.🔎 Proposed fix - check before opening writer
private fun writeSecretsFile( secretsFile: File, secrets: ParsedSecretsData, ) { try { + val isNewFile = !secretsFile.exists() || secrets.comments.isEmpty() secretsFile.printWriter().use { writer -> // Write header comment if it's a new file or no comments exist - if (!secretsFile.exists() || secrets.comments.isEmpty()) { + if (isNewFile) { writer.println("# GitHub Secrets Environment File")
🟡 Minor comments (17)
core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-118-126 (1)
118-126: Digest auth missingsendWithoutRequestconfiguration.Bearer and Basic auth providers use
sendWithoutRequest { request -> request.url.host in authRequiredUrl }to proactively send credentials to specified hosts. Digest auth lacks this, causing inconsistent behavior across auth types.🔎 Proposed fix
digestCredentialsProvider != null -> { install(Auth) { digest { credentials { digestCredentialsProvider() } + sendWithoutRequest { request -> + request.url.host in authRequiredUrl + } } } }build-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt-28-34 (1)
28-34: Update comment to reflect Java 17 configuration.The comment mentions "Up to Java 11 APIs" but the code configures Java 17. Update the comment to accurately reflect the Java version and desugaring capabilities.
🔎 Suggested comment update
compileOptions { - // Up to Java 11 APIs are available through desugaring - // https://developer.android.com/studio/write/java11-minimal-support-table + // Java 17 target with core library desugaring for backward compatibility + // https://developer.android.com/studio/write/java11-minimal-support-table sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17 isCoreLibraryDesugaringEnabled = truebuild-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt-48-53 (1)
48-53: Update comment to reflect Java 17 configuration.The comment references Java 11 and Android desugaring, but this configures Java 17 for non-Android JVM projects where desugaring isn't applicable. Update the comment for clarity.
🔎 Suggested comment update
extensions.configure<JavaPluginExtension> { - // Up to Java 11 APIs are available through desugaring - // https://developer.android.com/studio/write/java11-minimal-support-table + // Configure Java 17 for JVM projects sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-57-58 (1)
57-58: Fix the documentation to reference the correct type.The documentation mentions "Result" but should reference "NetworkResult" for consistency with the actual implementation.
🔎 Proposed fix
* @return A [Converter.SuspendResponseConverter] if the return type is - * `Result`, or `null` otherwise. + * `NetworkResult`, or `null` otherwise.core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-40-46 (1)
40-46: Fix the example code to use correct type names.The example shows
Result<List<User>, RemoteError>but should useNetworkResult<List<User>, NetworkError>to match the actual types defined in this codebase.🔎 Proposed fix
* Example usage: * ```kotlin * interface ApiService { * @GET("users") - * suspend fun getUsers(): Result<List<User>, RemoteError> + * suspend fun getUsers(): NetworkResult<List<User>, NetworkError> * } * ```cmp-desktop/src/jvmMain/kotlin/main.kt-43-43 (1)
43-43: Use a more descriptive window title.The hardcoded title "DesktopApp" appears to be a placeholder from the template. Consider using a more descriptive name that reflects the actual application, such as "MifosPay" or "Mifos Mobile Wallet".
🔎 Proposed fix
- title = "DesktopApp", + title = "MifosPay",build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt-38-38 (1)
38-38: Fix spacing afterelsekeyword.Missing space after the closing brace.
🔎 Proposed fix
- }else { + } else {build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt-51-51 (1)
51-51: Key size validation is inconsistent withConfigurationValidator.Here,
keySize < 1024is an error, butConfigurationValidator.validateKeyAlgorithm()enforceskeySize < 2048for RSA. This inconsistency could allow 1024-bit RSA keys, which are considered weak.🔎 Suggested fix
-if (keySize < 1024) errors.add("Key size must be at least 1024 bits") +if (keySize < 2048) errors.add("Key size must be at least 2048 bits for secure key generation")build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt-42-48 (1)
42-48: Unreachable code:validity < 1condition can never be true.After checking
validity <= 0on line 42, any value that reaches line 44 must be>= 1. The conditionvalidity < 1is therefore unreachable for integer types.🔎 Suggested fix
// Validate validity period if (config.validity <= 0) { errors.add("Validity period must be positive") -} else if (config.validity < 1) { - warnings.add("Validity period is very short (${config.validity} years)") } else if (config.validity > 50) { warnings.add("Validity period is very long (${config.validity} years)") }If you want to warn about short validity (e.g., 1-2 years), change the condition:
} else if (config.validity <= 2) { warnings.add("Validity period is very short (${config.validity} years)") }build-logic/convention/src/main/kotlin/org/convention/keystore/BaseKeystoreTask.kt-81-96 (1)
81-96: Process output streams are not consumed, which may cause hangs on some systems.When a process writes to stdout/stderr and the buffers fill up,
waitFor()can block indefinitely. Additionally, there's no timeout, so a stuckkeytoolprocess would hang the build.🔎 Suggested fix
protected fun checkKeytoolAvailable(): Boolean { return try { - val process = ProcessBuilder("keytool", "-help").start() - val exitCode = process.waitFor() + val process = ProcessBuilder("keytool", "-help") + .redirectErrorStream(true) + .start() + // Consume output to prevent buffer blocking + process.inputStream.bufferedReader().readText() + val exitCode = process.waitFor(30, java.util.concurrent.TimeUnit.SECONDS) + .let { completed -> if (completed) process.exitValue() else -1 } if (exitCode == 0) { logInfo("keytool is available") true } else { logError("keytool command failed") false } } catch (e: Exception) { logError("keytool not found. Please ensure JDK is installed and keytool is in PATH") false } }build-logic/convention/src/main/kotlin/org/convention/AndroidCompose.kt-46-61 (1)
46-61: TherelativeToRootProjecthelper ignores its input provider's value.The
itparameter fromflatMapon line 48 is captured but never used. The function computes the directory path based onprojectDirregardless of what the provider contains. If the intent is simply to transform the provider to a directory, consider documenting that the input provider's value is intentionally discarded or refactor to usemapdirectly instead offlatMap.build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt-218-229 (1)
218-229: Overly broad regex patterns will match unrelated environment variables.Patterns like
.*UPLOAD.*,.*ORIGINAL.*,.*RELEASE.*,.*DEBUG.*will match many unrelated variables (e.g.,FILE_UPLOAD_LIMIT,ORIGINAL_IMAGE_PATH,RELEASE_DATE,DEBUG_MODE). Consider more specific patterns:🔎 Suggested more specific patterns
val keystorePatterns = listOf( ".*KEYSTORE.*", ".*SIGNING.*", "STORE_PASSWORD", "STORE_FILE", "KEY_ALIAS", "KEY_PASSWORD", "UPLOAD_KEYSTORE.*", "ORIGINAL_KEYSTORE.*", "RELEASE_KEYSTORE.*", "DEBUG_KEYSTORE.*" )build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt-167-172 (1)
167-172: Hardcoded Firebase app IDs look like real values.The Firebase app IDs on lines 168-169 appear to be real-looking identifiers rather than obvious placeholders. Consider using clearly fake placeholder values (e.g.,
"YOUR_FIREBASE_APP_ID_HERE") or making these configurable to avoid confusion.build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt-114-116 (1)
114-116: Unreachable code: same issue as multiline block handling.When
split("=", limit = 2)is called on a string thatcontains("="), it will always return exactly 2 parts. Thiselsebranch is dead code.build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt-296-303 (1)
296-303: Naming inconsistency: Constant names don't match their values.
ORIGINAL_KEYSTORE_PASSWORDcontains"ORIGINAL_KEYSTORE_FILE_PASSWORD"andUPLOAD_KEYSTORE_PASSWORDcontains"UPLOAD_KEYSTORE_FILE_PASSWORD". This is confusing and error-prone. Either align the constant names with the values or add clarifying comments.🔎 Proposed fix - align names
// Original keystore specific - const val ORIGINAL_KEYSTORE_PASSWORD = "ORIGINAL_KEYSTORE_FILE_PASSWORD" + const val ORIGINAL_KEYSTORE_FILE_PASSWORD = "ORIGINAL_KEYSTORE_FILE_PASSWORD" const val ORIGINAL_KEYSTORE_ALIAS = "ORIGINAL_KEYSTORE_ALIAS" const val ORIGINAL_KEYSTORE_ALIAS_PASSWORD = "ORIGINAL_KEYSTORE_ALIAS_PASSWORD" // Upload keystore specific - const val UPLOAD_KEYSTORE_PASSWORD = "UPLOAD_KEYSTORE_FILE_PASSWORD" + const val UPLOAD_KEYSTORE_FILE_PASSWORD = "UPLOAD_KEYSTORE_FILE_PASSWORD" const val UPLOAD_KEYSTORE_ALIAS = "UPLOAD_KEYSTORE_ALIAS" const val UPLOAD_KEYSTORE_ALIAS_PASSWORD = "UPLOAD_KEYSTORE_ALIAS_PASSWORD"Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt-96-98 (1)
96-98: Unreachable code:parts.sizewill always be 2 whenline.contains("<<")is true.When
split("<<", limit = 2)is called on a string thatcontains("<<"), it will always return exactly 2 parts. Theelsebranch on line 96-98 is dead code.🔎 Proposed fix
- if (parts.size == 2) { - multilineKey = parts[0].trim() - multilineEnd = parts[1].trim() - - if (multilineKey.isBlank()) { - errors.add("Line $lineNumber: Empty key in multiline declaration") - } else if (multilineEnd.isBlank()) { - errors.add("Line $lineNumber: Empty delimiter in multiline declaration") - } else { - inMultilineBlock = true - multilineValue.clear() - } - } else { - errors.add("Line $lineNumber: Invalid multiline syntax") - } + multilineKey = parts[0].trim() + multilineEnd = parts[1].trim() + + if (multilineKey.isBlank()) { + errors.add("Line $lineNumber: Empty key in multiline declaration") + } else if (multilineEnd.isBlank()) { + errors.add("Line $lineNumber: Empty delimiter in multiline declaration") + } else { + inMultilineBlock = true + multilineValue.clear() + }Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/Badging.kt-30-36 (1)
30-36: Complete the incomplete documentation sentence.The KDoc comment has an incomplete sentence on lines 34-35 that trails off without completing the explanation of incremental behavior.
🔎 Proposed fix
/** * Generates the badging information of the APK. * This task is cacheable, meaning that if the inputs and outputs have not changed, * the task will be considered up-to-date and will not run. - * This task is also incremental, meaning that if the inputs have not changed, - * + * This task is also incremental, meaning that if the inputs have not changed, + * the task will skip execution and reuse the previous output. */
| if (isLinuxOrMacOs()) { | ||
| commandLine("chmod", "-R", "+x", ".git/hooks/") | ||
| }else { | ||
| commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*") | ||
| } |
There was a problem hiding this comment.
Windows command incorrect: attrib doesn't set executable permissions.
The Windows command uses attrib -R +X, but attrib is for file attributes (read-only, hidden, system, archive) and doesn't control executable permissions. Windows doesn't use Unix-style executable bits. Git for Windows handles hook executability automatically when hooks are placed in .git/hooks/ without a file extension.
🔎 Recommended fix
For Windows, either skip the permission-setting step or simply verify hooks were copied:
if (isLinuxOrMacOs()) {
commandLine("chmod", "-R", "+x", ".git/hooks/")
- } else {
- commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*")
}Alternatively, you can make the Exec task conditional:
if (isLinuxOrMacOs()) {
commandLine("chmod", "-R", "+x", ".git/hooks/")
+ } else {
+ // Git for Windows handles hook executability automatically
+ commandLine("cmd", "/c", "echo", "Git hooks copied (Windows)")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isLinuxOrMacOs()) { | |
| commandLine("chmod", "-R", "+x", ".git/hooks/") | |
| }else { | |
| commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*") | |
| } | |
| if (isLinuxOrMacOs()) { | |
| commandLine("chmod", "-R", "+x", ".git/hooks/") | |
| } |
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt around
lines 36 - 40, The Windows branch is using an incorrect attrib command to set
executable bits; instead, make the permission-setting step only run on Unix by
removing or replacing the else branch that calls commandLine("cmd", "/c",
"attrib", ...). Update the isLinuxOrMacOs() conditional so that the Exec task
invokes commandLine("chmod", "-R", "+x", ".git/hooks/") only when true and
otherwise either do nothing or perform a harmless verification (e.g., check/copy
files exist), removing the incorrect attrib call; adjust the Exec task
configuration where commandLine(...) is used to reflect this change.
| apk.set( | ||
| variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for bundle-related build configurations
echo "=== Searching for bundle configurations ==="
rg -i "bundle" --type gradle -C 2
echo ""
echo "=== Searching for regular APK build configurations ==="
rg -i "assemble|apk" --type gradle -C 2 | head -100
echo ""
echo "=== Checking build.gradle files in repository ==="
find . -name "build.gradle*" -type f | head -20Repository: openMF/mobile-wallet
Length of output: 1206
🏁 Script executed:
#!/bin/bash
# Search gradle files directly without type restriction
echo "=== Searching for bundle configurations in gradle files ==="
rg -i "bundle" ./build.gradle.kts ./cmp-android/build.gradle.kts ./build-logic/convention/build.gradle.kts 2>/dev/null | head -50
echo ""
echo "=== Searching for APK/assemble configurations ==="
rg -i "apk|assemble" ./build.gradle.kts ./cmp-android/build.gradle.kts 2>/dev/null | head -50
echo ""
echo "=== Examining the Badging.kt file for context ==="
cat -n ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt | head -130
echo ""
echo "=== Check if there are other artifact types referenced in the codebase ==="
rg "SingleArtifact\." ./build-logic/convention/src/main/kotlin/org/convention/ 2>/dev/nullRepository: openMF/mobile-wallet
Length of output: 6133
🏁 Script executed:
#!/bin/bash
# Check the main build.gradle.kts to understand the project structure
echo "=== Examining main build.gradle.kts ==="
cat ./build.gradle.kts | head -100
echo ""
echo "=== Checking cmp-android build configuration ==="
cat ./cmp-android/build.gradle.kts | head -150
echo ""
echo "=== Check for bundle release configuration in cmp-android ==="
rg -i "release|bundle" ./cmp-android/build.gradle.kts | head -20Repository: openMF/mobile-wallet
Length of output: 7944
🏁 Script executed:
#!/bin/bash
# Check if there's any bundle configuration elsewhere
echo "=== Search for 'bundle' keyword across entire codebase ==="
rg "bundle" --type kotlin -C 2 | head -50
echo ""
echo "=== Check the entire Badging.kt file to understand context ==="
wc -l ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== View the rest of Badging.kt (lines 130-200) ==="
sed -n '130,200p' ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== Check if variant.artifacts supports APK_FROM_BUNDLE or if APK should be used ==="
rg "APK_FROM_BUNDLE|SingleArtifact\.APK" --type kotlin -B 3 -A 3Repository: openMF/mobile-wallet
Length of output: 8813
🏁 Script executed:
#!/bin/bash
# Check how the Badging tasks are integrated into the build
echo "=== Search for where configureBadgingTasks is called ==="
rg "configureBadgingTasks" --type kotlin -B 5 -A 5
echo ""
echo "=== Check if badging tasks are in the default build flow ==="
rg "configureBadgingTasks|GenerateBadgingTask|CheckBadgingTask" ./build-logic/convention/build.gradle.kts -C 3
echo ""
echo "=== Check the convention plugins to see if badging is always applied ==="
fd ".*\.kt$" ./build-logic/convention/src/main/kotlin/ | xargs rg "configureBadgingTasks" -lRepository: openMF/mobile-wallet
Length of output: 5060
🏁 Script executed:
#!/bin/bash
# Check if there are any CI/CD workflows or gradle tasks that actually build bundles
echo "=== Search for bundleRelease or bundle tasks in gradle ==="
rg "bundleRelease|bundle" ./cmp-android/build.gradle.kts -i
echo ""
echo "=== Check if there's a separate module or flavor for bundle builds ==="
rg "bundle" ./cmp-android/build.gradle.kts -C 5
echo ""
echo "=== Check the AndroidApplicationConventionPlugin more carefully ==="
cat ./build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt | head -80
echo ""
echo "=== Check if both Badging.kt files are identical (convention vs mifospay) ==="
diff ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt ./build-logic/convention/src/main/kotlin/org/mifospay/Badging.kt | head -50Repository: openMF/mobile-wallet
Length of output: 2718
🏁 Script executed:
#!/bin/bash
# Let me check if the project actually produces APK_FROM_BUNDLE artifacts
echo "=== Check if the app's build actually creates APK_FROM_BUNDLE artifacts ==="
rg "APK_FROM_BUNDLE" --type kotlin -B 3 -A 3
echo ""
echo "=== Verify what artifact types are available for main variants (not test) ==="
# Search for Android Gradle Plugin documentation usage
rg "SingleArtifact\." --type kotlin | grep -v androidTest | head -20
echo ""
echo "=== Check if there are any conditional checks or try-catch around APK_FROM_BUNDLE ==="
sed -n '100,156p' ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== Check git history to see if this is newly added or changed ==="
git log --oneline -n 20 -- ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt 2>/dev/null | head -10Repository: openMF/mobile-wallet
Length of output: 4393
Use SingleArtifact.APK instead of SingleArtifact.APK_FROM_BUNDLE for regular APK builds.
The project builds regular APKs (not Android App Bundles), as evidenced by the absence of any bundleRelease or bundle-related configuration. The current code uses SingleArtifact.APK_FROM_BUNDLE, which only provides APKs extracted from bundles and will not wire correctly for standard APK builds. Change to SingleArtifact.APK at line 117.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Badging.kt around
lines 116 - 118, Replace the artifact reference used when wiring APK outputs:
change the call that sets apk via
variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE) to use SingleArtifact.APK
instead so apk.set(...) points to regular APK outputs; update the reference in
the apk.set(...) line where variant.artifacts.get(...) is used to use
SingleArtifact.APK in place of SingleArtifact.APK_FROM_BUNDLE.
| internal fun Project.configureJacoco( | ||
| androidComponentsExtension: AndroidComponentsExtension<*, *, *>, | ||
| ) { | ||
| configure<JacocoPluginExtension> { | ||
| toolVersion = libs.findVersion("jacoco").get().toString() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the jacoco version is defined in the version catalog
# Expected: A 'jacoco' version entry should exist
fd -t f 'libs.versions.toml' --exec cat {} | grep -i 'jacoco'Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if libs.versions.toml exists
fd -t f 'libs.versions.toml'
# List gradle/build configuration files
echo "=== Gradle files ==="
fd -t f '\.(gradle|toml)$' | head -20
# Search for any jacoco references in the codebase
echo "=== JaCoCo references ==="
rg -i 'jacoco' --type gradle --type toml --type kotlin | head -30Repository: openMF/mobile-wallet
Length of output: 2188
🏁 Script executed:
cat gradle/libs.versions.tomlRepository: openMF/mobile-wallet
Length of output: 28053
Add JaCoCo version to the version catalog.
The code references libs.findVersion("jacoco") but no jacoco version entry exists in gradle/libs.versions.toml. This will cause a NoSuchElementException at runtime when .get() is called on an empty Optional. Define the jacoco version in the version catalog under the [versions] section.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Jacoco.kt around lines
61 - 66, The Project.configureJacoco code calls libs.findVersion("jacoco").get()
which will throw if the version key is missing; add a "jacoco" entry to the
version catalog (gradle/libs.versions.toml) under the [versions] section (e.g.
jacoco = "0.8.8" or your desired version) so libs.findVersion("jacoco") returns
a value for configure<JacocoPluginExtension> in configureJacoco.
| private fun createNewFastlaneConfig(configFile: File, config: KeystoreConfig, keystoreName: String) { | ||
| val content = """ | ||
| module FastlaneConfig | ||
| module AndroidConfig | ||
| STORE_CONFIG = { | ||
| default_store_file: "$keystoreName", | ||
| default_store_password: "${config.keystorePassword}", | ||
| default_key_alias: "${config.keyAlias}", | ||
| default_key_password: "${config.keyPassword}" | ||
| } | ||
|
|
||
| FIREBASE_CONFIG = { | ||
| firebase_prod_app_id: "1:728433984912738:android:3902eb32kjaska3363b0938f1a1dbb", | ||
| firebase_demo_app_id: "1:72843493212738:android:8392hjks3298ak9032skja", | ||
| firebase_service_creds_file: "secrets/firebaseAppDistributionServiceCredentialsFile.json", | ||
| firebase_groups: "mifos-mobile-apps" | ||
| } | ||
|
|
||
| BUILD_PATHS = { | ||
| prod_apk_path: "cmp-android/build/outputs/apk/prod/release/cmp-android-prod-release.apk", | ||
| demo_apk_path: "cmp-android/build/outputs/apk/demo/release/cmp-android-demo-release.apk", | ||
| prod_aab_path: "cmp-android/build/outputs/bundle/prodRelease/cmp-android-prod-release.aab" | ||
| } | ||
| end | ||
| end | ||
| """.trimIndent() | ||
|
|
||
| configFile.writeText(content) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if .gitignore exists and what it contains regarding fastlane
cat .gitignore | grep -i fastlane || echo "No fastlane entries found"Repository: openMF/mobile-wallet
Length of output: 83
🏁 Script executed:
# Check if there are any references to the actual fastlane config directory
rg -i "fastlane" --type kotlin -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 15074
🏁 Script executed:
# Also check what the actual output path is in the code
rg "configFile" build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt -B 5 -A 5Repository: openMF/mobile-wallet
Length of output: 2708
🏁 Script executed:
# Check entire .gitignore for any patterns that might match fastlane-config
cat .gitignoreRepository: openMF/mobile-wallet
Length of output: 1302
Add fastlane-config/ directory to .gitignore to prevent credential exposure.
The fastlane config file writes plaintext keystore passwords to fastlane-config/android_config.rb, which is not currently gitignored and risks accidental version control commits. Add fastlane-config/ to .gitignore immediately.
🤖 Prompt for AI Agents
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt
around lines 156 - 184, Add the fastlane-config directory to .gitignore to
prevent committing plaintext credentials: update the repo .gitignore to include
"fastlane-config/" (or "fastlane-config/**") so files written by
createNewFastlaneConfig (which writes the Android Fastlane config via
configFile.writeText) are ignored; commit the .gitignore change and verify that
fastlane-config/android_config.rb is no longer tracked (remove from git index if
already committed using git rm --cached) before merging.
| private fun buildKeytoolCommand(config: KeystoreConfig, keystorePath: File): List<String> { | ||
| return listOf( | ||
| "keytool", | ||
| "-genkey", | ||
| "-v", | ||
| "-keystore", keystorePath.absolutePath, | ||
| "-alias", config.keyAlias, | ||
| "-keyalg", config.keyAlgorithm, | ||
| "-keysize", config.keySize.toString(), | ||
| "-validity", (config.validity * 365).toString(), // Convert years to days | ||
| "-storepass", config.keystorePassword, | ||
| "-keypass", config.keyPassword, | ||
| "-dname", config.distinguishedName | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
keytool -storepass env file password input methods
💡 Result:
Options for supplying a keystore password to keytool
-
Direct on command line (not recommended — visible in process list/shell history):
keytool ... -storepass MyPassword -
From an environment variable (recommended over plain CLI):
keytool ... -storepass:env MY_VAR
(keytool reads the env var named MY_VAR). [1] -
From a file:
keytool ... -storepass:file /path/to/passfile
(keytool reads the file contents as the password). [1] -
Interactive prompt:
Omit -storepass and keytool will prompt you to type the password (no echo). [1]
Notes / best practices
- Avoid plain CLI passwords (exposed in history / ps). Use -storepass:env or -storepass:file, or the interactive prompt. [1][2]
- Ensure file permissions or environment handling keep secrets protected (e.g., restrict file mode, avoid exporting vars in shared shells). [2]
Sources: Oracle keytool docs and community examples. [1][2]
🏁 Script executed:
# First, let's examine the file context to understand how passwords are handled
cd build-logic/convention/src/main/kotlin/org/convention/keystore && wc -l KeystoreGenerationTask.ktRepository: openMF/mobile-wallet
Length of output: 93
🏁 Script executed:
# Examine the file to understand the password handling context
cat -n build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt | head -200 | tail -100Repository: openMF/mobile-wallet
Length of output: 4706
🏁 Script executed:
# Also check how this function is called and where passwords come from
rg -B5 -A5 "buildKeytoolCommand" build-logic/convention/src/main/kotlin/org/convention/keystore/Repository: openMF/mobile-wallet
Length of output: 2696
🏁 Script executed:
# Check the executeKeytoolCommand function to see how the command is executed
cat -n build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt | sed -n '196,230p'Repository: openMF/mobile-wallet
Length of output: 1659
🏁 Script executed:
# Check KeystoreConfig to understand where passwords come from
rg -B3 -A10 "class KeystoreConfig" build-logic/convention/src/main/kotlin/org/convention/keystore/Repository: openMF/mobile-wallet
Length of output: 1718
Security: Passwords exposed in command-line arguments and build logs.
Passwords passed directly as command-line arguments (-storepass, -keypass) are exposed in process listings (ps aux) and build logs. Line 198 logs the complete command including passwords via logInfo(). Use keytool's -storepass:env or -storepass:file options instead, and mask sensitive values when logging commands.
| private fun executeKeytoolCommand(command: List<String>): Boolean { | ||
| return try { | ||
| logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") | ||
|
|
There was a problem hiding this comment.
Critical: Logging passwords in plain text.
Line 198 logs the entire command including -storepass and -keypass values. This will expose keystore passwords in build logs.
🔎 Proposed fix - mask sensitive arguments
private fun executeKeytoolCommand(command: List<String>): Boolean {
return try {
- logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}")
+ // Mask password arguments in logs
+ val maskedCommand = command.mapIndexed { index, arg ->
+ val prevArg = command.getOrNull(index - 1)
+ if (prevArg in listOf("-storepass", "-keypass")) "****" else arg
+ }
+ logInfo("Command: ${maskedCommand.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun executeKeytoolCommand(command: List<String>): Boolean { | |
| return try { | |
| logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") | |
| private fun executeKeytoolCommand(command: List<String>): Boolean { | |
| return try { | |
| // Mask password arguments in logs | |
| val maskedCommand = command.mapIndexed { index, arg -> | |
| val prevArg = command.getOrNull(index - 1) | |
| if (prevArg in listOf("-storepass", "-keypass")) "****" else arg | |
| } | |
| logInfo("Command: ${maskedCommand.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") |
🤖 Prompt for AI Agents
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt
around lines 196 - 199, The log currently prints the full command (including
sensitive `-storepass`/`-keypass` values) in executeKeytoolCommand; change the
logging to mask sensitive arguments by constructing a maskedCommand from the
command List<String> before logInfo: treat both "--flag=value" and separate
flag/value forms, replace the password value with a fixed mask (e.g. "****")
whenever an element startsWith("-storepass") or startsWith("-keypass") or
immediately follows those flags, and then call logInfo with the maskedCommand
instead of the raw command to avoid exposing keystore passwords.
| import com.diffplug.gradle.spotless.SpotlessExtension | ||
| import org.gradle.api.Project | ||
|
|
||
| const val ktlintVersion = "1.0.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ktlint latest version 2025 stable release
💡 Result:
The latest stable ktlint release is 1.8.0 (released Nov 14, 2025). [1][2]
Sources:
[1] GitHub Releases — pinterest/ktlint (1.8.0).
[2] Maven / mvnrepository listing for ktlint artifacts (shows 1.8.0 as the newest version).
🌐 Web query:
ktlint 1.0.1 security vulnerabilities CVE
💡 Result:
Short answer: ktlint 1.0.1 has no CVE assigned to the ktlint project itself, but scanners/metadata flag a dependency vulnerability (CVE-2023-6378 — a Logback deserialization/DOS issue). The older ktlint CVE (CVE-2019-1010260) affected ktlint versions before 0.30.0 and therefore does NOT apply to 1.0.1. [1][2][3]
Sources:
- mvnrepository entry for ktlint-cli 1.0.1 (lists CVE-2023-6378 from dependencies). [1]
- NVD entry for CVE-2019-1010260 (ktlint custom-ruleset MITM → RCE; fixed in 0.30.0+). [2]
- NVD / vendor pages for CVE-2023-6378 (Logback receiver deserialization DoS). [3]
Upgrade ktlint from 1.0.1 to 1.8.0.
ktlint 1.0.1 is significantly outdated and has a transitive dependency vulnerability (CVE-2023-6378 in Logback, a deserialization/DoS issue). The latest stable release is 1.8.0.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Spotless.kt at line 6,
Update the ktlint version constant to remove the vulnerable old release: change
the const val ktlintVersion (in Spotless.kt) from "1.0.1" to "1.8.0", then
refresh dependencies/lockfiles and run the build/spotless checks to ensure
compatibility and resolve any formatter rule changes from the upgrade.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt:
- Around line 3-4: Replace the internal TestNG imports with JUnit 5 assertions
by removing imports of org.gradle.internal.impldep.org.testng.Assert and
importing org.junit.jupiter.api.Assertions; update all assertion calls in
SecretsEnvUpdateTaskTest (e.g., usages of assertEquals and assertTrue) to use
Assertions.assertEquals/assertTrue and, for assertEquals, swap the argument
order from (actual, expected, message) to (expected, actual, message) to match
JUnit 5's signature; ensure static import or fully-qualified References to
Assertions are consistent across the test file.
🧹 Nitpick comments (1)
build-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.kt (1)
269-363: Test double doesn't test the actual task and misses critical error-recovery logic.The
TestConfigurationFileUpdatesTaskreimplements helper methods instead of testing the realConfigurationFileUpdatesTask. More critically, the test'stestUpdateGradleConfig()is missing the backup-and-restore mechanism present in the actual implementation (lines 214–250 of the real task), meaning backup file failures go undetected.Recommended approaches:
- Test the actual task via Gradle's
ProjectBuilderinstead of a duplicate helper class- Extract the configuration update logic into a utility class that both the real task and tests can use
- Ensure test coverage includes the backup/recovery error path
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build-logic/convention/build.gradle.ktsbuild-logic/convention/src/main/kotlin/KMPRoomConventionPlugin.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/KeystoreGenerationTaskTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvParserTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.ktgradle/libs.versions.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (17)
build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt (2)
34-51: LGTM!The test setup is well-structured, using
ProjectBuilderfor Gradle integration testing and@TempDirfor isolated file system operations. The configuration of task properties is clear and follows good testing practices.
376-437: LGTM!The helper methods are well-designed for test isolation. The base64 validation helpers (
isValidBase64,isValidBase64Line,extractBase64Content) provide good utilities for verifying the encoding output, and the file creation helpers set up realistic test scenarios.build-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.kt (2)
25-44: LGTM!The test setup creates a comprehensive
KeystoreConfigwith all relevant fields populated. The use of@TempDirfor file isolation and proper cleanup via the annotation is appropriate.
184-202: LGTM!Good validation coverage testing multiple error conditions in a single configuration. The test verifies that all expected validation messages are present for invalid inputs (empty password/alias, small key size, negative validity, invalid country code).
build-logic/convention/src/test/kotlin/org/convention/keystore/KeystoreGenerationTaskTest.kt (3)
22-29: LGTM!The setup correctly uses
ProjectBuilderto create a Gradle project context and registers the actualKeystoreGenerationTaskfor testing. This approach ensures tests exercise the real implementation rather than a test double.
91-157: LGTM!Excellent test coverage for loading configuration from secrets. The test validates all expected fields for both ORIGINAL and UPLOAD keystore configurations, including passwords, aliases, DN components, cryptographic parameters, and the overwrite flag.
243-275: LGTM!Good validation testing that covers both valid and invalid configurations. The test properly checks for specific error messages using
any { it.contains(...) }which is flexible enough to handle message variations while still verifying the error types.build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvParserTest.kt (4)
24-28: LGTM!Simple and clean test setup. Using a default
SecretsConfig()is appropriate since individual tests can override configuration as needed.
151-170: LGTM!Good error handling coverage testing both empty delimiter (
BAD_KEY<<) and unterminated block scenarios. The assertions properly verify that specific error messages are generated for each failure mode.
360-382: LGTM!This is an important edge case test. The parser correctly treats everything between the outer
OUTERdelimiters as literal content, including what looks like nested heredoc syntax. This matches expected shell heredoc behavior where the content is opaque until the terminating delimiter.
339-358: LGTM!Comprehensive coverage of malformed input scenarios. The test verifies that the parser:
- Successfully parses valid lines
- Handles empty values (
key_without_value=)- Reports errors for truly malformed lines (missing key, no equals sign)
- Continues parsing after encountering errors (partial parse with error reporting)
build-logic/convention/src/main/kotlin/KMPRoomConventionPlugin.kt (1)
1-17: LGTM! Clean plugin setup.The plugin structure and KSP configuration for Room are correct.
gradle/libs.versions.toml (2)
206-209: LGTM! Library declarations follow standard conventions.The library declarations are properly structured and correctly reference the version catalog entries.
Also applies to: 242-250
42-46: Consider upgrading Jackson and OkHttp to current releases; JUnit version is safe but behind latest.These versions exist and are valid. However:
- JUnit 5.13.4 is behind the latest (6.0.2, released Jan 6, 2026), but is safe: it includes the fix for CVE-2025-53103 that affected 5.12.0–5.13.1.
- Jackson 2.19.2 (latest 2.x: 2.20.1) addressed CVEs in the 2.19 line; consider upgrading if compatible.
- OkHttp 4.12.0 (latest: 5.3.0) lacks fixes for upstream vulnerabilities (okio CVE-2023-3635, brotli decompression-bomb DoS); upgrade recommended if compatible.
Likely an incorrect or invalid review comment.
build-logic/convention/build.gradle.kts (3)
32-49: LGTM! Dependencies are correctly configured.
- Room gradle plugin correctly added as
compileOnly- Keystore management dependencies properly declared as
implementation- JUnit 5 test dependencies appropriately configured
57-63: LGTM! Test configuration is correct.JUnit Platform is properly configured with useful test logging.
131-143: Plugin implementation classes are properly defined.Both plugin registrations reference existing implementation classes in
build-logic/convention/src/main/kotlin/:
KMPRoomConventionPlugin.kt✓KeystoreManagementConventionPlugin.kt✓The registrations are correctly structured.
| dependencies { | ||
| // Adding ksp dependencies for multiple platforms | ||
| "implementation"(libs.findLibrary("androidx.room.ktx").get()) | ||
| listOf( | ||
| "kspDesktop", | ||
| "kspAndroid", | ||
| "kspIosArm64", | ||
| "kspIosX64", | ||
| "kspIosSimulatorArm64", | ||
| // Add any other platform you may support | ||
| ).forEach { platform -> | ||
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | ||
| // Kotlin Extensions and Coroutines support for Room | ||
| // add(platform, libs.findLibrary("androidx.room.ktx").get()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Android-specific dependency in KMP context will break non-Android builds.
Line 28 adds androidx.room.ktx as a general "implementation" dependency, but this is Android-specific and will cause failures on other KMP targets (Desktop, iOS).
In KMP projects, platform-specific dependencies must be added conditionally. The Room KTX extension should only be added for Android.
🔧 Proposed fix for platform-specific dependencies
dependencies {
// Adding ksp dependencies for multiple platforms
- "implementation"(libs.findLibrary("androidx.room.ktx").get())
listOf(
"kspDesktop",
"kspAndroid",
"kspIosArm64",
"kspIosX64",
"kspIosSimulatorArm64",
// Add any other platform you may support
).forEach { platform ->
add(platform, libs.findLibrary("androidx.room.compiler").get())
-// Kotlin Extensions and Coroutines support for Room
-// add(platform, libs.findLibrary("androidx.room.ktx").get())
+ // Add Room KTX only for Android (it's Android-specific)
+ if (platform == "kspAndroid") {
+ add("androidMainImplementation", libs.findLibrary("androidx.room.ktx").get())
+ }
}
}Alternatively, configure it using Kotlin Multiplatform source sets:
// After the dependencies block, configure source sets
val commonMain = extensions.getByType<KotlinMultiplatformExtension>()
.sourceSets.getByName("commonMain")
val androidMain = extensions.getByType<KotlinMultiplatformExtension>()
.sourceSets.getByName("androidMain")
androidMain.dependencies {
implementation(libs.findLibrary("androidx.room.ktx").get())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dependencies { | |
| // Adding ksp dependencies for multiple platforms | |
| "implementation"(libs.findLibrary("androidx.room.ktx").get()) | |
| listOf( | |
| "kspDesktop", | |
| "kspAndroid", | |
| "kspIosArm64", | |
| "kspIosX64", | |
| "kspIosSimulatorArm64", | |
| // Add any other platform you may support | |
| ).forEach { platform -> | |
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | |
| // Kotlin Extensions and Coroutines support for Room | |
| // add(platform, libs.findLibrary("androidx.room.ktx").get()) | |
| } | |
| } | |
| dependencies { | |
| // Adding ksp dependencies for multiple platforms | |
| listOf( | |
| "kspDesktop", | |
| "kspAndroid", | |
| "kspIosArm64", | |
| "kspIosX64", | |
| "kspIosSimulatorArm64", | |
| // Add any other platform you may support | |
| ).forEach { platform -> | |
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | |
| } | |
| } | |
| // Add Room KTX only for Android using proper KMP sourceSet configuration | |
| extensions.getByType<KotlinMultiplatformExtension>().apply { | |
| sourceSets.getByName("androidMain").dependencies { | |
| implementation(libs.findLibrary("androidx.room.ktx").get()) | |
| } | |
| } |
| import org.gradle.internal.impldep.org.testng.Assert.assertEquals | ||
| import org.gradle.internal.impldep.org.testng.Assert.assertTrue |
There was a problem hiding this comment.
Avoid using internal Gradle APIs for assertions.
The imports org.gradle.internal.impldep.org.testng.Assert reference internal Gradle implementation classes that are not part of the public API and may change or be removed without notice. Use org.junit.jupiter.api.Assertions instead, which is the standard for JUnit 5 and is already used in other test files in this PR.
Proposed fix
-import org.gradle.internal.impldep.org.testng.Assert.assertEquals
-import org.gradle.internal.impldep.org.testng.Assert.assertTrue
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertTrueNote: After switching to JUnit 5 assertions, the argument order for assertEquals changes from (actual, expected, message) to (expected, actual, message). You'll need to swap the first two arguments in calls like:
-assertEquals(originalKeystoreFile.readBytes().toList(), originalDecoded.toList(),
- "Decoded original keystore should match original file")
+assertEquals(originalDecoded.toList(), originalKeystoreFile.readBytes().toList(),
+ "Decoded original keystore should match original file")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.gradle.internal.impldep.org.testng.Assert.assertEquals | |
| import org.gradle.internal.impldep.org.testng.Assert.assertTrue | |
| import org.junit.jupiter.api.Assertions.assertEquals | |
| import org.junit.jupiter.api.Assertions.assertTrue |
🤖 Prompt for AI Agents
In
@build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt
around lines 3 - 4, Replace the internal TestNG imports with JUnit 5 assertions
by removing imports of org.gradle.internal.impldep.org.testng.Assert and
importing org.junit.jupiter.api.Assertions; update all assertion calls in
SecretsEnvUpdateTaskTest (e.g., usages of assertEquals and assertTrue) to use
Assertions.assertEquals/assertTrue and, for assertEquals, swap the argument
order from (actual, expected, message) to (expected, actual, message) to match
JUnit 5's signature; ensure static import or fully-qualified References to
Assertions are consistent across the test file.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core-base/ui/README.md (2)
388-405: Update code examples to provide requiredcontextparameter torememberImageLoader()andrememberImageRequest().Both the
CircularProfileImageandBackgroundImageexamples are broken. The functionsrememberImageLoader(context: PlatformContext)andrememberImageRequest(context: PlatformContext, url: String)require a context parameter, but the examples either don't pass it or reference an undefinedcontextvariable.🐛 Fixes required
For
CircularProfileImage(lines 388-405):@Composable fun CircularProfileImage(url: String, size: Dp = 48.dp) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) AsyncImage( model = rememberImageRequest(context, url),For
BackgroundImage(lines 411-432):@Composable fun BackgroundImage(url: String, overlay: Color = Color.Black.copy(alpha = 0.3f)) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) Box { AsyncImage( model = rememberImageRequest(context, url),
411-431: BothrememberImageLoader()andrememberImageRequest()require context parameter, but none is provided.Line 414 calls
rememberImageLoader()without the requiredPlatformContextparameter, and line 417 references an undefinedcontextvariable inrememberImageRequest(). Both function signatures require a context parameter that must be retrieved fromLocalPlatformContext.current:🐛 Proposed fix
@Composable fun BackgroundImage(url: String, overlay: Color = Color.Black.copy(alpha = 0.3f)) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) Box { AsyncImage( model = rememberImageRequest(context, url),cmp-android/src/main/AndroidManifest.xml (1)
14-16: Re-add the android.permission.CAMERA permission to the manifest.The codebase uses CameraX (androidx.camera library) extensively for QR code scanning in
feature/qr, not intent-based camera access as assumed. CameraX requires theandroid.permission.CAMERApermission to function. Without it, the camera will fail to initialize and the QR scanning feature will not work. The optional hardware feature declaration does not eliminate the need for the runtime permission.
🤖 Fix all issues with AI agents
In @build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt:
- Line 42: KMPLibraryConventionPlugin currently adds the
kotlinx.serialization.json dependency but does not apply the Kotlin
serialization compiler plugin, causing @Serializable uses to fail; update
KMPLibraryConventionPlugin to also apply the
"org.jetbrains.kotlin.plugin.serialization" plugin when configuring the KMP
convention (or alternatively remove the add("commonMainImplementation",
libs.findLibrary("kotlinx.serialization.json").get()) line and require modules
to opt into both the dependency and plugin themselves); locate
KMPLibraryConventionPlugin and either call the Kotlin serialization plugin
application in its setup or remove the serialization dependency entry to keep
plugin/dependency inclusion consistent.
In @cmp-android/src/prod/AndroidManifest.xml:
- Around line 16-19: The prod manifest enables
google_analytics_adid_collection_enabled via the meta-data tag
(android:name="google_analytics_adid_collection_enabled") but the main manifest
intentionally strips the AD_ID permission; to resolve, change the prod manifest
meta-data android:value to "false" to match the main manifest intent (or
alternatively remove the AD_ID removal in the main manifest and add proper
disclosure if you genuinely need ad ID collection); update the meta-data entry
in the prod manifest (google_analytics_adid_collection_enabled) or adjust the
AD_ID permission removal to keep manifest behavior consistent and avoid runtime
failures.
In
@core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt:
- Around line 84-96: The SMS URL in sendViaSMS is using "&" between the
recipient and query string; change it to use "?" so the body query parameter is
valid: update the smsUrl construction in sendViaSMS (where smsUrl is built) to
use "sms:$number?body=$encodedMessage" when number is present and
"sms:?body=$encodedMessage" when empty, keeping the existing encode function and
the subsequent
NSURL.URLWithString/UIApplication.sharedApplication.canOpenURL/openURL logic
intact.
🧹 Nitpick comments (6)
core-base/ui/src/jsCommonMain/kotlin/template/core/base/ui/ShareUtils.kt (1)
46-62: Document or implement the empty JS platform functions.All six utility functions are empty stubs with no implementation. If these operations are not supported on the JS/web platform, consider:
- Adding KDoc comments explaining why they're no-ops
- Throwing
NotImplementedErrorto make the limitation explicit to callers- Implementing web-based alternatives where possible (e.g.,
openUrlcould usewindow.open())Silent no-ops may confuse developers who expect these functions to work.
core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt (1)
69-82: Extract the duplicateencodefunction to reduce code duplication.The local
encodefunction is defined identically in bothsendEmail(line 70) andsendViaSMS(line 85). Extract it as a private top-level function or extension to avoid duplication.♻️ Proposed refactor
Add this private function before the
ShareUtilsobject:private fun String.encodeForUrl(): String = replace(" ", "%20").replace("\n", "%0A")Then use it in both functions:
actual fun sendEmail(to: String, subject: String?, body: String?) { - fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") - val encodedSubject = subject?.let { encode(it) } ?: "" - val encodedBody = body?.let { encode(it) } ?: "" + val encodedSubject = subject?.encodeForUrl() ?: "" + val encodedBody = body?.encodeForUrl() ?: "" // ... rest of implementation }core-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.kt (1)
47-110: Consider using structured logging instead of println.Error messages are printed to console using
println(lines 53, 67, 82, 98, 108). For production code, consider using a proper logging framework (e.g., SLF4J, kotlin-logging) to:
- Enable log level control
- Support structured logging and filtering
- Facilitate debugging in production environments
core-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt (2)
108-116: Remove redundantletblock.Line 110 uses
url.let { url.toUri() }, but the lambda doesn't use theitparameter and directly referencesurl. This can be simplified to justurl.toUri().♻️ Proposed refactor
actual fun openUrl(url: String) { val context = ShareUtils.activityProvider.invoke().application.baseContext - val uri = url.let { url.toUri() } + val uri = url.toUri() val intent = Intent(Intent.ACTION_VIEW).apply { data = uri addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } context.startActivity(intent) }
108-173: Consider adding error handling for Intent operations.None of the new functions handle
ActivityNotFoundExceptionthat could be thrown if no app is available to handle the intent. While this might be acceptable for the current use case, consider wrappingstartActivitycalls in try-catch blocks to prevent crashes.Example:
try { context.startActivity(intent) } catch (e: ActivityNotFoundException) { Log.w("ShareUtils", "No activity found to handle intent: ${e.message}") }cmp-android/src/prod/AndroidManifest.xml (1)
21-25: Clarify the double-negative configuration naming.The comment says "Enable Firebase analytics for
prodbuilds" but the metadata name isfirebase_analytics_collection_deactivatedset tofalse. While the logic is correct (false = not deactivated = enabled), the double-negative is confusing.Consider adding a clarifying comment such as:
<!-- Enable Firebase analytics for `prod` builds (deactivated=false means enabled) -->
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build-logic/convention/src/main/kotlin/AndroidApplicationComposeConventionPlugin.ktbuild-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.ktbuild-logic/convention/src/main/kotlin/KMPKoinConventionPlugin.ktbuild-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktcmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/src/main/AndroidManifest.xmlcmp-android/src/prod/AndroidManifest.xmlcmp-ios/iosApp.xcodeproj/project.xcworkspace/xcuserdata/hekmatullahamin.xcuserdatad/UserInterfaceState.xcuserstatecmp-web/build.gradle.ktscore-base/ui/README.mdcore-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.ktcore-base/ui/src/commonMain/kotlin/template/core/base/ui/ShareUtils.ktcore-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.ktcore-base/ui/src/jsCommonMain/kotlin/template/core/base/ui/ShareUtils.ktcore-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt
✅ Files skipped from review due to trivial changes (3)
- build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt
- build-logic/convention/src/main/kotlin/AndroidApplicationComposeConventionPlugin.kt
- build-logic/convention/src/main/kotlin/KMPKoinConventionPlugin.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (5)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txt (1)
1-3082: This is a generated dependency tree artifact—verify actual dependency declarations in build.gradle files.This file documents the resolved Gradle dependency graph for the
prodReleaseRuntimeClasspathconfiguration. The marked changes showkotlinx-serialization-json:1.8.1being consistently added across numerous modules (core:common, core:datastore, core:model, core-base:network, and all feature modules).Key observations:
- No dependency conflicts or resolution issues are evident; version convergence is clean.
- The serialization library addition aligns with the PR objective to include template modules.
Recommendations:
- Verify that the actual dependency declarations (e.g.,
build.gradle.ktsorgradle.toml) in each module have been updated to explicitly declarekotlinx-serialization-json:1.8.1. This generated tree is a consequence, not the source.- Check whether this file should be committed to version control. Dependency trees can often be regenerated on-demand via
./gradlew dependencies, so verify whether keeping this in the repo adds value or introduces merge-conflict noise.- Run
./gradlew check(as mentioned in the PR checklist) to ensure the new dependencies don't introduce compilation or test failures.cmp-web/build.gradle.kts (1)
53-54: No issues with the Koin and Ktor client dependencies added at lines 53–54.
libs.koin.coreis actively used viainitKoin()in bothjsMainandwasmJsMainentry points.libs.ktor.client.jscorrectly provides the JavaScript engine;ktor-client-coreis already available through the transitive dependency chain viacore-base/network(which exposes it as anapidependency), and the sameJsengine is properly configured for both JS and WASM targets incore-base/network/src/jsMain/andcore-base/network/src/wasmJsMain/.core-base/ui/src/commonMain/kotlin/template/core/base/ui/ShareUtils.kt (1)
43-87: LGTM! Well-documented API surface.The expect declarations are clear, well-documented, and provide a consistent cross-platform API for utility operations. The KDoc comments effectively explain the purpose and parameters of each function.
cmp-android/src/main/AndroidManifest.xml (1)
69-76: Firebase configuration pattern looks good.The default-disabled configuration for Firebase analytics and Crashlytics in the main manifest, with the prod variant overriding to enable them, follows a good privacy-conscious pattern. This ensures analytics/crash reporting is only active in production builds.
build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt (1)
12-14: LGTM!Clear and concise documentation that accurately describes the plugin's purpose.
| } | ||
|
|
||
| dependencies { | ||
| add("commonMainImplementation", libs.findLibrary("kotlinx.serialization.json").get()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for serialization plugin application in convention plugins
rg -n "plugin.serialization|kotlin.*serialization" build-logic/convention/src/ -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 2069
🏁 Script executed:
#!/bin/bash
# Read the full KMPLibraryConventionPlugin.kt file to see context around line 42
wc -l build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 135
🏁 Script executed:
#!/bin/bash
# List all convention plugin files to check for serialization plugin elsewhere
fd -e ".kt" build-logic/convention/src/main/kotlin/ | head -20Repository: openMF/mobile-wallet
Length of output: 525
🏁 Script executed:
#!/bin/bash
cat -n build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 2229
🏁 Script executed:
#!/bin/bash
# Also read the CMPFeatureConventionPlugin.kt to see the context
cat -n build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 3905
🏁 Script executed:
#!/bin/bash
# Search for modules/build files that apply org.convention.kmp.library
rg -n "org.convention.kmp.library" --type kotlin --type gradle-kotlinRepository: openMF/mobile-wallet
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Also search for plugins.id pattern in build files
fd -e ".gradle.kts" -e ".gradle" | head -30Repository: openMF/mobile-wallet
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Search for org.convention.kmp.library in build files
rg -n "org.convention.kmp.library" .Repository: openMF/mobile-wallet
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Also search for which plugins are commonly applied
rg -n "id\(\"org.convention" . -A 1 -B 1 | head -50Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for modules that use org.convention.kmp.library directly
rg -n "kmp-library-convention|org\.convention\.kmp\.library" --type gradle-kts . 2>/dev/null || rg -n "kmp-library-convention|org\.convention\.kmp\.library" .Repository: openMF/mobile-wallet
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Check a few module build files to see what convention plugins they apply
head -20 core/common/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Check more module build files
head -20 core/data/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 681
🏁 Script executed:
#!/bin/bash
# Check more module build files to see the pattern of plugin usage
for file in core/designsystem/build.gradle.kts core/domain/build.gradle.kts core/model/build.gradle.kts core/network/build.gradle.kts; do
echo "=== $file ===" && head -20 "$file" && echo ""
doneRepository: openMF/mobile-wallet
Length of output: 2506
🏁 Script executed:
#!/bin/bash
# Count how many modules apply kotlin.serialization
rg -l "kotlin.serialization|kotlin\.serialization" . --glob "*/build.gradle.kts" | wc -lRepository: openMF/mobile-wallet
Length of output: 65
🏁 Script executed:
#!/bin/bash
# List all modules that apply kmp-library-convention
rg -l "kmp.library.convention|kmp-library-convention" . --glob "*/build.gradle.kts"Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get a complete list of modules by checking all build.gradle.kts files
find . -name "build.gradle.kts" -not -path "*/build-logic/*" -not -path "*/build/*" -not -path "*/.gradle/*" | sortRepository: openMF/mobile-wallet
Length of output: 1668
🏁 Script executed:
#!/bin/bash
# Check which modules apply serialization plugin
for file in $(find . -name "build.gradle.kts" -not -path "*/build-logic/*" -not -path "*/build/*" -not -path "*/.gradle/*" | sort); do
if grep -q "kotlin.serialization\|kotlinx-serialization" "$file" 2>/dev/null; then
echo "$file: YES"
else
echo "$file: NO"
fi
doneRepository: openMF/mobile-wallet
Length of output: 1866
🏁 Script executed:
#!/bin/bash
# Check the full plugin section of core/designsystem/build.gradle.kts since it was truncated
head -40 core/designsystem/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Check core/domain which applies kmp-library-convention without serialization
cat -n core/domain/build.gradle.kts | head -30Repository: openMF/mobile-wallet
Length of output: 1117
Add the Kotlin serialization plugin to KMPLibraryConventionPlugin.
The kotlinx-serialization-json dependency is added at line 42, but the org.jetbrains.kotlin.plugin.serialization plugin is not applied in this convention plugin. This creates a mismatch: modules that use KMPLibraryConventionPlugin (like core/domain, core/designsystem, core/ui) receive the dependency but lack the required compiler plugin. If any of these modules attempt to use @Serializable annotations, compilation will fail.
Either apply the serialization plugin here alongside the dependency, or remove the dependency and require it to be added explicitly in modules that need it (along with the plugin).
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt at line
42, KMPLibraryConventionPlugin currently adds the kotlinx.serialization.json
dependency but does not apply the Kotlin serialization compiler plugin, causing
@Serializable uses to fail; update KMPLibraryConventionPlugin to also apply the
"org.jetbrains.kotlin.plugin.serialization" plugin when configuring the KMP
convention (or alternatively remove the add("commonMainImplementation",
libs.findLibrary("kotlinx.serialization.json").get()) line and require modules
to opt into both the dependency and plugin themselves); locate
KMPLibraryConventionPlugin and either call the Kotlin serialization plugin
application in its setup or remove the serialization dependency entry to keep
plugin/dependency inclusion consistent.
| <meta-data | ||
| android:name="google_analytics_adid_collection_enabled" | ||
| android:value="true" | ||
| tools:replace="android:value" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd AndroidManifest.xml | head -20Repository: openMF/mobile-wallet
Length of output: 1081
🏁 Script executed:
cat -n cmp-android/src/main/AndroidManifest.xmlRepository: openMF/mobile-wallet
Length of output: 3474
🏁 Script executed:
cat -n cmp-android/src/prod/AndroidManifest.xmlRepository: openMF/mobile-wallet
Length of output: 1389
Critical conflict: Prod manifest enables AD_ID collection but permission is removed.
The main manifest explicitly removes the AD_ID permission (lines 25-27) with a clear comment stating that Firebase adds it automatically but the app doesn't use it to avoid Google Play rejection. It also disables google_analytics_adid_collection_enabled (line 66).
However, the prod variant overrides this and enables google_analytics_adid_collection_enabled="true" (lines 16-19), which requires the AD_ID permission that was removed. This will cause runtime failures when the analytics library attempts to collect ad IDs in production builds.
Recommended action: Either set android:value="false" in the prod manifest to align with the main manifest and its documented intent, or remove the AD_ID permission removal from the main manifest and add proper disclosure in the privacy policy if ad ID collection is actually needed.
🤖 Prompt for AI Agents
In @cmp-android/src/prod/AndroidManifest.xml around lines 16 - 19, The prod
manifest enables google_analytics_adid_collection_enabled via the meta-data tag
(android:name="google_analytics_adid_collection_enabled") but the main manifest
intentionally strips the AD_ID permission; to resolve, change the prod manifest
meta-data android:value to "false" to match the main manifest intent (or
alternatively remove the AD_ID removal in the main manifest and add proper
disclosure if you genuinely need ad ID collection); update the meta-data entry
in the prod manifest (google_analytics_adid_collection_enabled) or adjust the
AD_ID permission removal to keep manifest behavior consistent and avoid runtime
failures.
| actual fun sendViaSMS(number: String, message: String) { | ||
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | ||
| val encodedMessage = encode(message) | ||
| val smsUrl = if (number.isNotEmpty()) { | ||
| "sms:$number&body=$encodedMessage" | ||
| } else { | ||
| "sms:&body=$encodedMessage" | ||
| } | ||
| val url = NSURL.URLWithString(smsUrl) | ||
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | ||
| UIApplication.sharedApplication.openURL(url) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect SMS URL format.
Line 88 uses & as the separator between the phone number and the body parameter, but SMS URLs require ? for the query string separator (standard URL format).
🐛 Proposed fix
actual fun sendViaSMS(number: String, message: String) {
fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A")
val encodedMessage = encode(message)
val smsUrl = if (number.isNotEmpty()) {
- "sms:$number&body=$encodedMessage"
+ "sms:$number?body=$encodedMessage"
} else {
- "sms:&body=$encodedMessage"
+ "sms:?body=$encodedMessage"
}
val url = NSURL.URLWithString(smsUrl)
if (url != null && UIApplication.sharedApplication.canOpenURL(url)) {
UIApplication.sharedApplication.openURL(url)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| actual fun sendViaSMS(number: String, message: String) { | |
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | |
| val encodedMessage = encode(message) | |
| val smsUrl = if (number.isNotEmpty()) { | |
| "sms:$number&body=$encodedMessage" | |
| } else { | |
| "sms:&body=$encodedMessage" | |
| } | |
| val url = NSURL.URLWithString(smsUrl) | |
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | |
| UIApplication.sharedApplication.openURL(url) | |
| } | |
| } | |
| actual fun sendViaSMS(number: String, message: String) { | |
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | |
| val encodedMessage = encode(message) | |
| val smsUrl = if (number.isNotEmpty()) { | |
| "sms:$number?body=$encodedMessage" | |
| } else { | |
| "sms:?body=$encodedMessage" | |
| } | |
| val url = NSURL.URLWithString(smsUrl) | |
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | |
| UIApplication.sharedApplication.openURL(url) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt
around lines 84 - 96, The SMS URL in sendViaSMS is using "&" between the
recipient and query string; change it to use "?" so the body query parameter is
valid: update the smsUrl construction in sendViaSMS (where smsUrl is built) to
use "sms:$number?body=$encodedMessage" when number is present and
"sms:?body=$encodedMessage" when empty, keeping the existing encode function and
the subsequent
NSURL.URLWithString/UIApplication.sharedApplication.canOpenURL/openURL logic
intact.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
settings.gradle.kts (2)
50-51: Pre-existing duplicate module include.Line 51 duplicates the
:core:networkinclude from line 50. While this doesn't break the build, it should be cleaned up to avoid confusion.Suggested fix
include(":core:network") -include(":core:network")
76-77: Pre-existing duplicate module include.Line 77 duplicates the
:feature:invoicesinclude from line 76.Suggested fix
include(":feature:invoices") -include(":feature:invoices")cmp-android/dependencies/prodReleaseRuntimeClasspath.txt (1)
337-346: Align FileKit artifact versions — all modules must use the same version number.FileKit publishes all modules (core, dialogs, coil, compose) with matching version numbers and they are designed to be used together. The current dependencies mix versions:
filekit-compose*at0.8.8filekit-coil*,filekit-core*,filekit-dialogs*at0.10.0-beta04Ensure all FileKit artifacts in the dependency tree use the same version. Additionally, avoid beta versions (
0.10.0-beta04) in production runtime classpaths; use a stable release version for all FileKit modules.
🤖 Fix all issues with AI agents
In `@cmp-android/dependencies/prodReleaseRuntimeClasspath.txt`:
- Around line 312-315: Replace the RC artifacts with the stable 3.2.0 release:
update the entries for io.coil-kt.coil3:coil-compose-android and
io.coil-kt.coil3:coil-compose from 3.2.0-rc02 to 3.2.0 so all Coil artifacts
consistently use the stable 3.2.0 version in the runtime classpath.
In `@cmp-navigation/src/commonMain/composeResources/values/strings.xml`:
- Line 12: Replace the placeholder string value for the resource named
"app_name" in strings.xml (the <string name="app_name"> entry) with the real
application name used by this project (e.g., "MifosPay" or "Mobile Wallet"),
ensuring the displayed app title/branding throughout the UI is correct.
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigation.kt`:
- Around line 1-35: Implement the missing composable
AuthenticatedNavbarNavigationScreen in this package: add a new Kotlin file
(e.g., AuthenticatedNavbarNavigationScreen.kt) and define a `@Composable` function
named AuthenticatedNavbarNavigationScreen that accepts the same parameter used
here (navigateToSettingsScreen: () -> Unit) and composes the authenticated
navbar UI; ensure it is accessible from AuthenticatedNavbarNavigation.kt
(internal or public as needed) and matches the call signature used in
authenticatedNavbarGraph and
composableWithStayTransitions<AuthenticatedNavbarRoute>.
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt`:
- Around line 15-21: Fix the Spotless import-order violation in
RootNavViewModel.kt by reordering the imports to match the project's
Kotlin/Spotless rules (group standard library, third-party, then internal
packages) or simply run the formatter: execute ./gradlew
:cmp-navigation:spotlessApply to automatically correct the import ordering in
the imports block (the lines importing kotlinx.coroutines.flow.*,
org.mifospay.core.*, template.core.base.ui.BaseViewModel).
- Around line 83-91: The nested sealed class hierarchy is broken because
Internal does not extend RootNavAction while its member UserStateUpdateReceive
directly extends RootNavAction; fix this by making Internal inherit from
RootNavAction (change the declaration to sealed class Internal :
RootNavAction()) so UserStateUpdateReceive can remain as-is, or alternatively
make UserStateUpdateReceive extend Internal (data class
UserStateUpdateReceive(...) : Internal()); pick one consistent approach so the
sealed hierarchy is correct.
- Around line 29-37: Remove the hardcoded
AuthState.Authenticated("sample-token") in the init mapping of
userDataRepository.userData; instead obtain the real auth state from the
appropriate source (e.g., an AuthRepository or from userDataRepository if it
exposes authentication info) and pass that into UserStateUpdateReceive, or if
authState is unused by handleUserStateUpdateReceive/handleAction then remove the
authState field from UserStateUpdateReceive entirely; update the mapping in init
(the lambda that builds UserStateUpdateReceive), references to
UserStateUpdateReceive, and any call sites of handleAction to use the real auth
state or the trimmed payload.
In `@cmp-shared/src/commonMain/kotlin/cmp/shared/utils/KoinExt.kt`:
- Around line 21-26: The file imports the wrong package for KoinModules causing
a compile error; update the import to reference the actual object location and
keep the initKoin implementation as-is: replace the incorrect import of
cmp.navigation.di.KoinModules with org.mifospay.shared.di.KoinModules so that
the initKoin function can call KoinModules.allModules successfully.
In `@core/model/src/commonMain/kotlin/org/mifospay/core/model/AuthState.kt`:
- Around line 27-30: Authenticated is a data class whose auto-generated
toString() will include the sensitive accessToken; override toString() on
Authenticated to return a masked representation (e.g., show only last 4 chars or
a fixed placeholder) instead of the raw token to prevent accidental logging
exposure, or convert Authenticated to a regular class if you need to avoid
data-class auto-generated toString/copy behavior; update any call sites if you
change the class type.
In
`@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationBarItem.kt`:
- Around line 60-61: KptNavigationBarItem currently hardcodes the
NavigationBarItem parameter selected = false, preventing selection styling;
update the call in KptNavigationBarItem to pass selected = isSelected (same as
KptNavigationRailItem) so the component reflects the selected state, ensuring
alwaysShowLabel remains tied to isSelected as well.
In
`@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.kt`:
- Around line 29-48: The KDoc for rememberKptPullToRefreshState is missing
documentation for the isEnabled parameter; update the function KDoc to add a
line describing isEnabled (e.g., whether pull-to-refresh is enabled, default
false) so all parameters (isEnabled, isRefreshing, onRefresh) are documented;
locate the function rememberKptPullToRefreshState and add the isEnabled
description in its KDoc block to match the data class docs.
- Around line 15-27: The KDoc for the KptPullToRefreshState data class is
missing documentation for the isEnabled parameter; update the class KDoc to add
a brief description for isEnabled (e.g., whether pull-to-refresh is
enabled/allowed) alongside the existing entries for isRefreshing and onRefresh
so all constructor parameters are documented.
In `@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.kt`:
- Line 221: Variant C is passing contentWindowInsets into KptScaffold/Scaffold
and then also applying windowInsetsPadding to the same content, causing double
insets; update KptScaffold so that when using contentWindowInsets it does not
reapply windowInsetsPadding (make Scaffold receive WindowInsets(0.dp) like
Variants A/B or remove the extra windowInsetsPadding call), adjust references
around contentWindowInsets and the windowInsetsPadding invocation in
KptScaffold/Scaffold to ensure only one application of insets (keep unique
symbols: KptScaffold, contentWindowInsets, windowInsetsPadding, Scaffold).
- Around line 213-217: The Variant C implementation of KptScaffold wraps the
floatingActionButton with Modifier.navigationBarsPadding(), but Variants A and B
do not, causing inconsistent FAB positioning; update the other overloads of
KptScaffold (the variants that accept floatingActionButton) to apply the same
Modifier.navigationBarsPadding() to their floatingActionButton Box (or
alternatively remove it from Variant C if consistent removal is desired) so all
floatingActionButton usages use the same padding behavior; look for the
floatingActionButton lambdas in KptScaffold and ensure they consistently wrap
the content with Box(modifier = Modifier.navigationBarsPadding()) or remove that
wrapper from all to match the chosen consistent behavior.
🧹 Nitpick comments (15)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.kt (4)
51-54: Remove or document commented-out code.The commented-out
.union(WindowInsets.displayCutout)appears in all three variants. If this is intentional, add a comment explaining why it's disabled. Otherwise, remove the dead code.Also applies to: 125-128, 202-205
81-110: Consider extracting duplicated pull-to-refresh content logic.The content block implementation (lines 81-110, 156-185, 222-249) is duplicated across all three variants. Extract this into a private helper composable to improve maintainability.
♻️ Suggested extraction
`@OptIn`(ExperimentalMaterial3Api::class) `@Composable` private fun KptScaffoldContent( paddingValues: PaddingValues, contentWindowInsets: WindowInsets, pullToRefreshState: KptPullToRefreshState, content: `@Composable` () -> Unit, ) { val internalPullToRefreshState = rememberPullToRefreshState() Box( modifier = Modifier .padding(paddingValues = paddingValues) .consumeWindowInsets(paddingValues = paddingValues) .imePadding() .navigationBarsPadding(), ) { Box( modifier = Modifier .windowInsetsPadding(insets = contentWindowInsets) .pullToRefresh( state = internalPullToRefreshState, isRefreshing = pullToRefreshState.isRefreshing, onRefresh = pullToRefreshState.onRefresh, enabled = pullToRefreshState.isEnabled, ), ) { content() PullToRefreshDefaults.Indicator( modifier = Modifier.align(Alignment.TopCenter), isRefreshing = pullToRefreshState.isRefreshing, state = internalPullToRefreshState, ) } } }Also applies to: 156-185
55-55: API inconsistency in snackbar handling.Variants A/B accept
snackbarHost:@composable() -> Unitwhile Variant C acceptssnackbarHostState: SnackbarHostState. Consider aligning the APIs for a more consistent developer experience, or document the intentional difference.Also applies to: 129-129, 197-197
254-258: Consider addingcontainerColortoFloatingActionButtonContent.The data class exposes
contentColorbut notcontainerColor, limiting FAB background customization. Consider adding it for fuller customization support.♻️ Suggested enhancement
data class FloatingActionButtonContent( val onClick: (() -> Unit), val contentColor: Color, + val containerColor: Color = Color.Unspecified, val content: (`@Composable` () -> Unit), )Then update the FloatingActionButton usages:
FloatingActionButton( onClick = content.onClick, contentColor = content.contentColor, + containerColor = content.containerColor.takeIf { it != Color.Unspecified } + ?: FloatingActionButtonDefaults.containerColor, content = content.content, )cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationViewModel.kt (1)
42-47: Empty handler forUserStateUpdateReceiveaction.The
UserStateUpdateReceiveaction handler has an empty body, which appears to be a placeholder. Consider either implementing the intended behavior or adding a TODO comment to document the pending work.📝 Suggested: Add TODO comment for clarity
private fun handleInternalAction(action: AuthenticatedNavBarAction.Internal) { when (action) { is AuthenticatedNavBarAction.Internal.UserStateUpdateReceive -> { + // TODO: Implement user state update handling } } }Would you like me to help implement user state handling logic or open an issue to track this?
cmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashScreen.kt (1)
19-28: Hardcoded color and modifier placement concerns.Two issues to consider:
Color.Whiteis hardcoded, which may cause issues with dark theme support. Consider usingMaterialTheme.colorScheme.surfaceorMaterialTheme.colorScheme.backgroundinstead.The
modifierparameter is applied to the innerBoxrather than the outermostSurface. Conventionally, the modifier should be applied to the root composable for proper sizing/positioning by parent layouts.♻️ Suggested fix
`@Composable` fun SplashScreen( modifier: Modifier = Modifier, ) { Surface( - color = Color.White, + modifier = modifier, + color = MaterialTheme.colorScheme.surface, ) { - Box(modifier = modifier.fillMaxSize()) + Box(modifier = Modifier.fillMaxSize()) } }You'll need to add the import:
import androidx.compose.material3.MaterialThemecore/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRail.kt (1)
40-44: HardcodedColor.Whitedoes not support dark theme.Using a hardcoded color prevents proper dark mode support. Consider using a theme-aware color from
KptTheme.colorScheme(e.g.,surfaceorbackground) for consistency with the design system used elsewhere in this file.♻️ Suggested fix
Surface( - color = Color.White, + color = KptTheme.colorScheme.surface, contentColor = Color.Unspecified, modifier = modifier, ) {cmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/KptRootScaffold.kt (1)
111-125: Remove or address commented-out code.The commented-out block for consuming display cutout insets appears to be work-in-progress. Either remove it or add a TODO comment explaining why it's disabled and when it should be re-enabled.
♻️ Suggested cleanup
Box( - modifier = Modifier.run { -// if (isNavigationRailVisible) { -// consumeWindowInsets( -// insets = WindowInsets.displayCutout.only(WindowInsetsSides.Start), -// ) -// } else - if (isNavigationBarVisible) { + modifier = Modifier.run { + // TODO: Handle display cutout insets for navigation rail layout + if (isNavigationBarVisible) { consumeWindowInsets( insets = WindowInsets.navigationBars.only(WindowInsetsSides.Bottom), ) } else { this } }, )cmp-navigation/src/androidMain/kotlin/cmp/navigation/rootnav/RootNavScreen.android.kt (1)
15-18: Side effect executed directly during composition.The
clearFocus()call runs directly during composition rather than as a controlled side effect. This can cause the effect to execute multiple times on recomposition and may run before the UI is fully settled.Consider wrapping in
SideEffectto ensure it runs after successful composition:♻️ Suggested refactor
`@Composable` actual fun ClearFocus() { - LocalActivity.current?.currentFocus?.clearFocus() + val activity = LocalActivity.current + SideEffect { + activity?.currentFocus?.clearFocus() + } }This requires adding the import:
import androidx.compose.runtime.SideEffectcore/data/src/desktopMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.kt (1)
19-19: Consider renaming the class toDesktopPlatformDependentDataModule.This class resides in the
desktopMainsource set but is namedJsPlatformDependentDataModule. This naming inconsistency could cause confusion when navigating the codebase.cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.kt (1)
96-98:ClearFocus()called outside of a side-effect scope.
ClearFocus()is a@Composablefunction called directly in the composition body. This will execute on every recomposition where the early return (lines 88-93) doesn't trigger. If state changes frequently, focus will be cleared repeatedly.Consider wrapping this in a
LaunchedEffectkeyed onstateortargetRouteto ensure it only runs when navigation actually occurs, not on every recomposition.Suggested approach
- // In some scenarios on an emulator the Activity can leak when recreated - // if we don't first clear focus anytime we change the root destination. - ClearFocus() - // When state changes, navigate to different root navigation state val rootNavOptions = navOptions { // ... } LaunchedEffect(state) { + // Clear focus before navigating to prevent Activity leaks on emulator when (state) {Then trigger focus clearing within the
LaunchedEffectbefore navigation, or use a separateLaunchedEffectthat tracks state changes.cmp-navigation/src/commonMain/kotlin/cmp/navigation/utils/RootUtils.kt (1)
19-27: Document@Serializablerequirement and InternalSerializationApi usage.Both functions use
InternalSerializationApi, which has no stability guarantees and may change or be removed in future kotlinx.serialization versions. Additionally,serializer()throwsSerializationExceptionif the class is not annotated with@Serializable. Update the KDoc to document these requirements and limitations.📝 Suggested documentation improvement
/** * Gets the route string for an object. + * + * `@throws` kotlinx.serialization.SerializationException if the object's class is not annotated with `@Serializable` */ `@OptIn`(InternalSerializationApi::class) fun <T : Any> T.toObjectNavigationRoute(): String = this::class.toObjectKClassNavigationRoute() /** * Gets the route string for a [KClass] of an object. + * + * Note: Uses internal serialization APIs with no stability guarantees. + * + * `@throws` kotlinx.serialization.SerializationException if the class is not annotated with `@Serializable` */ `@OptIn`(InternalSerializationApi::class) fun <T : Any> KClass<T>.toObjectKClassNavigationRoute(): String = this.serializer().descriptor.serialNamecmp-shared/src/commonMain/kotlin/cmp/shared/utils/KoinExt.kt (1)
17-19: Consider adding explicit return type for public API clarity.Since
koinConfiguration()is a public function, adding an explicit return type improves API discoverability and documentation:-fun koinConfiguration() = koinApplication { +fun koinConfiguration(): KoinApplication = koinApplication { modules(KoinModules.allModules) }This would require adding
import org.koin.core.KoinApplicationto the imports.gradle/libs.versions.toml (2)
42-42: Consider updating Jackson to a newer patch version."Branch is closed since it is not a Long-Term-Support (LTS) version: no more patch releases planned" and importantly "2.19.3 (BROKEN, do not use) 2.19.4 (29-Oct-2025)". Version
2.19.2is valid but2.19.4fixes issues in the 2.19.x line. Alternatively, "Current version 2.20.1" is the latest stable.Suggested update
-jacksonCore = "2.19.2" +jacksonCore = "2.19.4"Or for latest stable:
-jacksonCore = "2.19.2" +jacksonCore = "2.20.1"
46-46: Dependency versions verified and available, but consider updating GitHub API and Commons Codec to latest releases.All three versions are confirmed in Maven Central and stable:
- OkHttp
4.12.0(Oct 2023) ✓- GitHub API
1.329✓ — newer versions available:1.330,2.0-rc.5- Commons Codec
1.19.0✓ — newer version available:1.20.0(Oct 2025)Consider upgrading GitHub API to
1.330and Commons Codec to1.20.0for the latest bug fixes and features.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtcmp-android/src/main/kotlin/org/mifospay/MainActivity.ktcmp-navigation/build.gradle.ktscmp-navigation/src/androidMain/kotlin/cmp/navigation/rootnav/RootNavScreen.android.ktcmp-navigation/src/commonMain/composeResources/values/strings.xmlcmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigation.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationViewModel.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavNavigation.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashNavigation.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashScreen.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/KptRootScaffold.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/ScaffoldNavigationData.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/utils/RootUtils.ktcmp-navigation/src/nonAndroidMain/kotlin/cmp/navigation/rootnav/RootNavScreen.nonAndroid.ktcmp-shared/build.gradle.ktscmp-shared/src/commonMain/kotlin/cmp/shared/SharedApp.ktcmp-shared/src/commonMain/kotlin/cmp/shared/utils/KoinExt.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosAppState.ktcmp-shared/src/nativeMain/kotlin/org/mifos/shared/ViewController.ktcore/data/src/androidMain/kotlin/org/mifospay/core/data/di/AndroidPlatformDependentDataModule.ktcore/data/src/androidMain/kotlin/org/mifospay/core/data/di/AndroidPlatformModule.ktcore/data/src/androidMain/kotlin/org/mifospay/core/data/util/ConnectivityManagerNetworkMonitor.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/di/PlatformDependentDataModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/NetworkMonitor.ktcore/data/src/desktopMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.ktcore/data/src/jsMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.ktcore/data/src/nativeMain/kotlin/org/mifospay/core/data/NativePlatformDependentDataModule.ktcore/data/src/wasmJsMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.ktcore/model/src/commonMain/kotlin/org/mifospay/core/model/AuthState.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/NavigationItem.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptBottomBar.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationBarItem.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRail.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRailItem.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.ktgradle/libs.versions.tomlsettings.gradle.kts
✅ Files skipped from review due to trivial changes (2)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt
- core/data/src/wasmJsMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.kt
🧰 Additional context used
🧬 Code graph analysis (9)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptBottomBar.kt (1)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationBarItem.kt (1)
KptNavigationBarItem(31-70)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashNavigation.kt (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashScreen.kt (1)
SplashScreen(19-28)
cmp-shared/src/commonMain/kotlin/cmp/shared/SharedApp.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/ImageLoaderExt.kt (1)
LocalImageLoaderProvider(91-96)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.kt (1)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.kt (1)
rememberKptPullToRefreshState(37-48)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/KptRootScaffold.kt (3)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.kt (1)
rememberKptPullToRefreshState(37-48)core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptBottomBar.kt (1)
KptBottomBar(22-46)core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRail.kt (1)
KptNavigationRail(32-71)
cmp-shared/src/nativeMain/kotlin/org/mifos/shared/ViewController.kt (2)
cmp-shared/src/commonMain/kotlin/cmp/shared/utils/KoinExt.kt (1)
initKoin(21-26)cmp-shared/src/commonMain/kotlin/cmp/shared/SharedApp.kt (1)
SharedApp(21-42)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigation.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/NavGraphBuilderExtensions.kt (1)
composableWithStayTransitions(45-60)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRail.kt (1)
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRailItem.kt (1)
KptNavigationRailItem(29-65)
🪛 GitHub Actions: PR Checks
cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt
[error] 15-22: spotlessKotlinCheck failed: format violations detected in RootNavViewModel.kt. The diff shows an import order issue (added import 'org.mifospay.core.data.repository.UserDataRepository'). Run './gradlew :cmp-navigation:spotlessApply' to fix formatting.
🪛 LanguageTool
cmp-android/dependencies/prodReleaseRuntimeClasspath.txt
[uncategorized] ~337-~337: The official name of this software platform is spelled with a capital “H”.
Context: ...io.github.alexzhirkevich:qrose:1.0.1 io.github.vinceglb:filekit-coil-android:0.10.0-be...
(GITHUB)
[uncategorized] ~338-~338: The official name of this software platform is spelled with a capital “H”.
Context: ...b:filekit-coil-android:0.10.0-beta04 io.github.vinceglb:filekit-coil:0.10.0-beta04 io....
(GITHUB)
[uncategorized] ~339-~339: The official name of this software platform is spelled with a capital “H”.
Context: ....vinceglb:filekit-coil:0.10.0-beta04 io.github.vinceglb:filekit-compose-android:0.8.8 ...
(GITHUB)
[uncategorized] ~340-~340: The official name of this software platform is spelled with a capital “H”.
Context: ...nceglb:filekit-compose-android:0.8.8 io.github.vinceglb:filekit-compose:0.8.8 io.githu...
(GITHUB)
🔇 Additional comments (45)
core/data/src/nativeMain/kotlin/org/mifospay/core/data/NativePlatformDependentDataModule.kt (1)
16-16: LGTM!The import path update correctly aligns with the
NetworkMonitorpackage reorganization fromdata.utiltorepository. This is consistent with the broader refactoring mentioned in the PR.cmp-navigation/src/commonMain/composeResources/values/strings.xml (1)
13-16: LGTM!The navigation labels and connectivity message are appropriate for the module's purpose.
core/model/src/commonMain/kotlin/org/mifospay/core/model/AuthState.kt (1)
15-25: Sealed class structure looks good.The use of a sealed class for modeling authentication state is idiomatic Kotlin and enables exhaustive
whenexpressions. Usingdata objectfor the singleton states (Uninitialized, Unauthenticated) is the correct modern Kotlin approach.cmp-shared/src/nativeMain/kotlin/org/mifos/shared/ViewController.kt (1)
16-28: LGTM - correct iOS entry point structure.The use of the
configureblock for Koin initialization ensures DI is set up before composition begins. The no-op callbacks are appropriate placeholders for native iOS, where these platform-specific behaviors may need native implementations later.Consider adding a brief comment noting that the empty lambdas are intentional placeholders to help future contributors understand the design intent.
cmp-shared/src/commonMain/kotlin/cmp/shared/SharedApp.kt (1)
30-41: The functiongetDefaultImageLoaderdoes not exist in the codebase.The code imports and uses
getDefaultImageLoader(LocalPlatformContext.current), but this function is not defined anywhere in the repository. Similar functions exist intemplate.core.base.ui.ImageLoaderExtthat already handle memoization:
rememberImageLoader(context)— returns cached loader or creates a default onerememberDefaultImageLoader(context)— creates and memoizes an ImageLoader withremember(context)Replace
getDefaultImageLoader(LocalPlatformContext.current)withrememberImageLoader(LocalPlatformContext.current)orrememberDefaultImageLoader(LocalPlatformContext.current).Likely an incorrect or invalid review comment.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationViewModel.kt (2)
20-32: Well-structured ViewModel with proper flow management.The
isOfflineStateFlow correctly:
- Derives from
networkMonitor.isOnlinewith negation- Uses
WhileSubscribed(5_000)to allow brief survival during configuration changes- Provides sensible initial value
Using
Unitas state type is acceptable here since the only exposed state isisOffline.
58-79: Clean action/event sealed class hierarchy.The sealed class structure properly separates:
- External actions (
HomeTabClick,SettingsTabClick) from internal actions- Events with associated tab mapping via abstract
tabpropertyMinor observation:
SettingsTabClicktriggersNavigateToProfileScreen— verify this naming alignment is intentional (settings navigation leading to profile screen).core/data/src/androidMain/kotlin/org/mifospay/core/data/util/ConnectivityManagerNetworkMonitor.kt (1)
26-26: LGTM!The import update correctly aligns with the
NetworkMonitorinterface relocation to therepositorypackage. The implementation remains unchanged and properly handles network connectivity monitoring.settings.gradle.kts (1)
42-42: LGTM!The new
cmp-navigationmodule is correctly included alongside othercmp-*modules, following the established project structure.core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/NetworkMonitor.kt (1)
10-18: LGTM!The
NetworkMonitorinterface relocation to therepositorypackage is a sensible organizational improvement. Placing this interface alongside other repository contracts improves discoverability and aligns with common architecture patterns where monitors/observers sit at the repository layer.core/data/src/commonMain/kotlin/org/mifospay/core/data/di/PlatformDependentDataModule.kt (1)
13-14: LGTM!The import update correctly references
NetworkMonitorfrom its newrepositorypackage location whileTimeZoneMonitorappropriately remains in theutilpackage.core/data/src/androidMain/kotlin/org/mifospay/core/data/di/AndroidPlatformDependentDataModule.kt (1)
15-16: LGTM!The import correctly references the
NetworkMonitorinterface from its newrepositorypackage while the concrete implementationConnectivityManagerNetworkMonitorappropriately remains in theutilpackage. This separation of interface and implementation is a clean architectural pattern.cmp-navigation/build.gradle.kts (1)
1-55: LGTM!Build configuration looks appropriate for a multiplatform navigation module. The plugin aliases, dependencies, and compose resource configuration are well-structured.
Minor note: The copyright header uses 2024 while other new files in this PR (like SplashScreen.kt) use 2025. Consider updating for consistency if this is a newly created file.
cmp-shared/build.gradle.kts (1)
27-27: LGTM!The addition of
cmpNavigationas animplementationdependency is appropriate. Usingimplementationscope correctly keeps navigation internals encapsulated within cmp-shared without leaking to consumers.cmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/ScaffoldNavigationData.kt (1)
15-20: LGTM!Clean data class design. Using
ImmutableListfrom kotlinx-collections-immutable is a good choice for Compose stability, and makingselectedNavigationItemnullable appropriately handles the case when no item is selected.core/ui/src/commonMain/kotlin/org/mifospay/core/ui/NavigationItem.kt (1)
15-53: LGTM!Well-designed interface with comprehensive documentation. The contract covers all necessary aspects for navigation items: icons (selected/unselected states), localized strings, routing information, and test tags. Using
StringResourcefrom Jetbrains Compose Resources ensures proper multiplatform localization support.cmp-navigation/src/commonMain/kotlin/cmp/navigation/splash/SplashNavigation.kt (1)
20-31: LGTM!The splash navigation setup follows standard Navigation Compose patterns with type-safe routes. The
@Serializableannotation onSplashRouteenables type-safe navigation, and the extension functions provide a clean API for registering and navigating to the splash screen.core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRailItem.kt (1)
29-65: LGTM!The navigation rail item implementation correctly handles icon selection, label rendering with a minimal indicator pill, and selection state via
alwaysShowLabel. The use of aSpaceras a styled label indicator is an intentional design choice for a minimal visual appearance.core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationRail.kt (1)
59-68: LGTM!The item rendering correctly applies selection state, click handling, and test tags for each navigation item.
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationBarItem.kt (1)
31-47: LGTM on the icon and label rendering.The icon selection logic and label indicator styling are correct and consistent with the navigation rail item pattern.
core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptBottomBar.kt (1)
22-46: LGTM!The
KptBottomBarimplementation correctly orchestrates the navigation items, passes selection state, and applies test tags. ThetonalElevation = 0.dpprovides a flat appearance consistent with modern design patterns.Note: The selection state will work correctly once the
selected = falsebug inKptNavigationBarItemis fixed.cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt (1)
45-66: Verify the state transition logic for empty passcode.Line 56 transitions to
UserLockedwhenpasscode.isEmpty(), but this may not match intended behavior. If the user is authenticated but hasn't set a passcode yet, should they be locked out, or should they be prompted to create a passcode?Also note the TODO at line 50 indicates this logic needs further configuration.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavNavigation.kt (1)
16-28: LGTM!Clean implementation following standard Jetpack Navigation Compose patterns. The
@Serializableannotation enables type-safe navigation, and the extension function properly delegates toRootNavScreen.cmp-navigation/src/commonMain/kotlin/cmp/navigation/ui/KptRootScaffold.kt (2)
54-156: Well-structured adaptive scaffold implementation.The responsive layout logic correctly switches between navigation rail (wider screens) and bottom navigation bar (compact screens). The pull-to-refresh integration properly bridges
KptPullToRefreshStatewith Material3's internal state, and the animated visibility transitions provide smooth UX.
158-197: LGTM on helper composables.Both
ScaffoldBottomAppBarandScaffoldNavigationRailcorrectly delegate to the sharedKptBottomBarandKptNavigationRailcomponents. The z-index handling on the rail ensures proper layering during content transitions.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt (1)
17-17: Import path update is correct and consistent.The
NetworkMonitorimport has been correctly relocated fromutiltorepositorypackage. All 17 references across the codebase consistently use the new import path, and no old references remain.core/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.kt (1)
26-26: LGTM!The import path update and new DI binding for
NetworkMonitorare consistent with the package reorganization. The binding pattern mirrors the existingTimeZoneMonitorbinding on line 103.Also applies to: 102-102
cmp-android/src/main/kotlin/org/mifospay/MainActivity.kt (1)
31-31: LGTM!The import path update aligns with the
NetworkMonitorrelocation to the repository package. Usage remains unchanged.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosAppState.kt (1)
32-32: LGTM!The import path update is consistent with the
NetworkMonitorpackage relocation across the codebase.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
23-23: LGTM!The import path update is consistent with the
NetworkMonitorpackage relocation.cmp-navigation/src/nonAndroidMain/kotlin/cmp/navigation/rootnav/RootNavScreen.nonAndroid.kt (1)
14-16: LGTM!The no-op
actualimplementation for non-Android platforms is appropriate. Android requires explicit focus clearing to prevent activity leaks in certain scenarios, while other platforms don't have this concern.core/data/src/jsMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.kt (1)
16-16: LGTM!Import path correctly updated to reflect the
NetworkMonitorrelocation to therepositorypackage.core/data/src/androidMain/kotlin/org/mifospay/core/data/di/AndroidPlatformModule.kt (1)
17-17: LGTM!Import path correctly updated to align with the
NetworkMonitorpackage relocation.core/data/src/desktopMain/kotlin/org/mifospay/core/data/JsPlatformDependentDataModule.kt (1)
16-16: Import update looks correct.The import path change aligns with the
NetworkMonitorpackage relocation.cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.kt (4)
72-81: Incomplete state-to-route mapping returns empty strings.
ShowOnboarding,Auth, andUserLockedstates return empty strings, which will cause the route comparison at line 88 to behave unexpectedly if these states are ever reached. Combined with the no-op navigation blocks in theLaunchedEffect(lines 118-122), this could leave the app in an inconsistent navigation state.If these are intentional placeholders, consider adding a
TODOcomment or throwingNotImplementedErrorto make the incomplete status explicit and prevent silent failures.
56-70: Navigation graph structure looks good.The
NavHostsetup with transition providers and the split betweensplashDestinationandauthenticatedGraphis well-structured. The commented-out destinations (onboarding, auth, userUnlock) are clearly placeholders aligned with the PR's incremental approach.
130-150: Helper functions for route detection and transitions are well-implemented.The recursive
rootLevelRoute()traversal and transition provider selection based on route type provide clean abstractions for the navigation logic.
36-37: Uses experimental Kotlin Atomics API.
ExperimentalAtomicApiis used forAtomicReference. This API remains experimental and may change in future Kotlin releases. The code correctly includes the opt-in annotation.gradle/libs.versions.toml (4)
206-206: New Room Gradle plugin and Commons Codec library entries are well-formed.The library declarations follow the correct TOML format.
androidx-room-gradle-pluginandcommons-codecare properly defined with module coordinates and version references.Also applies to: 209-209
242-250: Library declarations are well-structured.The artifact coordinates and version references are correctly specified:
- Jackson modules correctly share the
jacksonCoreversion reference- JUnit Jupiter modules share the
junitJupiterApiversion reference- All group:artifact coordinates match Maven Central conventions
Note: Fix the JUnit version numbers as flagged above before these libraries can be resolved.
380-380: Kermit-Koin integration library added correctly.The
kermit-koinlibrary provides Koin dependency injection integration for Kermit logging, appropriately sharing thekermitversion reference with other Kermit libraries.
44-45: The versions specified are valid and exist on Maven Central. JUnit Jupiter API 5.13.4 and JUnit Platform Launcher 1.13.4 are legitimate releases. The release history includes consecutive 5.x versions (5.11.x, 5.12.x, 5.13.x, 5.14.x), not the gap claimed in the original review.If a version upgrade is desired, 5.14.2 or 6.0.2 are available as newer alternatives, but the current versions are not invalid.
Likely an incorrect or invalid review comment.
cmp-android/dependencies/prodReleaseRuntimeClasspath.txt (3)
1-8: LGTM: Internal module additions align with PR objectives.The new core-base modules (:cmp-navigation, :core-base:common, :core-base:datastore, :core-base:platform, :core-base:ui) align with the stated goal of adding missing template modules.
1-478: Note: This is a generated dependency file.This file (
prodReleaseRuntimeClasspath.txt) is auto-generated by Gradle. Any dependency changes should be made in the appropriate build configuration files (e.g.,build.gradle.kts, version catalogs, or convention plugins), not directly in this file.The static analysis hints about "GitHub" capitalization are false positives—these are Maven coordinates where lowercase is correct.
251-255: Verify security advisories for newly added dependencies.Ensure all newly added dependencies are checked for known security vulnerabilities, especially:
- Google Play libraries (app-update, review)
- Calf permissions
- JetBrains Compose material
- FileKit ecosystem libraries
Run the following script to check for security advisories:
#!/bin/bash # Check security advisories for key new dependencies PACKAGES=( "com.google.android.play:app-update" "com.google.android.play:review" "com.mohamedrejeb.calf:calf-permissions" "io.github.vinceglb:filekit-core" "org.jetbrains.compose.material:material" ) for package in "${PACKAGES[@]}"; do GROUP=$(echo "$package" | cut -d: -f1) ARTIFACT=$(echo "$package" | cut -d: -f2) echo "Checking $GROUP:$ARTIFACT..." # Note: GitHub's advisory database primarily covers npm, pip, rubygems # For Maven dependencies, consider using OWASP Dependency-Check or Snyk echo "Manual review recommended for Maven package: $GROUP:$ARTIFACT" echo "---" done echo "" echo "Recommendation: Run OWASP Dependency-Check or Snyk for comprehensive security scanning:" echo "./gradlew dependencyCheckAnalyze"Also applies to: 288-289, 434-434
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| io.coil-kt.coil3:coil-compose-android:3.2.0-rc02 | ||
| io.coil-kt.coil3:coil-compose-core-android:3.2.0 | ||
| io.coil-kt.coil3:coil-compose-core:3.2.0 | ||
| io.coil-kt.coil3:coil-compose:3.2.0-rc02 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmp-android/dependencies/prodReleaseRuntimeClasspath.txt | sed -n '310,320p'Repository: openMF/mobile-wallet
Length of output: 607
🏁 Script executed:
#!/bin/bash
# Check Maven Central for available versions of coil-compose packages
curl -s "https://search.maven.org/solrsearch/select?q=g:io.coil-kt.coil3+AND+a:coil-compose-android&rows=50&wt=json" | \
jq -r '.response.docs[] | .v' | sort -V
echo "---"
curl -s "https://search.maven.org/solrsearch/select?q=g:io.coil-kt.coil3+AND+a:coil-compose-core-android&rows=50&wt=json" | \
jq -r '.response.docs[] | .v' | sort -V
echo "---"
curl -s "https://search.maven.org/solrsearch/select?q=g:io.coil-kt.coil3+AND+a:coil-compose-core&rows=50&wt=json" | \
jq -r '.response.docs[] | .v' | sort -V
echo "---"
curl -s "https://search.maven.org/solrsearch/select?q=g:io.coil-kt.coil3+AND+a:coil-compose&rows=50&wt=json" | \
jq -r '.response.docs[] | .v' | sort -VRepository: openMF/mobile-wallet
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Try alternative Maven Central search for coil-compose packages
for package in "coil-compose-android" "coil-compose"; do
echo "Searching for: $package"
curl -s "https://search.maven.org/solrsearch/select?q=g:%22io.coil-kt.coil3%22+AND+a:%22${package}%22&core=gav&rows=100&wt=json" | \
jq -r '.response.docs[] | .v' | sort -V | tail -5
echo "---"
doneRepository: openMF/mobile-wallet
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Alternative approach: use Maven repo API
echo "coil-compose-android versions:"
curl -s "https://repo1.maven.org/maven2/io/coil-kt/coil3/coil-compose-android/maven-metadata.xml" | grep -oP '<version>\K[^<]+' | tail -10
echo ""
echo "coil-compose versions:"
curl -s "https://repo1.maven.org/maven2/io/coil-kt/coil3/coil-compose/maven-metadata.xml" | grep -oP '<version>\K[^<]+' | tail -10Repository: openMF/mobile-wallet
Length of output: 258
Use stable versions instead of RC versions in production.
The coil-compose and coil-compose-android dependencies are using RC version 3.2.0-rc02, while other Coil libraries use the stable 3.2.0 version. Release candidates should not be used in production runtime classpaths as they may contain undiscovered bugs or undergo API changes before the stable release. Stable version 3.2.0 is available for both packages.
🤖 Prompt for AI Agents
In `@cmp-android/dependencies/prodReleaseRuntimeClasspath.txt` around lines 312 -
315, Replace the RC artifacts with the stable 3.2.0 release: update the entries
for io.coil-kt.coil3:coil-compose-android and io.coil-kt.coil3:coil-compose from
3.2.0-rc02 to 3.2.0 so all Coil artifacts consistently use the stable 3.2.0
version in the runtime classpath.
| See https://github.com/openMF/mobile-wallet/blob/master/LICENSE.md | ||
| --> | ||
| <resources> | ||
| <string name="app_name">KPT Design</string> |
There was a problem hiding this comment.
Update template placeholder app_name to the actual application name.
The value "KPT Design" appears to be a leftover from the kmp-project-template. This should be updated to the actual application name (e.g., "MifosPay" or "Mobile Wallet") to avoid displaying incorrect branding in the UI.
Suggested fix
- <string name="app_name">KPT Design</string>
+ <string name="app_name">MifosPay</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="app_name">KPT Design</string> | |
| <string name="app_name">MifosPay</string> |
🤖 Prompt for AI Agents
In `@cmp-navigation/src/commonMain/composeResources/values/strings.xml` at line
12, Replace the placeholder string value for the resource named "app_name" in
strings.xml (the <string name="app_name"> entry) with the real application name
used by this project (e.g., "MifosPay" or "Mobile Wallet"), ensuring the
displayed app title/branding throughout the UI is correct.
| /* | ||
| * Copyright 2025 Mifos Initiative | ||
| * | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| * | ||
| * See https://github.com/openMF/mobile-wallet/blob/master/LICENSE.md | ||
| */ | ||
| @file:Suppress("MatchingDeclarationName") | ||
|
|
||
| package cmp.navigation.authenticatednavbar | ||
|
|
||
| import androidx.navigation.NavController | ||
| import androidx.navigation.NavGraphBuilder | ||
| import androidx.navigation.NavOptions | ||
| import kotlinx.serialization.Serializable | ||
| import template.core.base.ui.composableWithStayTransitions | ||
|
|
||
| @Serializable | ||
| data object AuthenticatedNavbarRoute | ||
|
|
||
| internal fun NavController.navigateToAuthenticatedNavBar(navOptions: NavOptions? = null) { | ||
| navigate(route = AuthenticatedNavbarRoute, navOptions = navOptions) | ||
| } | ||
|
|
||
| internal fun NavGraphBuilder.authenticatedNavbarGraph( | ||
| navigateToSettingsScreen: () -> Unit, | ||
| ) { | ||
| composableWithStayTransitions<AuthenticatedNavbarRoute> { | ||
| AuthenticatedNavbarNavigationScreen( | ||
| navigateToSettingsScreen = navigateToSettingsScreen, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify AuthenticatedNavbarNavigationScreen exists in the authenticatednavbar package
ast-grep --pattern 'fun AuthenticatedNavbarNavigationScreen($$$)'Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for AuthenticatedNavbarNavigationScreen in the codebase using rg
rg "AuthenticatedNavbarNavigationScreen" --type kotlinRepository: openMF/mobile-wallet
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Broader search for AuthenticatedNavbarNavigationScreen
rg "AuthenticatedNavbarNavigationScreen" --no-type -A 2Repository: openMF/mobile-wallet
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Search for AuthenticatedNavbarNavigationScreen definition across the entire codebase
rg "AuthenticatedNavbarNavigationScreen" -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 774
🏁 Script executed:
#!/bin/bash
# List all files in the authenticatednavbar package directory
find cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar -type fRepository: openMF/mobile-wallet
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Read AuthenticatedNavbarNavigationViewModel.kt to check for the composable definition
cat -n cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationViewModel.ktRepository: openMF/mobile-wallet
Length of output: 3274
AuthenticatedNavbarNavigationScreen composable is not defined.
The file references AuthenticatedNavbarNavigationScreen at line 29, but this composable does not exist in the codebase. It needs to be implemented in this package (typically in a separate file like AuthenticatedNavbarNavigationScreen.kt) before the code will compile.
🤖 Prompt for AI Agents
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigation.kt`
around lines 1 - 35, Implement the missing composable
AuthenticatedNavbarNavigationScreen in this package: add a new Kotlin file
(e.g., AuthenticatedNavbarNavigationScreen.kt) and define a `@Composable` function
named AuthenticatedNavbarNavigationScreen that accepts the same parameter used
here (navigateToSettingsScreen: () -> Unit) and composes the authenticated
navbar UI; ensure it is accessible from AuthenticatedNavbarNavigation.kt
(internal or public as needed) and matches the call signature used in
authenticatedNavbarGraph and
composableWithStayTransitions<AuthenticatedNavbarRoute>.
| import kotlinx.coroutines.flow.map | ||
| import kotlinx.coroutines.flow.onEach | ||
| import kotlinx.coroutines.flow.update | ||
| import org.mifospay.core.model.AuthState | ||
| import org.mifospay.core.data.repository.UserDataRepository | ||
| import org.mifospay.core.model.UserData | ||
| import template.core.base.ui.BaseViewModel |
There was a problem hiding this comment.
Fix import order to resolve pipeline failure.
The pipeline is failing due to spotless format violations. Run ./gradlew :cmp-navigation:spotlessApply to fix the import ordering.
🧰 Tools
🪛 GitHub Actions: PR Checks
[error] 15-22: spotlessKotlinCheck failed: format violations detected in RootNavViewModel.kt. The diff shows an import order issue (added import 'org.mifospay.core.data.repository.UserDataRepository'). Run './gradlew :cmp-navigation:spotlessApply' to fix formatting.
🤖 Prompt for AI Agents
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt`
around lines 15 - 21, Fix the Spotless import-order violation in
RootNavViewModel.kt by reordering the imports to match the project's
Kotlin/Spotless rules (group standard library, third-party, then internal
packages) or simply run the formatter: execute ./gradlew
:cmp-navigation:spotlessApply to automatically correct the import ordering in
the imports block (the lines importing kotlinx.coroutines.flow.*,
org.mifospay.core.*, template.core.base.ui.BaseViewModel).
| init { | ||
| userDataRepository.userData.map { userData -> | ||
| UserStateUpdateReceive( | ||
| authState = AuthState.Authenticated("sample-token"), | ||
| userData = userData, | ||
| ) | ||
| }.onEach(::handleAction) | ||
| .launchIn(viewModelScope) | ||
| } |
There was a problem hiding this comment.
Hardcoded authentication token should be removed.
AuthState.Authenticated("sample-token") appears to be placeholder/debug code. This hardcodes authentication state regardless of actual user authentication status, which will bypass proper authentication flow.
The authState should be obtained from the actual authentication source (e.g., from UserDataRepository or a separate AuthRepository).
🐛 Suggested approach
init {
- userDataRepository.userData.map { userData ->
+ userDataRepository.userData.map { (authState, userData) ->
UserStateUpdateReceive(
- authState = AuthState.Authenticated("sample-token"),
+ authState = authState,
userData = userData,
)
}.onEach(::handleAction)
.launchIn(viewModelScope)
}Alternatively, if authState isn't actually used in handleUserStateUpdateReceive, consider removing it from UserStateUpdateReceive entirely.
🤖 Prompt for AI Agents
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt`
around lines 29 - 37, Remove the hardcoded
AuthState.Authenticated("sample-token") in the init mapping of
userDataRepository.userData; instead obtain the real auth state from the
appropriate source (e.g., an AuthRepository or from userDataRepository if it
exposes authentication info) and pass that into UserStateUpdateReceive, or if
authState is unused by handleUserStateUpdateReceive/handleAction then remove the
authState field from UserStateUpdateReceive entirely; update the mapping in init
(the lambda that builds UserStateUpdateReceive), references to
UserStateUpdateReceive, and any call sites of handleAction to use the real auth
state or the trimmed payload.
| selected = false, | ||
| alwaysShowLabel = isSelected, |
There was a problem hiding this comment.
Bug: selected is hardcoded to false instead of using isSelected.
The selected parameter is hardcoded to false, which means the NavigationBarItem will never display its selected state styling. This is inconsistent with KptNavigationRailItem, which correctly passes selected = isSelected.
🐛 Proposed fix
- selected = false,
+ selected = isSelected,
alwaysShowLabel = isSelected,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selected = false, | |
| alwaysShowLabel = isSelected, | |
| selected = isSelected, | |
| alwaysShowLabel = isSelected, |
🤖 Prompt for AI Agents
In
`@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/bottombar/KptNavigationBarItem.kt`
around lines 60 - 61, KptNavigationBarItem currently hardcodes the
NavigationBarItem parameter selected = false, preventing selection styling;
update the call in KptNavigationBarItem to pass selected = isSelected (same as
KptNavigationRailItem) so the component reflects the selected state, ensuring
alwaysShowLabel remains tied to isSelected as well.
| /** | ||
| * Data class representing the pull-to-refresh state and behavior. | ||
| * | ||
| * @param isRefreshing Indicates whether the content is currently being | ||
| * refreshed. | ||
| * @param onRefresh Callback triggered when a pull-to-refresh gesture is | ||
| * performed. | ||
| */ | ||
| data class KptPullToRefreshState( | ||
| val isEnabled: Boolean, | ||
| val isRefreshing: Boolean, | ||
| val onRefresh: () -> Unit, | ||
| ) |
There was a problem hiding this comment.
Incomplete KDoc: isEnabled parameter is undocumented.
The data class has three fields, but the KDoc only documents isRefreshing and onRefresh. Consider adding documentation for isEnabled.
📝 Suggested documentation fix
/**
* Data class representing the pull-to-refresh state and behavior.
*
+ * `@param` isEnabled Whether pull-to-refresh gesture is enabled.
* `@param` isRefreshing Indicates whether the content is currently being
* refreshed.
* `@param` onRefresh Callback triggered when a pull-to-refresh gesture is
* performed.
*/🤖 Prompt for AI Agents
In
`@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.kt`
around lines 15 - 27, The KDoc for the KptPullToRefreshState data class is
missing documentation for the isEnabled parameter; update the class KDoc to add
a brief description for isEnabled (e.g., whether pull-to-refresh is
enabled/allowed) alongside the existing entries for isRefreshing and onRefresh
so all constructor parameters are documented.
| /** | ||
| * Remembers and returns a [KptPullToRefreshState] instance. | ||
| * | ||
| * @param isRefreshing Whether the refresh animation should be shown. | ||
| * *(Default: `false`)* | ||
| * @param onRefresh Callback to execute on pull-to-refresh. *(Default: | ||
| * empty lambda)* | ||
| */ | ||
| @Composable | ||
| fun rememberKptPullToRefreshState( | ||
| isEnabled: Boolean = false, | ||
| isRefreshing: Boolean = false, | ||
| onRefresh: () -> Unit = { }, | ||
| ) = remember(isEnabled, isRefreshing, onRefresh) { | ||
| KptPullToRefreshState( | ||
| isEnabled = isEnabled, | ||
| isRefreshing = isRefreshing, | ||
| onRefresh = onRefresh, | ||
| ) | ||
| } |
There was a problem hiding this comment.
KDoc for rememberKptPullToRefreshState also missing isEnabled documentation.
Similar to the data class, the isEnabled parameter should be documented.
Otherwise, the implementation correctly uses all three parameters as remember keys, ensuring proper recomposition behavior.
📝 Suggested documentation fix
/**
* Remembers and returns a [KptPullToRefreshState] instance.
*
+ * `@param` isEnabled Whether pull-to-refresh is enabled. *(Default: `false`)*
* `@param` isRefreshing Whether the refresh animation should be shown.
* *(Default: `false`)*
* `@param` onRefresh Callback to execute on pull-to-refresh. *(Default:
* empty lambda)*
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Remembers and returns a [KptPullToRefreshState] instance. | |
| * | |
| * @param isRefreshing Whether the refresh animation should be shown. | |
| * *(Default: `false`)* | |
| * @param onRefresh Callback to execute on pull-to-refresh. *(Default: | |
| * empty lambda)* | |
| */ | |
| @Composable | |
| fun rememberKptPullToRefreshState( | |
| isEnabled: Boolean = false, | |
| isRefreshing: Boolean = false, | |
| onRefresh: () -> Unit = { }, | |
| ) = remember(isEnabled, isRefreshing, onRefresh) { | |
| KptPullToRefreshState( | |
| isEnabled = isEnabled, | |
| isRefreshing = isRefreshing, | |
| onRefresh = onRefresh, | |
| ) | |
| } | |
| /** | |
| * Remembers and returns a [KptPullToRefreshState] instance. | |
| * | |
| * `@param` isEnabled Whether pull-to-refresh is enabled. *(Default: `false`)* | |
| * `@param` isRefreshing Whether the refresh animation should be shown. | |
| * *(Default: `false`)* | |
| * `@param` onRefresh Callback to execute on pull-to-refresh. *(Default: | |
| * empty lambda)* | |
| */ | |
| `@Composable` | |
| fun rememberKptPullToRefreshState( | |
| isEnabled: Boolean = false, | |
| isRefreshing: Boolean = false, | |
| onRefresh: () -> Unit = { }, | |
| ) = remember(isEnabled, isRefreshing, onRefresh) { | |
| KptPullToRefreshState( | |
| isEnabled = isEnabled, | |
| isRefreshing = isRefreshing, | |
| onRefresh = onRefresh, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
`@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptPullToRefreshState.kt`
around lines 29 - 48, The KDoc for rememberKptPullToRefreshState is missing
documentation for the isEnabled parameter; update the function KDoc to add a
line describing isEnabled (e.g., whether pull-to-refresh is enabled, default
false) so all parameters (isEnabled, isRefreshing, onRefresh) are documented;
locate the function rememberKptPullToRefreshState and add the isEnabled
description in its KDoc block to match the data class docs.
| floatingActionButton = { | ||
| Box(modifier = Modifier.navigationBarsPadding()) { | ||
| floatingActionButton() | ||
| } | ||
| }, |
There was a problem hiding this comment.
Inconsistent FAB padding between scaffold variants.
Variant C wraps the FAB with navigationBarsPadding(), but Variants A and B do not. This may cause FABs to be positioned differently depending on which overload is used. Consider aligning the behavior across all variants.
🤖 Prompt for AI Agents
In `@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.kt`
around lines 213 - 217, The Variant C implementation of KptScaffold wraps the
floatingActionButton with Modifier.navigationBarsPadding(), but Variants A and B
do not, causing inconsistent FAB positioning; update the other overloads of
KptScaffold (the variants that accept floatingActionButton) to apply the same
Modifier.navigationBarsPadding() to their floatingActionButton Box (or
alternatively remove it from Variant C if consistent removal is desired) so all
floatingActionButton usages use the same padding behavior; look for the
floatingActionButton lambdas in KptScaffold and ensure they consistently wrap
the content with Box(modifier = Modifier.navigationBarsPadding()) or remove that
wrapper from all to match the chosen consistent behavior.
| floatingActionButtonPosition = floatingActionButtonPosition, | ||
| containerColor = containerColor, | ||
| contentColor = contentColor, | ||
| contentWindowInsets = contentWindowInsets, |
There was a problem hiding this comment.
Potential double application of window insets in Variant C.
contentWindowInsets is passed to Scaffold (line 221) and then reapplied via windowInsetsPadding (line 233). This differs from Variants A/B which set WindowInsets(0.dp) on Scaffold to avoid double padding. This inconsistency may cause unexpected spacing.
🐛 Suggested fix for consistency with other variants
containerColor = containerColor,
contentColor = contentColor,
- contentWindowInsets = contentWindowInsets,
+ contentWindowInsets = WindowInsets(0.dp),
content = { paddingValues ->Also applies to: 233-233
🤖 Prompt for AI Agents
In `@core/ui/src/commonMain/kotlin/org/mifospay/core/ui/scaffold/KptScaffold.kt`
at line 221, Variant C is passing contentWindowInsets into KptScaffold/Scaffold
and then also applying windowInsetsPadding to the same content, causing double
insets; update KptScaffold so that when using contentWindowInsets it does not
reapply windowInsetsPadding (make Scaffold receive WindowInsets(0.dp) like
Variants A/B or remove the extra windowInsetsPadding call), adjust references
around contentWindowInsets and the windowInsetsPadding invocation in
KptScaffold/Scaffold to ensure only one application of insets (keep unique
symbols: KptScaffold, contentWindowInsets, windowInsetsPadding, Scaffold).
1 participant
Issue Fix
Jira Task: MW-328
Screenshots
Description
Summary of changes:
Apply the
AndroidStyle.xmlstyle template to your code in Android Studio.Run the unit tests with
./gradlew checkto 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
✏️ Tip: You can customize this high-level summary in your review settings.