Store stability output of Android variants to their own folder#150
Store stability output of Android variants to their own folder#150matejdro wants to merge 2 commits into
Conversation
|
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:
WalkthroughStabilityAnalyzerGradlePlugin refactors stability output directory naming using a new ChangesPer-Task Stability Directory Naming
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt`:
- Around line 291-295: The IDE tool window still only reads the flat path
"build/stability/stability-info.json" so after StabilityAnalyzerGradlePlugin.kt
started writing variant outputs into "stability/<variant>/stability-info.json"
the collector stops seeing Android results; update ComposableStabilityCollector
(class/method that scans build/stability) to either (a) enumerate the
build/stability directory and read stability-info.json from every subdirectory
(e.g., stability/<variant>/stability-info.json) or (b) fall back to the legacy
root file if present—locate the scan logic around the method that currently
constructs the path "build/stability/stability-info.json" and change it to walk
child directories or attempt both locations so both legacy and variant outputs
are discovered.
- Around line 129-135: The KotlinCompile task filters currently hardcode false
in the calls to isKotlinTaskApplicable, causing test compile tasks to be treated
as non-writing when stabilityValidation.includeTests is enabled; change those
calls so they pass the actual includeTests flag (the same boolean used for
stabilityValidation) instead of false in the
target.tasks.withType(KotlinCompile::class.java).named {
isKotlinTaskApplicable(it, false) } blocks (both the block that sets
outputs.dir(stabilityDir).optional(true) and the similar block at the later
location), so isKotlinTaskApplicable receives the real includeTests value and
test compile tasks correctly declare their build/stability outputs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c30514d1-dc51-40cc-8ad8-c5204ecc728a
📒 Files selected for processing (1)
stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
| val stabilityFolderName = if (variant.isBlank()) { | ||
| "stability" | ||
| } else { | ||
| "stability/$variant" | ||
| } |
There was a problem hiding this comment.
Update the IDEA collector before moving Android outputs.
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt lines 60-65 still only scan build/stability/stability-info.json. After this change, Android builds emit build/stability/<variant>/stability-info.json, so the tool window will stop discovering fresh Android results unless it also walks the variant subdirectories or you keep a compatibility file in the root folder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt`
around lines 291 - 295, The IDE tool window still only reads the flat path
"build/stability/stability-info.json" so after StabilityAnalyzerGradlePlugin.kt
started writing variant outputs into "stability/<variant>/stability-info.json"
the collector stops seeing Android results; update ComposableStabilityCollector
(class/method that scans build/stability) to either (a) enumerate the
build/stability directory and read stability-info.json from every subdirectory
(e.g., stability/<variant>/stability-info.json) or (b) fall back to the legacy
root file if present—locate the scan logic around the method that currently
constructs the path "build/stability/stability-info.json" and change it to walk
child directories or attempt both locations so both legacy and variant outputs
are discovered.
✅ Addressed in commit b61d18a
There was a problem hiding this comment.
Hm that's a good point, it would break IDEA plugin. @skydoves how should we handle this? Would we add a variant property to the idea plugin?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
What I think we could do is, have Intellij plugin scan the stability folder + all the subfolders for the stability-info.json. That way it detects the json file regardless of the variant.
There was a problem hiding this comment.
I have added check-all-folders, however it is untested. I'm not sure how can I build the IntelliJ plugin, it does not appear to be documented. Or did I miss it?
matejdro
force-pushed
the
android-variants-separate-folder
branch
from
8611989 to
b61d18a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt`:
- Around line 125-126: In ComposableStabilityCollector.kt, the catch (e:
Exception) that swallows parse failures for stability-info.json should log the
failing folder path and exception instead of ignoring it; update the catch to
call the module/class logger (or Logger.getInstance(...).error) including the
folder or file path being parsed and e (stacktrace/message) so users can
distinguish "no output" from "failed to read/generated" when stability-info.json
parsing fails.
- Around line 63-64: The collector currently flattens variantFolders +
rootStabilityFolder into allStabilityFolders and loses variant identity, causing
duplicate composables to be indistinguishable and StabilityStats to overcount;
update ComposableStabilityCollector to either (A) carry variant metadata with
each folder entry (e.g., change the collection element to a data class or pair
like (variantName, folder) and propagate that through the map/reduce that builds
StabilityStats and the UI model) or (B) deduplicate exact composable entries
before computing the summary (compare by canonical path/signature and collapse
identical entries) and apply the same change to the other aggregation sites
around the blocks that build the stats and UI model so counts reflect unique
composables per variant or globally as intended.
- Around line 61-64: The loop currently builds allStabilityFolders from
rootStabilityFolder ("build/stability") only, so non-Android test outputs
written to "build/stabilityTest/stability-info.json" are missed; update the
collector to also consider "build/stabilityTest" (e.g., create a
rootStabilityTest = File(contentRoot.path, "build/stabilityTest") and include
its directory plus its variant children into allStabilityFolders), and only
add/scan that folder when the includeTests flag/property is enabled; keep
references to rootStabilityFolder, rootStabilityTest, variantFolders,
allStabilityFolders, and the existing folder loop so the new folder is treated
the same as the original.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b3523a1-aec5-4bfc-aa92-01021269f4ba
📒 Files selected for processing (2)
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.ktstability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
| val allStabilityFolders = variantFolders + rootStabilityFolder | ||
| for (folder in allStabilityFolders) { |
There was a problem hiding this comment.
Preserve variant identity or dedupe before building stats.
The collector now merges debug, release, *UnitTest, etc. into one flat list but throws away the folder/variant name. Shared composables will show up as indistinguishable duplicates, and StabilityStats will overcount them. Please either carry the variant through the model/UI or collapse exact duplicates before computing the summary.
Also applies to: 110-123, 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt`
around lines 63 - 64, The collector currently flattens variantFolders +
rootStabilityFolder into allStabilityFolders and loses variant identity, causing
duplicate composables to be indistinguishable and StabilityStats to overcount;
update ComposableStabilityCollector to either (A) carry variant metadata with
each folder entry (e.g., change the collection element to a data class or pair
like (variantName, folder) and propagate that through the map/reduce that builds
StabilityStats and the UI model) or (B) deduplicate exact composable entries
before computing the summary (compare by canonical path/signature and collapse
identical entries) and apply the same change to the other aggregation sites
around the blocks that build the stats and UI model so counts reflect unique
composables per variant or globally as intended.
There was a problem hiding this comment.
That is a good point, but I feel extra UI is out of scope for this PR. What do you think @skydoves?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt`:
- Around line 87-92: The code uses stabilityInputFiles.setFrom(...) twice which
causes the second call to overwrite the first; change the second invocation to
stabilityInputFiles.from(target.layout.buildDirectory.file("stability/test/stability-info.json"))
so the test file is appended instead of replacing the initial
stability/stability-info.json entry, ensuring stabilityInputFiles contains both
files.
- Around line 175-187: The stabilityInputFiles property is being overwritten by
repeated setFrom() calls so only the last file (AndroidTest) is kept; update the
code that sets stabilityInputFiles (references: stabilityInputFiles and
variant.name in StabilityAnalyzerGradlePlugin.kt) to aggregate all three files
instead of replacing them—use a single setFrom(...) with a collection of files
or call from(...) / addAll(...) for each file (e.g., gather the three
target.layout.buildDirectory.file(...) results into a list and pass that to
stabilityInputFiles.setFrom or call stabilityInputFiles.from(...) for each) so
main variant, UnitTest and AndroidTest files are all included.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d43cb7f-227f-42bc-bdd4-4819296aee53
📒 Files selected for processing (1)
stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
|
@skydoves any inputs here? |
|
Hm, it looks like this might be superseded by the #154? |
|
Hi @matejdro, thanks for the PR! I worked on a broader fix for this as part of #153, which also covers implicit dependency failures with Kover/Gradle 9.x, mustRunAfter cleanup, and IDE backward compatibility. But I really like your variant-based naming (build/stability/debug vs build/stability/compileDebugKotlin) much cleaner. Would you like to rebase this PR on top of the #153 fix and focusing on just the variant naming improvement? That way we can incorporate your approach without duplicating the rest. One quick note: the setFrom() calls for Android variants overwrite each other (only the last path survives) - should be from() for additive behavior. |
matejdro
force-pushed
the
android-variants-separate-folder
branch
from
56b51f0 to
bfc2f27
Compare
|
Updated. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt (1)
107-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-Android input glob misses the root
stability-info.json.At Line 108 and Line 126,
include("*/stability-info.json")won’t matchbuild/stability/stability-info.json(thecompileKotlinoutput folder). That can leave dump/check with no main input on JVM/non-Android projects.Suggested fix
stabilityInputFiles.setFrom( target.fileTree(target.layout.buildDirectory.dir("stability")) { - include("*/stability-info.json") + include("**/stability-info.json") }, ) @@ stabilityInputFiles.from( target.fileTree(target.layout.buildDirectory.dir("stability")) { - include("*/stability-info.json") + include("**/stability-info.json") }, )Also applies to: 125-127
🤖 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 `@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt` around lines 107 - 109, The include glob currently used in StabilityAnalyzerGradlePlugin (the target.fileTree { include("*/stability-info.json") } calls) misses files directly under build/stability (e.g., stability-info.json) for non-Android projects; update those include calls to match both root and nested files — either use include("**/stability-info.json") or list both include("stability-info.json", "*/stability-info.json") — and apply the same change to the second occurrence of target.fileTree/include in the file so the dump/check tasks always pick up build/stability/stability-info.json on JVM projects.
♻️ Duplicate comments (1)
stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt (1)
177-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAndroid dump/check currently ignore test stability files when
includeTests=true.At Line 179 and Line 198, inputs are fixed to
stability/$variantNameLowerCase/stability-info.jsononly. But test compilations (enabled viaincludeTests) write to variant test folders, so those results are dropped from dump/check inputs.Suggested fix
val stabilityDumpTask = target.tasks.register( "${variantNameLowerCase}StabilityDump", StabilityDumpTask::class.java, ) { projectName.set(target.name) - stabilityInputFiles.setFrom( - target.layout.buildDirectory.file( - "stability/$variantNameLowerCase/stability-info.json", - ), - ) + stabilityInputFiles.setFrom( + target.fileTree(target.layout.buildDirectory.dir("stability")) { + include("$variantNameLowerCase/stability-info.json") + include("${variantNameLowerCase}UnitTest/stability-info.json") + include("${variantNameLowerCase}AndroidTest/stability-info.json") + }, + ) @@ val stabilityCheckTask = target.tasks.register( "${variantNameLowerCase}StabilityCheck", StabilityCheckTask::class.java, ) { projectName.set(target.name) - stabilityInputFiles.from( - target.layout.buildDirectory.file( - "stability/$variantNameLowerCase/stability-info.json", - ), - ) + stabilityInputFiles.from( + target.fileTree(target.layout.buildDirectory.dir("stability")) { + include("$variantNameLowerCase/stability-info.json") + include("${variantNameLowerCase}UnitTest/stability-info.json") + include("${variantNameLowerCase}AndroidTest/stability-info.json") + }, + )Also applies to: 196-199
🤖 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.
Outside diff comments:
In
`@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt`:
- Around line 107-109: The include glob currently used in
StabilityAnalyzerGradlePlugin (the target.fileTree {
include("*/stability-info.json") } calls) misses files directly under
build/stability (e.g., stability-info.json) for non-Android projects; update
those include calls to match both root and nested files — either use
include("**/stability-info.json") or list both include("stability-info.json",
"*/stability-info.json") — and apply the same change to the second occurrence of
target.fileTree/include in the file so the dump/check tasks always pick up
build/stability/stability-info.json on JVM projects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a651b24e-c60b-4041-92ff-9f6740a1d8f6
📒 Files selected for processing (1)
stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
2 participants
🎯 Goal
A continuation of the #101
Up until now, every Kotlin task dropped its stability output into
build/stability. This was problematic with multi-variant runs:PR fixes this by changing plugin to store stability output into
build/stability/$VARIANT(such as/build/stability/debug)Summary by CodeRabbit