-
-
Notifications
You must be signed in to change notification settings - Fork 0
chore: sync upstream PR #7598 - fix(cli): Android apk name multi flavor dimensions parsing #15
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
base: plus
Are you sure you want to change the base?
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
2f4cada to
9bd9c50
Compare
9bd9c50 to
de435c4
Compare
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: 0
🧹 Nitpick comments (1)
cli/src/common.ts (1)
540-543: Perfect! I now have all the information needed to verify and rewrite the review comment. Let me summarize my findings:Key Verification Results:
- ✅ Function exists at lines 539-545 with the exact code described
- ✅ Logic is correct for converting camelCase to kebab-case APK naming
- ✅ Function is actively used in
cli/src/android/run.tsandcli/src/config.ts- ✅ Commit confirms upstream sync from PR ionic-team#7598
- ❌ This is a NEW function being added, not replacing existing code - so the fork compatibility concern is moot
⚠️ No tests found for this function in the fork (reasonable for new upstream code)- ✅ No hardcoded APK patterns that would break
Issue with original review: The review asks to "verify that no fork-specific code depends on the old APK name format," but this is a new function being added to the codebase. There is no "old format" to break compatibility with in this fork.
Logic correctly converts camelCase flavor names to kebab-case APK naming per Android Gradle conventions.
The implementation is sound. This is a new function from upstream PR ionic-team#7598, not replacing existing code, so there's no fork-specific compatibility concern.
Minor nitpick: Line 542 could be more idiomatic:
- if (convertedName.startsWith('-')) convertedName = convertedName.replace('-', ''); + if (convertedName.startsWith('-')) convertedName = convertedName.substring(1);Verify that upstream PR ionic-team#7598 includes tests for this new function (not found in this fork's codebase).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/common.ts(1 hunks)
⏰ 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). (4)
- GitHub Check: test-ios (/Applications/Xcode_26.0.app)
- GitHub Check: test-android
- GitHub Check: test-cli
- GitHub Check: test-core
Upstream PR Sync
This PR syncs changes from an external contributor's PR on the official Capacitor repository.
Original PR
Automation
Synced from upstream by Capacitor+ Bot
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.