-
Notifications
You must be signed in to change notification settings - Fork 508
Fix: ios build #1942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: ios build #1942
Conversation
WalkthroughThe pull request consolidates changes across multiple project components: updates Android manifest permissions and resources, corrects a CocoaPods resource path from Windows to POSIX format, and simplifies the interbank transfer date input by removing interactive date picker functionality from the TransferDetailsScreen, converting it to a static display. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/send-interbank/src/commonMain/kotlin/org/mifospay/feature/send/interbank/screens/TransferDetailsScreen.kt (3)
101-105: Add date validation to continue button logic.The continue button validation checks amount and description but doesn't validate the date field. Looking at line 536 in the preview, an empty date string is possible, which could lead to invalid transfers.
Apply this diff to add date validation:
enabled = amount.isNotEmpty() && amount.toDoubleOrNull()?.let { it <= (fromAccount?.balance ?: 0.0) } ?: false && - description.isNotEmpty(), + description.isNotEmpty() && + date.isNotEmpty(),
69-82: Missing date picker implementation in TransferDetailsScreen UI.The function signature correctly accepts a formatted date string (no modification callback), and the ViewModel handles date updates via
InterbankTransferAction.UpdateDate(Long). However, the UI displays a calendar icon at lines 208-234 that suggests the date is editable, yet there's no click handler, date picker dialog, or callback to actually modify the date. This creates a non-functional, misleading UX.Either remove the calendar icon if the date is read-only, or implement a date picker dialog that invokes
onDateChanged(which would need to be added to the function signature).
207-237: Calendar icon creates misleading affordance—users will expect interactivity that doesn't exist.The review comment is accurate. The recent iOS build fix (commit 8d398a8) removed all date picker functionality but left the calendar icon intact. The icon's
contentDescriptionexplicitly states "edit," strongly suggesting users can modify the date, but:
- The
.clickable()modifier was removed- No date picker dialog or state remains
- No
onDateChangedcallback exists- The date parameter is now read-only
This creates a false user affordance. Users seeing the calendar icon with "edit" text will expect to interact with it and be confused when nothing happens.
Recommended actions:
- Remove the calendar icon if the date must remain system-generated and read-only:
- Icon( - imageVector = Icons.Default.CalendarMonth, - contentDescription = stringResource(Res.string.feature_send_interbank_edit), - tint = KptTheme.colorScheme.primary, - )
- Or restore date picker functionality in a follow-up PR once the iOS build issue is resolved.
🧹 Nitpick comments (1)
cmp-shared/cmp_shared.podspec (1)
53-53: Resource path fix is correct; optional robustness improvementSwitching to a POSIX-style path for
spec.resourcesmatches the other paths in this podspec and should resolve iOS/CocoaPods lookups on macOS. If you ever need this to be portable or slightly more robust, you could build the path viaFile.join('build', 'compose', 'cocoapods', 'compose-resources'), but that’s strictly optional given the current macOS-only usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmp-android/prodRelease-badging.txt(1 hunks)cmp-shared/cmp_shared.podspec(1 hunks)feature/send-interbank/src/commonMain/kotlin/org/mifospay/feature/send/interbank/InterbankTransferFlowScreen.kt(0 hunks)feature/send-interbank/src/commonMain/kotlin/org/mifospay/feature/send/interbank/screens/TransferDetailsScreen.kt(2 hunks)
💤 Files with no reviewable changes (1)
- feature/send-interbank/src/commonMain/kotlin/org/mifospay/feature/send/interbank/InterbankTransferFlowScreen.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 (2)
feature/send-interbank/src/commonMain/kotlin/org/mifospay/feature/send/interbank/screens/TransferDetailsScreen.kt (2)
67-67: LGTM: Opt-in annotation updated correctly.The removal of
@OptIn(ExperimentalTime::class)is appropriate since date picker functionality has been removed.
69-270: Clarify the intent: Is this a temporary workaround or permanent design change?The PR title mentions "Fix: ios build," but the description doesn't explain how removing the date picker functionality relates to the iOS build issue. Please clarify:
- Does the date picker cause iOS build failures? If so, what is the specific error?
- Is this removal a temporary workaround until the underlying issue is fixed?
- Or is this a permanent design change where the date is system-generated?
- If temporary, should a follow-up issue be created to restore date picker functionality?
Understanding this context will help ensure the right solution is implemented.
Issue Fix
Fixes #{Issue Number}
Jira Task: Task_Number
Screenshots
Description
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
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.