Target Android 17 (API 37)#771
Conversation
|
Warning Review limit reached
More reviews will be available in 9 minutes and 13 seconds. 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)
📝 WalkthroughWalkthroughRefactors runtime permission handling to a multi-permission request with a retry queue, centralizes permission metadata in a new enum, adds ACCESS_LOCAL_NETWORK and string resources for it, bumps target SDK to 37 and test matrix to API 37, and updates instrumented tests to use TestContext extension helpers. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MA as MainActivity
participant Sys as PermissionSystem
participant Dialog as AlertDialog
participant Settings
User->>MA: App startup
MA->>Sys: requestRuntimePermissions()
Sys->>Sys: Determine permissions array<br/>based on SDK (POST_NOTIFICATIONS, ACCESS_LOCAL_NETWORK)
Sys->>Sys: Launch RequestMultiplePermissions
Sys-->>MA: Permission results
alt Denied but rationale eligible
MA->>Sys: shouldShowRequestPermissionRationale(permission)
Sys-->>MA: true (enqueue permission)
MA->>Dialog: Show rationale message for permission
User->>Dialog: Chooses "No" or "Yes"
alt User chooses "Yes"
Dialog->>Sys: Trigger request for the queued permission
Sys-->>MA: Result for re-request
else User chooses "No"
Dialog->>Settings: Option to open App Settings
end
else All permissions granted
MA->>MA: Continue initialization
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
|
No dependency violations found. 👍 |
|
Generated by 🚫 Danger |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
app/src/androidTest/kotlin/artemis/agent/screens/MainScreen.kt (1)
85-94:⚠️ Potential issue | 🟠 Major
assertPermissionRationaleDialogOpenhardcodesnotification_rationalebut the queue can surface other permissions' rationales.On API 34+ (
CINNAMON_BUN),MainActivity.requestRuntimePermissionsrequests bothPOST_NOTIFICATIONSandACCESS_LOCAL_NETWORK. When both are denied,resentPermissionRequestsqueues both and the rationale dialog sequentially shows each permission'srationaleMessage—R.string.notification_rationalefor the first,R.string.local_network_rationalefor the second (perRuntimePermissionInfo.ACCESS_LOCAL_NETWORK). BecausePermissionRationaleTest.testPermissionDialogloopsnumPermissionDialogstimes and calls this assertion each iteration, the 2nd iteration on API 34+ will fail matchingnotification_rationaleagainst the actuallocal_network_rationaletext.🐛 Proposed fix: parameterize the expected rationale string
- fun assertPermissionRationaleDialogOpen() { + fun assertPermissionRationaleDialogOpen(`@StringRes` messageRes: Int) { alertDialog { isCompletelyDisplayed() title.isRemoved() - message.isDisplayedWithText(R.string.notification_rationale) + message.isDisplayedWithText(messageRes) positiveButton.isDisplayedWithText(R.string.yes) negativeButton.isDisplayedWithText(R.string.no) neutralButton.isRemoved() } }Then in
PermissionRationaleTest.testPermissionDialog, iterate with the expected rationales in the same order the queue produces them (driven byRuntimePermissionInfo.entriesfiltered by SDK), e.g.:val expectedRationales = buildList { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) add(R.string.notification_rationale) if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.CINNAMON_BUN) add(R.string.local_network_rationale) } expectedRationales.forEach { messageRes -> step("Check for permission rationale dialog") { assertPermissionRationaleDialogOpen(messageRes) } withDialog() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/androidTest/kotlin/artemis/agent/screens/MainScreen.kt` around lines 85 - 94, The current assertPermissionRationaleDialogOpen hardcodes R.string.notification_rationale so it fails when other permission rationales (e.g., R.string.local_network_rationale) are shown; modify the function assertPermissionRationaleDialogOpen to accept an expected message resource id parameter (e.g., messageRes: Int) and use that instead of the hardcoded R.string.notification_rationale, and update PermissionRationaleTest.testPermissionDialog to build and iterate an expectedRationales list in the same order RuntimePermissionInfo.entries produces (filtering by SDK version) and call assertPermissionRationaleDialogOpen(messageRes) for each entry so each dialog assertion matches the queued rationale text.
🧹 Nitpick comments (1)
app/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.kt (1)
10-26: Ordinal-indexed expectations are fragile; consider per-entry mapping.Driving expectations via
permissionNames[info.ordinal]/rationaleResIds[info.ordinal]silently breaks if entries are reordered or a new entry is inserted in the middle — the test will then assert the wrong pair without any visible change to this file. AMap<RuntimePermissionInfo, Pair<String, Int>>(or a list of triples fed intowithData) keeps expectations co-located with the entry they describe.♻️ Proposed refactor
- val rationaleResIds = - intArrayOf(R.string.notification_rationale, R.string.local_network_rationale) - val permissionNames = - arrayOf( - "android.permission.POST_NOTIFICATIONS", - "android.permission.ACCESS_LOCAL_NETWORK", - ) - - withData(nameFn = { it.name }, RuntimePermissionInfo.entries) { info -> - it("Permission name") { - info.permission shouldBeEqual permissionNames[info.ordinal] - } - - it("Rationale message") { - info.rationaleMessage shouldBeEqual rationaleResIds[info.ordinal] - } - } + val expected = mapOf( + RuntimePermissionInfo.POST_NOTIFICATIONS to + ("android.permission.POST_NOTIFICATIONS" to R.string.notification_rationale), + RuntimePermissionInfo.ACCESS_LOCAL_NETWORK to + ("android.permission.ACCESS_LOCAL_NETWORK" to R.string.local_network_rationale), + ) + + withData(nameFn = { it.name }, RuntimePermissionInfo.entries) { info -> + val (permissionName, rationaleResId) = expected.getValue(info) + it("Permission name") { info.permission shouldBeEqual permissionName } + it("Rationale message") { info.rationaleMessage shouldBeEqual rationaleResId } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.kt` around lines 10 - 26, The test currently uses ordinal-indexed arrays (permissionNames, rationaleResIds) and compares via permissionNames[info.ordinal] and rationaleResIds[info.ordinal], which breaks if RuntimePermissionInfo.entries order changes; replace this with a per-entry mapping: create a Map<RuntimePermissionInfo, Pair<String, Int>> (or list of triples) that maps each RuntimePermissionInfo value to its expected permission string and rationale res id, then iterate that map in the withData call (or feed the list into withData) and assert info.permission equals the mapped string and info.rationaleMessage equals the mapped res id, updating the assertions that reference info.ordinal to use the lookup by key instead so expectations stay co-located with each entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.gradle.kts`:
- Line 32: The project currently sets the SDK to 37 via set("sdkVersion", 37)
which makes both compile/target use API 37; change the target SDK to 36 for now
(keep compileSdk at 37 for testing) by updating the Gradle Kotlin DSL so the
compile SDK remains 37 (leave set("sdkVersion", 37) if that maps to compileSdk)
and explicitly set targetSdk (e.g., set("targetSdk", 36) or the existing
targetSdk property) to 36 until Android 17 is stable; if this repo uses a
different property name for targetSdk, change that property to 36 instead of 37.
---
Outside diff comments:
In `@app/src/androidTest/kotlin/artemis/agent/screens/MainScreen.kt`:
- Around line 85-94: The current assertPermissionRationaleDialogOpen hardcodes
R.string.notification_rationale so it fails when other permission rationales
(e.g., R.string.local_network_rationale) are shown; modify the function
assertPermissionRationaleDialogOpen to accept an expected message resource id
parameter (e.g., messageRes: Int) and use that instead of the hardcoded
R.string.notification_rationale, and update
PermissionRationaleTest.testPermissionDialog to build and iterate an
expectedRationales list in the same order RuntimePermissionInfo.entries produces
(filtering by SDK version) and call
assertPermissionRationaleDialogOpen(messageRes) for each entry so each dialog
assertion matches the queued rationale text.
---
Nitpick comments:
In `@app/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.kt`:
- Around line 10-26: The test currently uses ordinal-indexed arrays
(permissionNames, rationaleResIds) and compares via
permissionNames[info.ordinal] and rationaleResIds[info.ordinal], which breaks if
RuntimePermissionInfo.entries order changes; replace this with a per-entry
mapping: create a Map<RuntimePermissionInfo, Pair<String, Int>> (or list of
triples) that maps each RuntimePermissionInfo value to its expected permission
string and rationale res id, then iterate that map in the withData call (or feed
the list into withData) and assert info.permission equals the mapped string and
info.rationaleMessage equals the mapped res id, updating the assertions that
reference info.ordinal to use the lookup by key instead so expectations stay
co-located with each entry.
🪄 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: d85332a6-4836-4dc7-b5a9-f5eb9175053e
📒 Files selected for processing (11)
.github/workflows/instrumented-tests.ymlapp/src/androidTest/kotlin/artemis/agent/MainActivityTest.ktapp/src/androidTest/kotlin/artemis/agent/PermissionRationaleTest.ktapp/src/androidTest/kotlin/artemis/agent/screens/MainScreen.ktapp/src/main/AndroidManifest.xmlapp/src/main/kotlin/artemis/agent/MainActivity.ktapp/src/main/kotlin/artemis/agent/RuntimePermissionInfo.ktapp/src/main/res/values-v33/strings.xmlapp/src/main/res/values-v37/strings.xmlapp/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.ktbuild.gradle.kts
|
|
||
| extra.apply { | ||
| set("sdkVersion", 36) | ||
| set("sdkVersion", 37) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the build.gradle.kts file
fd -n "build\.gradle\.kts" | head -5Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 309
🏁 Script executed:
# Let's look at the content around line 32 to understand the full context
head -n 50 build.gradle.kts | tail -n 25Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 644
🏁 Script executed:
# Search for all SDK-related configurations
rg "sdkVersion|compileSdk|targetSdk" build.gradle.kts -A 2 -B 2Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 188
🏁 Script executed:
# Search for compileSdk and targetSdk usage across the project
rg "compileSdk|targetSdk" -t gradle -t kotlinRepository: JPL-Software-Solutions/ArtemisAgent
Length of output: 236
🏁 Script executed:
# Look for AGP version and Build-Tools
rg "com\.android\.tools\.build:gradle|buildToolsVersion|androidGradlePlugin" -t gradle -t kotlinRepository: JPL-Software-Solutions/ArtemisAgent
Length of output: 61
🏁 Script executed:
# Check the gradle wrapper and libs.versions.toml if available
find . -name "gradle-wrapper.properties" -o -name "libs.versions.toml" | head -5Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 149
🏁 Script executed:
# Check libs.versions.toml for AGP version
cat gradle/libs.versions.toml | head -50Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 1310
🏁 Script executed:
# Look at app/build.gradle.kts to see full Android configuration
head -n 100 app/build.gradle.kts | grep -A 10 -B 5 "android\|compileSdk\|targetSdk\|buildToolsVersion"Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 1756
🏁 Script executed:
# Search more thoroughly for AGP version in libs.versions.toml
cat gradle/libs.versions.toml | grep -i "android\|agp"Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 4097
🏁 Script executed:
# Also check if there's a plugins section in libs.versions.toml
cat gradle/libs.versions.toml | grep -A 30 "\[plugins\]"Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 1075
🏁 Script executed:
# Check the gradle wrapper version
cat gradle/wrapper/gradle-wrapper.propertiesRepository: JPL-Software-Solutions/ArtemisAgent
Length of output: 417
Consider deferring targetSdk to 36 until Android 17 reaches stable release.
Both compileSdk and targetSdk are set to API level 37 (Android 17/Cinnamon Bun). As of April 2026, Android 17 has not yet reached stable release (scheduled for June 2026, currently Beta 4), meaning your app will opt into pre-release behavior changes that may still shift, including the lock-free MessageQueue for apps targeting SDK 37+ (potentially breaking reflective clients) and removal of the developer opt-out for orientation/resizability restrictions on large screens (sw > 600 dp).
AGP 9.2.0 supports API 37 compilation, so there are no build-tool compatibility blockers. However, if stable release is not required for this build, consider keeping compileSdk=37 with targetSdk=36 until Android 17 stabilizes in June.
If targeting API 37 pre-release is intentional (e.g., pre-release testing branch), this can be ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.gradle.kts` at line 32, The project currently sets the SDK to 37 via
set("sdkVersion", 37) which makes both compile/target use API 37; change the
target SDK to 36 for now (keep compileSdk at 37 for testing) by updating the
Gradle Kotlin DSL so the compile SDK remains 37 (leave set("sdkVersion", 37) if
that maps to compileSdk) and explicitly set targetSdk (e.g., set("targetSdk",
36) or the existing targetSdk property) to 36 until Android 17 is stable; if
this repo uses a different property name for targetSdk, change that property to
36 instead of 37.
There was a problem hiding this comment.
I did want to be an early bird on letting this project adopt Android 17's API. My only regret is that the stable emulator images have not been released yet. Until that happens, the instrumented tests will not pass.
I don't see MessageQueue behaviour as a concern because, AFAIK, my app does not use it anywhere. AFAICT, the only major change that my app needs to account for is the local network permission.
There was a problem hiding this comment.
@JordanLongstaff, that makes perfect sense — thank you for the context. A few things worth noting that support your approach:
- Android 17 has officially reached Platform Stability with Beta 3, meaning the API surface is now locked — so there's no risk of APIs still shifting under you.
- Beta 4 is the final scheduled beta of the release cycle, and the API surface is confirmed final, meaning no major new features are expected before the stable rollout.
- The stable release is expected in June 2026, timed around Google I/O (scheduled for May 20, 2026) — so stable emulator images should arrive soon.
- Regarding
MessageQueue: the new lock-free implementation only affects clients that reflect onMessageQueueprivate fields and methods, so if your app doesn't do that, it's a non-issue as you've assessed.
Your early adoption approach is completely reasonable here, especially given that Platform Stability is already declared. I'll withdraw my concern.
(ᵔ◡ᵔ)🐇
✏️ Learnings added
Learnt from: JordanLongstaff
Repo: JPL-Software-Solutions/ArtemisAgent PR: 771
File: build.gradle.kts:32-32
Timestamp: 2026-05-18T02:56:07.819Z
Learning: The ArtemisAgent project (JPL-Software-Solutions/ArtemisAgent) intentionally targets the latest Android SDK version, even pre-stable (e.g., API 37 / Android 17), for early adoption purposes. Do not flag targeting a pre-release SDK version as a concern in this project when it has reached Platform Stability (API surface locked). The author is aware of the trade-off that instrumented tests may not pass until stable emulator images are released.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: JordanLongstaff
Repo: JPL-Software-Solutions/ArtemisAgent PR: 762
File: app/build.gradle.kts:120-122
Timestamp: 2026-04-21T10:57:28.416Z
Learning: When reviewing Android projects using the Android Gradle Plugin (AGP), recognize that local JVM unit test tasks are generated per build variant with the naming convention `test<VariantName>UnitTest` (for example, `testDebugUnitTest`, `testReleaseUnitTest`). If a PR references a task name matching this pattern, do not flag it as invalid or non-existent solely due to the name.
Learnt from: JordanLongstaff
Repo: JPL-Software-Solutions/ArtemisAgent PR: 762
File: app/build.gradle.kts:120-122
Timestamp: 2026-04-21T12:14:21.084Z
Learning: For Android projects using Android Gradle Plugin (AGP) 9+, do not assume a `release`-variant local JVM unit test task (e.g., `testReleaseUnitTest`) will be generated. Only the `debug` unit test task (e.g., `testDebugUnitTest`) is created by default. If you need unit tests (including any konsist/unit-test integration) to always run before `assemble*` tasks across variants, wire the relevant `assemble` tasks to depend on `:app:<test-task>` (specifically `testDebugUnitTest` in AGP 9) rather than hardcoding per-variant test task names that may not exist for `release`. Ensure the dependency targets the actual generated task and matches AGP 9 task naming/creation behavior.
Learnt from: JordanLongstaff
Repo: JPL-Software-Solutions/ArtemisAgent PR: 775
File: gradle/libs.versions.toml:254-255
Timestamp: 2026-04-25T22:22:05.582Z
Learning: In ArtemisAgent modules that apply the `io.kotzilla.kotzilla-plugin`, the plugin will auto-add `kotzilla-sdk` but will not auto-add `kotzilla-core`. Always declare `kotzilla-core` explicitly in that module’s dependencies. If a module omits `kotzilla-core`, the Dependency Analysis Gradle plugin will fail (so this should be treated as a review-blocking misconfiguration).
Code Coverage (IAN/enums)
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Code Coverage (IAN/udp)
|
Code Coverage (IAN/util)
|
Code Coverage (IAN)
|
Code Coverage (IAN/grid)
|
Code Coverage (IAN/world)
|
Code Coverage (IAN/packets)
|
Code Coverage (app)
|
Code Coverage (IAN/vesseldata)
|
Unit Test Results 128 files 128 suites 17m 26s ⏱️ Results for commit 7c7050d. ♻️ This comment has been updated with latest results. |
Konsist Test Results 14 files 14 suites 45s ⏱️ Results for commit 7c7050d. ♻️ This comment has been updated with latest results. |
Code Coverage (IAN/listener)
|
Total Code Coverage
|
Instrumented Test Results 7 files 104 suites 37m 7s ⏱️ For more details on these failures, see this check. Results for commit 7c7050d. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/androidTest/kotlin/artemis/agent/PermissionRationaleTest.kt (1)
46-49: Consider derivingrationaleMessagesfromRuntimePermissionInfoto avoid duplication.The array hardcodes the same
rationaleMessageIDs already declared on eachRuntimePermissionInfoentry, and its positional indexing relies on the implicit assumption that the enum's API-gating order (TIRAMISU → CINNAMON_BUN) matchesnumPermissionDialogs's iteration. If a future permission is inserted between these API levels, the index alignment in the loop at lines 59–62 will silently use the wrong rationale. Sourcing the list from the enum (filtered byBuild.VERSION.SDK_INT) keeps the test in lockstep with production metadata.♻️ Suggested approach
- val rationaleMessages = intArrayOf( - R.string.notification_rationale, - R.string.local_network_rationale, - ) + val rationaleMessages: IntArray = + RuntimePermissionInfo.entries + .filter { Build.VERSION.SDK_INT >= /* min API per entry */ } + .map { it.rationaleMessage } + .toIntArray()(Exposing the min API on
RuntimePermissionInfo— or a helper likeRuntimePermissionInfo.applicableEntries()— would make this clean.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/androidTest/kotlin/artemis/agent/PermissionRationaleTest.kt` around lines 46 - 49, The test hardcodes rationaleMessages which duplicates IDs from RuntimePermissionInfo and relies on positional order; instead derive the expected rationale IDs from RuntimePermissionInfo (filtering entries by Build.VERSION.SDK_INT or using a helper like RuntimePermissionInfo.applicableEntries()) and use that list when asserting against numPermissionDialogs in the loop, so replace the hardcoded rationaleMessages with a computed array sourced from RuntimePermissionInfo to keep the test aligned with production metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/androidTest/kotlin/artemis/agent/PermissionRationaleTest.kt`:
- Around line 46-49: The test hardcodes rationaleMessages which duplicates IDs
from RuntimePermissionInfo and relies on positional order; instead derive the
expected rationale IDs from RuntimePermissionInfo (filtering entries by
Build.VERSION.SDK_INT or using a helper like
RuntimePermissionInfo.applicableEntries()) and use that list when asserting
against numPermissionDialogs in the loop, so replace the hardcoded
rationaleMessages with a computed array sourced from RuntimePermissionInfo to
keep the test aligned with production metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1e2018f-82c3-4fa1-90b7-962c6f02be3d
📒 Files selected for processing (4)
.github/workflows/instrumented-tests.ymlapp/src/androidTest/kotlin/artemis/agent/PermissionRationaleTest.ktapp/src/androidTest/kotlin/artemis/agent/screens/MainScreen.ktapp/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/test/kotlin/artemis/agent/RuntimePermissionInfoTest.kt
- .github/workflows/instrumented-tests.yml
- app/src/androidTest/kotlin/artemis/agent/screens/MainScreen.kt
|
Actionable comments posted: 0 |
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
Localization