feat: revert to multi-activity + wire first-run flow#140
Conversation
Replace two-activity (MainActivity + IconActivity) pattern with a single MainActivity hosting MainFragment and IconFragment via the Navigation Component. IconActivity is renamed to IconFragment and ported to Fragment lifecycle; MainFragment extracts the app-list UI from MainActivity. Add Navigation graph (main_navigation.xml) to define destinations and the icon-detail back-stack. Replace activity_icon layouts with fragment_icon (portrait + landscape); add fragment_main layout; update activity_main to host NavHostFragment. Remove IconActivity from the manifest and delete the now-obsolete instrumented-test class. Wire local common-utils include-build substitution for development until the fragment/nav APIs are published. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update IconViewModelTest and MainActivityScreenshotTest for the renamed/refactored fragment classes. Update BaselineProfileGenerator to navigate via NavController instead of starting IconActivity directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…x drawer menu IDs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bump mockk 1.14.9 → 1.14.11 (NewerVersionAvailable). Update lint baseline to include Overdraw entries for activity_icon.xml layouts restored during multi-activity revert. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ch planning docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a build-time flag to skip first-run onboarding for benchmark variants, gates MainActivity startup on onboardIfNeeded, extends baseline profile generation to pass the skip extra and navigate UI, refactors drawer menu handling, and updates docs, lint baselines, and dependency versions. ChangesOnboarding Skip Control and Baseline Profile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the first-run flow using common-utils, updates the baseline profile generator to skip the first-run chain, deletes the IconActivityTest file, and temporarily includes the local common-utils build in settings. Feedback highlights critical issues: passing an empty Intent to startActivityAndWait will fail to launch the app, unconditionally including the sibling build path in settings.gradle.kts will break CI/CD environments, and using findViewById with menu item IDs in MainActivity can lead to null pointer exceptions.
… benchmark Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
handleFirstRun → onboardIfNeeded EXTRA_SKIP_FIRST_RUN → EXTRA_SKIP_ONBOARDING Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Restore IconActivityTest.kt deleted by single-activity migration - Replace (findViewByid ?: binding.navigationView) with findViewByid<View>: the ?: caused Kotlin to infer T=DrawerNavigationView, inserting a checkcast that failed at runtime; IDs exist in R.id so no fallback needed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes to support skipping the onboarding flow during baseline profile generation, updates the mockk dependency, and refactors navigation and onboarding logic in MainActivity. Feedback on the changes includes a recommendation to safely handle potential null values from findViewById when handling navigation clicks to prevent crashes, and a suggestion to remove a redundant non-null assertion operator (!!) in the Gradle build configuration.
…onboardIfNeeded) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the onboarding flow by integrating onboardIfNeeded from common-utils v1.1.0, replacing the previous OOBE setup. It introduces a FIRST_RUN_SKIPPABLE build configuration to allow baseline profile benchmarks to bypass onboarding, and enhances the baseline profile generator to navigate to the icon screen and back. Additionally, it updates dependency versions and refactors the navigation drawer menu item IDs in MainActivity. Feedback is provided regarding a potential NullPointerException when using findViewById on menu item IDs within the navigation drawer, suggesting a safe fallback helper to prevent crashes if the views are recycled or not yet laid out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/lint-baseline.xml (1)
15-35: ⚡ Quick winVerify the Overdraw suppression is intentional.
These baseline entries suppress Overdraw warnings for
activity_icon.xmllayouts where the root element paints?roundedCornerColorwhile the theme also paints a background. Overdraw can impact rendering performance, though modern devices handle it well.Please confirm:
- Is the background on the root element necessary for visual design (e.g., rounded corners)?
- Can the theme background be disabled for this activity to eliminate the overdraw?
If the background is required, the suppression is acceptable. Otherwise, consider removing one of the redundant backgrounds.
💡 Potential fix if theme background is not needed
If the roundedCornerColor is sufficient, you could apply a transparent theme background for IconActivity:
<style name="Theme.GetIcon.IconActivity" parent="CommonUtils.AppTheme"> <item name="android:windowBackground">`@android`:color/transparent</item> </style>Then reference it in AndroidManifest.xml for IconActivity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lint-baseline.xml` around lines 15 - 35, The lint baseline is suppressing Overdraw for the root background in the layouts activity_icon.xml (root uses android:background="?roundedCornerColor" and inferred theme `@style/CommonUtils.AppTheme`); verify whether the root background is required for the rounded-corner visual, and if not remove the root android:background from src/main/res/layout/activity_icon.xml and src/main/res/layout-land/activity_icon.xml to eliminate redundant theme background, otherwise create/apply an activity-specific theme (e.g., Theme.GetIcon.IconActivity derived from CommonUtils.AppTheme) that sets android:windowBackground to transparent and assign it to IconActivity in the manifest so the theme background doesn't double-paint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/de/lemke/geticon/ui/MainActivity.kt`:
- Around line 80-83: The call to prepareActivityTransformationFrom() is executed
before the onboarding early-return, causing UI/transform setup even when
onboardIfNeeded(...) returns null; move prepareActivityTransformationFrom() so
it runs after the onboarding gate (i.e., after the
onboardIfNeeded(BuildConfig.VERSION_CODE, BuildConfig.VERSION_NAME, ...) ?:
return check inside MainActivity.onCreate()), keeping installSplashScreen() and
super.onCreate(savedInstanceState) in place as needed; update the onCreate()
sequence to call prepareActivityTransformationFrom() only after onboardIfNeeded
allows continuation.
In
`@baselineprofile/src/main/java/de/lemke/geticon/baselineprofile/BaselineProfileGenerator.kt`:
- Around line 52-60: In navigateToIconAndBack(), the UIAutomator finds are
treated as optional so the benchmark can continue silently when selectors fail;
replace the safe calls with explicit null checks (e.g., use requireNotNull or
throw IllegalStateException) for the result of
device.wait(Until.findObject(By.res(PACKAGE_NAME, "appPicker")), TIMEOUT_MS) and
for the subsequent findObject(By.clazz("android.widget.TextView")) (referenced
as appItem) and also for the final
device.wait(Until.findObject(By.res(PACKAGE_NAME, "icon")), TIMEOUT_MS); if any
are null, fail fast with a clear error message mentioning the missing selector
(appPicker/TextView/icon) so the benchmark stops instead of proceeding silently.
In `@CLAUDE.md`:
- Around line 18-20: Update the stale sentence in CLAUDE.md that reads "No
instrumented tests yet." to reflect that instrumented tests now exist; mention
the new instrumentation files such as TestApp
(app/src/androidTest/java/de/lemke/geticon/TestApp.kt) and the baseline-profile
generator test path so the README accurately documents current instrumented-test
coverage (replace or remove the outdated line and add a brief note pointing to
those instrumentation locations).
---
Nitpick comments:
In `@app/lint-baseline.xml`:
- Around line 15-35: The lint baseline is suppressing Overdraw for the root
background in the layouts activity_icon.xml (root uses
android:background="?roundedCornerColor" and inferred theme
`@style/CommonUtils.AppTheme`); verify whether the root background is required for
the rounded-corner visual, and if not remove the root android:background from
src/main/res/layout/activity_icon.xml and
src/main/res/layout-land/activity_icon.xml to eliminate redundant theme
background, otherwise create/apply an activity-specific theme (e.g.,
Theme.GetIcon.IconActivity derived from CommonUtils.AppTheme) that sets
android:windowBackground to transparent and assign it to IconActivity in the
manifest so the theme background doesn't double-paint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b8bbfb5-4163-452a-a859-94881c92c668
📒 Files selected for processing (7)
CLAUDE.mdapp/build.gradle.ktsapp/lint-baseline.xmlapp/src/androidTest/java/de/lemke/geticon/TestApp.ktapp/src/main/java/de/lemke/geticon/ui/MainActivity.ktbaselineprofile/src/main/java/de/lemke/geticon/baselineprofile/BaselineProfileGenerator.ktgradle/libs.versions.toml
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the onboarding flow to use the new onboardIfNeeded utility from common-utils (which was upgraded to version 1.1.1). It introduces a FIRST_RUN_SKIPPABLE build config field to allow baseline profile benchmarks to bypass the onboarding flow using a new EXTRA_SKIP_ONBOARDING intent extra. Additionally, it refactors navigation logic in MainActivity, updates documentation in CLAUDE.md, and refines the baseline profile generator to navigate to the icon screen and back. I have no feedback to provide as there are no review comments.
Was called before super.onCreate() and before the onboardIfNeeded early
return — running unnecessarily on every first-run redirect and violating
the library's own kdoc ("after super.onCreate").
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the onboarding flow by integrating onboardIfNeeded from common-utils (which was upgraded to version 1.1.1), allowing benchmarks to skip the first-run chain via a new FIRST_RUN_SKIPPABLE build configuration. It also enhances the baseline profile generator to navigate through the app picker and icon screens, updates documentation, and cleans up MainActivity navigation logic. Feedback on these changes includes removing a redundant double-bang (!!) operator in the Gradle build file and resolving a potential UI Automator race condition in the baseline profile generator by waiting for descendants to be laid out.
|
🤖 Claude Code · Won't fix (re: CodeRabbitAI at |
Avoids a race condition where the container appears before list items are bound, causing findObject(TextView) to return null immediately.
1 participant
Summary
MainActivity+IconActivityas separate activitieshandleFirstRun(...)is the first call inMainActivity.onCreate(beforesetContentView) — OOBE-onlyabout_app_dest→commonutils_about_destetc.)BuildConfig.FIRST_RUN_SKIPPABLE(false in production, true fornonMinifiedReleaseprofiling variant); updatesBaselineProfileGeneratorto passEXTRA_SKIP_FIRST_RUNso OOBE is never in the profile pathTest Plan
./gradlew build→ BUILD SUCCESSFUL (spotless, detekt, lint, Konsist, screenshot tests)./gradlew :app:assembleNonMinifiedRelease→ BUILD SUCCESSFULDepends on
🤖 Generated with Claude Code
Architecture & Onboarding
Configuration & Build
Fixes & Updates
Builds: ./gradlew build and :app:assembleNonMinifiedRelease reported successful; drawer navigation verified. Depends on common-utils PR
#126.