task/ADFA-3961 avd generate cache phase#1320
Open
jomen-adfa wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 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 | ✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/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
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
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
jomen-adfa
force-pushed
the
task/ADFA-3961-avd-generate-cache
branch
from
7dcce2e to
4053bac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt (1)
47-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse localized toolbar labels instead of hardcoded English actions.
Line 47 and Line 83 pass
"Sync project"/"Quick run"directly; this is locale-fragile while the file already usesR.stringelsewhere.Suggested fix
step("Dismiss first build notice and start init") { + val targetContext = InstrumentationRegistry.getInstrumentation().targetContext + val syncProjectDescription = targetContext.getString(R.string.cd_toolbar_sync_project) ... - clickToolbarButton("Sync project") + clickToolbarButton(syncProjectDescription) } step("Click quick-run to build APK") { - clickToolbarButton("Quick run") + val targetContext = InstrumentationRegistry.getInstrumentation().targetContext + val quickRunDescription = targetContext.getString(R.string.cd_toolbar_quick_run) + clickToolbarButton(quickRunDescription) }Also applies to: 83-83
🤖 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 47, Replace the hardcoded English toolbar labels passed to clickToolbarButton with the localized string resources: find the calls to clickToolbarButton("Sync project") and clickToolbarButton("Quick run") and change them to use the app's resources (e.g., context.getString(R.string.sync_project) and context.getString(R.string.quick_run) or the test's resource accessor), ensuring you import/use the same R.string identifiers used elsewhere in the file so the toolbar labels are locale-safe.
♻️ Duplicate comments (6)
app/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.kt (1)
31-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuarantee close-project cleanup with
try/finally.If either scenario throws,
CloseProjectScenario()is skipped and contaminates following tests.Suggested 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 test setup in TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild can skip CloseProjectScenario if an earlier scenario throws; wrap the calls to scenario(InitializationProjectAndCancelingBuildScenario(...)) and scenario(RunAssembleTasksScenario()) in a try/finally and invoke scenario(InitializationProjectAndCancelingBuildScenario.CloseProjectScenario()) from the finally block so CloseProjectScenario always runs even on exceptions; update the method to call CloseProjectScenario in finally while keeping the same scenario constructors (InitializationProjectAndCancelingBuildScenario, RunAssembleTasksScenario, CloseProjectScenario) for identification.app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt (2)
164-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoded
"Save files and close project"text.Line 164 uses a literal that will fail on non-English devices.
🤖 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, Replace the hardcoded literal "Save files and close project" with a localized string resource: add a key like save_files_and_close_project to the app/test strings.xml and change the val saveAndClose assignment in InitializationProjectAndCancelingBuildScenario.kt to fetch the value via a context-based getString call (e.g., using InstrumentationRegistry or ApplicationProvider to obtain a Context and then context.getString(R.string.save_files_and_close_project)) so the test works on non-English devices.
167-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer save-and-close before without-saving options.
The selector order currently prioritizes “without saving”, which risks data loss when both actions are present.
Suggested fix
listOf( - UiSelector().text(closeWithoutSaving), - UiSelector().textContains("without saving"), UiSelector().text(saveAndClose), + UiSelector().text(closeWithoutSaving), + UiSelector().textContains("without saving"), )Also applies to: 260-264, 281-285
🤖 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 167 - 171, The UI selector list currently orders closeWithoutSaving before saveAndClose, risking accidental selection of the “without saving” action; update the listOf(...) used with firstNotNullOfOrNull so saveAndClose appears before closeWithoutSaving (i.e., listOf(UiSelector().text(saveAndClose), UiSelector().textContains("without saving"), UiSelector().text(closeWithoutSaving))) and apply the same change to the other occurrences that use the same pattern in InitializationProjectAndCancelingBuildScenario (the blocks using listOf(...).firstNotNullOfOrNull with closeWithoutSaving and saveAndClose).app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt (2)
74-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate confirmation for every requested task, not only
:app:assemble.The current check can pass even when some selected tasks are missing from the confirmation.
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 current single check only asserts that ":app:assemble" appears; replace it with a loop over the list of requested tasks (e.g., the variable holding the selected/requested tasks) and for each task call check(d.findObject(UiSelector().textContains(task)).waitForExists(10_000)) { "Run tasks confirmation did not show selected task: $task" } so every requested task is validated; keep using the existing check(...) helper and d.findObject(UiSelector().textContains(...)).waitForExists(10_000) pattern.
56-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle off-screen task rows before asserting existence.
Current selection only waits for immediate visibility; long lists can keep valid tasks off-screen and fail the step.
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() } }🤖 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 fails when the desired Gradle task is off-screen; update the tasks.forEach block (the step("Select Gradle task $task") that uses device.uiDevice and clickFirstAccessibilityNodeParentByText) to first attempt to scroll the tasks list into view (e.g., use a UiScrollable or other scroll-into-view call targeting the container) before asserting existence with d.findObject(UiSelector().text(task)).waitForExists and before calling clickFirstAccessibilityNodeParentByText(task), so the element is brought on-screen and the existence check won't false-fail for off-screen rows.app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt (1)
60-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace fixed launch sleep with a state-based wait.
Line 61 is timing-sensitive and can flake under CI/device load.
🤖 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 wait for a specific UI/state indicating the splash finished (e.g., use Espresso.onView with a ViewMatcher for a view in the next screen and .check(matches(isDisplayed())) or register a CountingIdlingResource/IdlingRegistry that the SplashActivity signals when ready), or use ActivityScenario.onActivity to poll the Activity state until the expected condition is true; target the call sites ActivityScenario.launch(SplashActivity::class.java) and Thread.sleep(1000) when making the change.
🤖 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 118-130: Tests are using hardcoded English strings ("NEXT",
"Finish installation") which are locale-fragile; replace these with
resource-based or accessibility-id based lookups. Update the call sites (e.g.,
nextObj and clickFirstAccessibilityNodeByText calls that pass searchText =
"NEXT" and the other occurrence for "Finish installation") to locate buttons by
stable identifiers such as view/resource IDs (UiSelector().resourceId or
node.viewIdResourceName), contentDescription keys tied to R.string, or a
localization-aware lookup that reads the app's string resources for the current
locale and uses that value as searchText; keep a fallback matchBy that checks
node.viewIdResourceName or accessibility labels instead of literal English text.
Ensure both occurrences (the NEXT block and the Finish installation check) are
changed to use these stable, localized identifiers.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/screens/ProjectSettingsScreen.kt`:
- Around line 57-58: The test is brittle because it looks up dropdown items by
hardcoded English text (calls to d.findObject(UiSelector().text("Kotlin")),
.text("Java"), and .text("Project language")), so replace those text-based
selectors with locale-safe selectors: use resourceId or content-description
selectors (UiSelector().resourceId(...) or UiSelector().description(...)) or
match against the app's localized string values obtained from the app context
(e.g., context.getString(R.string.some_label)) and then use
UiSelector().text(...) with that value; update the three occurrences (the
d.findObject(...) calls and their subsequent check(...) assertions) to use the
resourceId/description or localized string lookup instead of hardcoded English
literals.
In `@build.gradle.kts`:
- Around line 476-478: Normalize both destination and source paths before
performing the nested-path check: convert destination and source to Path (or
absolute Path), call normalize (e.g., toAbsolutePath().normalize()) and then use
startsWith on the normalized destination/path to ensure
`require(!destination.startsWith(source))` can’t be bypassed by `..` segments;
update the check surrounding the existing `require` that references
`destination` and `source` so it compares normalized paths instead of raw
strings.
---
Outside diff comments:
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Line 47: Replace the hardcoded English toolbar labels passed to
clickToolbarButton with the localized string resources: find the calls to
clickToolbarButton("Sync project") and clickToolbarButton("Quick run") and
change them to use the app's resources (e.g.,
context.getString(R.string.sync_project) and
context.getString(R.string.quick_run) or the test's resource accessor), ensuring
you import/use the same R.string identifiers used elsewhere in the file so the
toolbar labels are locale-safe.
---
Duplicate 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 wait for a specific UI/state indicating the
splash finished (e.g., use Espresso.onView with a ViewMatcher for a view in the
next screen and .check(matches(isDisplayed())) or register a
CountingIdlingResource/IdlingRegistry that the SplashActivity signals when
ready), or use ActivityScenario.onActivity to poll the Activity state until the
expected condition is true; target the call sites
ActivityScenario.launch(SplashActivity::class.java) and Thread.sleep(1000) when
making the change.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/helper/ProjectBuildHelper.kt`:
- Around line 31-36: The test setup in
TestContext<Unit>.initializeProjectRunAssembleTasksAndCancelBuild can skip
CloseProjectScenario if an earlier scenario throws; wrap the calls to
scenario(InitializationProjectAndCancelingBuildScenario(...)) and
scenario(RunAssembleTasksScenario()) in a try/finally and invoke
scenario(InitializationProjectAndCancelingBuildScenario.CloseProjectScenario())
from the finally block so CloseProjectScenario always runs even on exceptions;
update the method to call CloseProjectScenario in finally while keeping the same
scenario constructors (InitializationProjectAndCancelingBuildScenario,
RunAssembleTasksScenario, CloseProjectScenario) for identification.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Line 164: Replace the hardcoded literal "Save files and close project" with a
localized string resource: add a key like save_files_and_close_project to the
app/test strings.xml and change the val saveAndClose assignment in
InitializationProjectAndCancelingBuildScenario.kt to fetch the value via a
context-based getString call (e.g., using InstrumentationRegistry or
ApplicationProvider to obtain a Context and then
context.getString(R.string.save_files_and_close_project)) so the test works on
non-English devices.
- Around line 167-171: The UI selector list currently orders closeWithoutSaving
before saveAndClose, risking accidental selection of the “without saving”
action; update the listOf(...) used with firstNotNullOfOrNull so saveAndClose
appears before closeWithoutSaving (i.e., listOf(UiSelector().text(saveAndClose),
UiSelector().textContains("without saving"),
UiSelector().text(closeWithoutSaving))) and apply the same change to the other
occurrences that use the same pattern in
InitializationProjectAndCancelingBuildScenario (the blocks using
listOf(...).firstNotNullOfOrNull with closeWithoutSaving and saveAndClose).
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`:
- Around line 74-76: The current single check only asserts that ":app:assemble"
appears; replace it with a loop over the list of requested tasks (e.g., the
variable holding the selected/requested tasks) and for each task call
check(d.findObject(UiSelector().textContains(task)).waitForExists(10_000)) {
"Run tasks confirmation did not show selected task: $task" } so every requested
task is validated; keep using the existing check(...) helper and
d.findObject(UiSelector().textContains(...)).waitForExists(10_000) pattern.
- Around line 56-63: The test fails when the desired Gradle task is off-screen;
update the tasks.forEach block (the step("Select Gradle task $task") that uses
device.uiDevice and clickFirstAccessibilityNodeParentByText) to first attempt to
scroll the tasks list into view (e.g., use a UiScrollable or other
scroll-into-view call targeting the container) before asserting existence with
d.findObject(UiSelector().text(task)).waitForExists and before calling
clickFirstAccessibilityNodeParentByText(task), so the element is brought
on-screen and the existence check won't false-fail for off-screen rows.
🪄 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: 7559d694-1ea1-4f0d-bee0-0555e4bf758d
📒 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
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt (2)
183-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded close-dialog label with localized resource text.
This literal is locale-fragile and can break close flow on non-English devices.
🤖 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 183, The hardcoded dialog label stored in the variable saveAndClose should be replaced with a localized resource lookup to avoid locale fragility; change the assignment of saveAndClose to fetch the string from resources (e.g., getString(R.string.save_files_and_close) or the appropriate project messages/bundle accessor) and add the corresponding resource key (save_files_and_close) to the strings resource file if it doesn't exist so tests run correctly on non-English devices.
186-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer “save and close” before “without saving”.
Current selector order can choose data-loss action when both buttons are present.
Proposed fix
fun findCloseDialogButton() = listOf( + UiSelector().text(saveAndClose), UiSelector().text(closeWithoutSaving), UiSelector().textContains("without saving"), - UiSelector().text(saveAndClose), ).firstNotNullOfOrNull { selector -> d.findObject(selector).takeIf { it.waitForExists(2_000) && it.exists() } } @@ val closeButton = listOf( + UiSelector().text(saveAndClose), UiSelector().text(closeWithoutSaving), UiSelector().textContains("without saving"), - UiSelector().text(saveAndClose), ).firstNotNullOfOrNull { selector -> d.findObject(selector).takeIf { it.waitForExists(10_000) && it.exists() } } @@ val fallbackCloseButton = listOf( + UiSelector().text(saveAndClose), UiSelector().text(closeWithoutSaving), UiSelector().textContains("without saving"), - UiSelector().text(saveAndClose), ).firstNotNullOfOrNull { selector -> d.findObject(selector).takeIf { it.waitForExists(3_000) && it.exists() } }Also applies to: 279-283, 300-304
🤖 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 186 - 190, The selector list currently prefers the "without saving" match first and may choose the data-loss action; reorder the selector entries so the save-and-close option is checked before the without-saving option by placing saveAndClose (UiSelector().text(saveAndClose)) before UiSelector().textContains("without saving") in the listOf(...) used with firstNotNullOfOrNull, and apply this same reordering for the other occurrences referenced (the blocks using listOf(...) with closeWithoutSaving, UiSelector().textContains("without saving"), saveAndClose at the other two sites).app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt (2)
70-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd scroll-and-retry when selecting Gradle tasks.
Task selection currently assumes the item is already visible. On smaller screens/longer lists this can fail even when the task exists.
Proposed fix
+import androidx.test.uiautomator.UiScrollable import androidx.test.uiautomator.UiSelector @@ 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() } }🤖 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 70 - 77, The test assumes the Gradle task is already visible and directly queries UiSelector().text(task), which fails on small screens; update the task selection in RunAssembleTasksScenario (the tasks.forEach block) to attempt scrolling and retry before failing: use the device.uiDevice UiScrollable approach (e.g., UiScrollable(UiSelector().scrollable(true)).scrollTextIntoView(task)) or loop with repeated calls to scroll and check UiSelector().text(task).waitForExists(timeout) and only call clickFirstAccessibilityNodeParentByText(task) after the element is found; include a small retry count and final assertion message unchanged.
88-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate confirmation against every selected task.
The current confirmation check can pass even if only one task is shown.
Proposed 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 88 - 90, The confirmation check currently only asserts presence of a single ":app:assemble" substring; update the test in RunAssembleTasksScenario to iterate over the actual list of selected tasks (e.g., selectedTasks or tasksToRun) and for each task assert d.findObject(UiSelector().textContains(task)).waitForExists(10_000) with a clear failure message that includes the task name so every selected task is validated. Ensure you use the same task strings used when selecting tasks (including the ":" prefix if present) so the check matches the confirmation UI for each task.app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt (2)
159-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoded onboarding button text.
Using
"Finish installation"directly is locale-fragile; use a resource-backed or stable identifier lookup.🤖 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 159 - 162, Replace the hardcoded button text lookup in the test step that calls clickFirstAccessibilityNodeByText("Finish installation") with a stable, resource-backed lookup: obtain the expected string from app resources (or use a resource ID/contentDescription or accessibility label) and pass that value to clickFirstAccessibilityNodeByText (or switch to a helper that finds by resource id/accessibility id). Update the AutomationEndToEndTest.kt step ("Tap Finish installation") to resolve the string via the app context/resources (or use a contentDescription constant) so the test is locale-robust and not dependent on the literal "Finish installation".
59-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse state-based waiting instead of fixed sleep at launch.
This fixed delay is timing-sensitive and can be flaky.
Proposed fix
step("Launch app") { ActivityScenario.launch(SplashActivity::class.java) - Thread.sleep(1000) + 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 59 - 61, Replace the brittle Thread.sleep(1000) after ActivityScenario.launch(SplashActivity::class.java) with a state-based wait: capture the returned ActivityScenario (val scenario = ActivityScenario.launch(SplashActivity::class.java)) and either use Espresso to wait for a stable UI element (e.g., Espresso.onView(withId(R.id.your_splash_root)).check(matches(isDisplayed()))) or assert the Activity lifecycle state via scenario.onActivity { assert(it.lifecycle.currentState == Lifecycle.State.RESUMED) } so the test proceeds only when SplashActivity is ready.
🤖 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.
Duplicate comments:
In `@app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.kt`:
- Around line 159-162: Replace the hardcoded button text lookup in the test step
that calls clickFirstAccessibilityNodeByText("Finish installation") with a
stable, resource-backed lookup: obtain the expected string from app resources
(or use a resource ID/contentDescription or accessibility label) and pass that
value to clickFirstAccessibilityNodeByText (or switch to a helper that finds by
resource id/accessibility id). Update the AutomationEndToEndTest.kt step ("Tap
Finish installation") to resolve the string via the app context/resources (or
use a contentDescription constant) so the test is locale-robust and not
dependent on the literal "Finish installation".
- Around line 59-61: Replace the brittle Thread.sleep(1000) after
ActivityScenario.launch(SplashActivity::class.java) with a state-based wait:
capture the returned ActivityScenario (val scenario =
ActivityScenario.launch(SplashActivity::class.java)) and either use Espresso to
wait for a stable UI element (e.g.,
Espresso.onView(withId(R.id.your_splash_root)).check(matches(isDisplayed()))) or
assert the Activity lifecycle state via scenario.onActivity {
assert(it.lifecycle.currentState == Lifecycle.State.RESUMED) } so the test
proceeds only when SplashActivity is ready.
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.kt`:
- Line 183: The hardcoded dialog label stored in the variable saveAndClose
should be replaced with a localized resource lookup to avoid locale fragility;
change the assignment of saveAndClose to fetch the string from resources (e.g.,
getString(R.string.save_files_and_close) or the appropriate project
messages/bundle accessor) and add the corresponding resource key
(save_files_and_close) to the strings resource file if it doesn't exist so tests
run correctly on non-English devices.
- Around line 186-190: The selector list currently prefers the "without saving"
match first and may choose the data-loss action; reorder the selector entries so
the save-and-close option is checked before the without-saving option by placing
saveAndClose (UiSelector().text(saveAndClose)) before
UiSelector().textContains("without saving") in the listOf(...) used with
firstNotNullOfOrNull, and apply this same reordering for the other occurrences
referenced (the blocks using listOf(...) with closeWithoutSaving,
UiSelector().textContains("without saving"), saveAndClose at the other two
sites).
In
`@app/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt`:
- Around line 70-77: The test assumes the Gradle task is already visible and
directly queries UiSelector().text(task), which fails on small screens; update
the task selection in RunAssembleTasksScenario (the tasks.forEach block) to
attempt scrolling and retry before failing: use the device.uiDevice UiScrollable
approach (e.g.,
UiScrollable(UiSelector().scrollable(true)).scrollTextIntoView(task)) or loop
with repeated calls to scroll and check
UiSelector().text(task).waitForExists(timeout) and only call
clickFirstAccessibilityNodeParentByText(task) after the element is found;
include a small retry count and final assertion message unchanged.
- Around line 88-90: The confirmation check currently only asserts presence of a
single ":app:assemble" substring; update the test in RunAssembleTasksScenario to
iterate over the actual list of selected tasks (e.g., selectedTasks or
tasksToRun) and for each task assert
d.findObject(UiSelector().textContains(task)).waitForExists(10_000) with a clear
failure message that includes the task name so every selected task is validated.
Ensure you use the same task strings used when selecting tasks (including the
":" prefix if present) so the check matches the confirmation UI for each task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b7f2293-c2d5-43bb-991d-d90b221ed1bd
📒 Files selected for processing (3)
app/src/androidTest/kotlin/com/itsaky/androidide/AutomationEndToEndTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/InitializationProjectAndCancelingBuildScenario.ktapp/src/androidTest/kotlin/com/itsaky/androidide/scenarios/RunAssembleTasksScenario.kt
jomen-adfa
force-pushed
the
task/ADFA-3961-avd-generate-cache
branch
from
87d71d4 to
16e79fb
Compare
jatezzz
requested changes
May 26, 2026
Collaborator
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Please use constants for hardcoded numbers, like 2_000
jatezzz
approved these changes
May 26, 2026
jomen-adfa
force-pushed
the
task/ADFA-3961-avd-generate-cache
branch
from
2941526 to
53bb953
Compare
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
2 participants
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.