Introduce tool windows for the compose stability analysis #27
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an IntelliJ Compose Stability Tool Window and collector, integrates kotlinx.serialization for emitting stability reports, refines compiler stability analysis (suspend detection, sealed and Java mutable types), bumps versions to 0.4.3, and updates a sample app's composable signatures and stability manifest. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ToolWindow as Stability Tool Window
participant Collector as ComposableStabilityCollector
participant Parser as Json (kotlinx.serialization)
participant PSI as Kotlin PSI
participant UI as Tree/Details UI
User->>ToolWindow: Open / Click Refresh
ToolWindow->>Collector: collectAll()
activate Collector
Collector->>Collector: iterate modules, locate build/.../stability-info.json
Collector->>Parser: parse JSON -> composable list
Parser-->>Collector: composable entries
Collector->>PSI: findSourceLocation(qualifiedName)
PSI-->>Collector: file path, file name, line
Collector-->>ToolWindow: ComposableStabilityResults (list + stats)
deactivate Collector
ToolWindow->>UI: build/refresh tree, apply filter
UI-->>User: display tree + details
User->>UI: double-click composable
UI->>PSI: open file and position caret
PSI-->>User: file opened at line
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/stability/app.stability (1)
158-162: Fix StringBuilder stability classification—it should be marked UNSTABLE, not STABLE.StringBuilder and other mutable Java types are considered unstable by the Compose compiler because it cannot assume their contents won't change without notification. The only ways to treat a mutable type like StringBuilder as stable are: convert it to an immutable String, wrap it in Compose state (mutableStateOf), create a stable adapter type, or use the compiler stability-configuration file. Marking it STABLE without one of these patterns will cause unnecessary recomposition and bypass Compose's skipping optimization, degrading performance.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/skydoves/myapplication/MainActivity.kt (1)
298-314: Consider grouping related parameters for clarity.The new parameters expand test coverage for sealed class variants and additional FontWeight scenarios. However, the parameter ordering could be more intuitive—grouping related types together (e.g., all FontWeight parameters, all MySealed variants) would improve readability even in test code.
Optional refactor to group related parameters:
@Composable fun <T> GenericDisplay( item: T, - fontWeight4: FontWeight, mySealed: MySealed, + child: MySealed.Child, child2: MySealed.Child2, child3: MySealed.Child3, child4: MySealed.Child4, - child: MySealed.Child, fontWeight: FontWeight, + fontWeight4: FontWeight, fontWeight2: FontWeight?, values: kotlin.String, value: Int?, testValueClass: TestValueClass, textAlign: TextAlign, textAlign2: TextAlign?, displayText: (T) -> String, ) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
compose-stability-analyzer-idea/src/main/resources/META-INF/pluginIcon.svgis excluded by!**/*.svgcompose-stability-analyzer-idea/src/main/resources/META-INF/pluginIcon_dark.svgis excluded by!**/*.svgcompose-stability-analyzer-idea/src/main/resources/icons/stability.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
app/src/main/kotlin/com/skydoves/myapplication/MainActivity.kt(4 hunks)app/stability/app.stability(1 hunks)compose-stability-analyzer-idea/CHANGELOG.md(1 hunks)compose-stability-analyzer-idea/api/compose-stability-analyzer-idea.api(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/StabilityNodeData.kt(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/StabilityToolWindow.kt(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/StabilityToolWindowFactory.kt(1 hunks)compose-stability-analyzer-idea/src/main/resources/META-INF/plugin.xml(1 hunks)gradle.properties(1 hunks)gradle/libs.versions.toml(1 hunks)stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt(10 hunks)stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt (2)
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt (2)
analyzeClassProperties(410-493)analyzeSuperclassStability(500-544)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/StabilityAnalyzer.kt (1)
analyzeClassProperties(886-944)
🪛 detekt (1.23.8)
stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt
[warning] 403-403: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt
[warning] 120-120: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 165-165: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 174-174: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Tests
🔇 Additional comments (4)
app/src/main/kotlin/com/skydoves/myapplication/MainActivity.kt (2)
155-155: LGTM! Expands suspend function stability test coverage.The addition of
func3with a suspend function returningStableUsercomplements the existingfunc2parameter and helps test the analyzer's handling of suspend functions with different return type stability.
284-287: LGTM! Well-designed sealed class stability test cases.The new
Child3(with stable property) andChild4(with unstable property) provide contrasting test cases for sealed class stability detection, complementing the existing empty child classes.app/stability/app.stability (2)
134-140: LGTM! New parameter correctly classified as STABLE.The addition of the
immutableListparameter with stability classification as STABLE is correct.ImmutableListfrom kotlinx.collections.immutable is indeed a stable type.
16-26: Report appears incomplete; func3 parameter missing from stability analysis.The stability report lists only 3 parameters for the
Cardcomposable (func, func2, content), but the source code defines 4 parameters (func, func2, func3, content). The criticalfunc3: suspend () -> StableUseris absent from the report.Current observations:
func(@composable returning UnstableUser) → marked STABLEfunc2(suspend returning UnstableUser) → marked RUNTIMEfunc3(suspend returning StableUser) → not listed in reportSuspend functions should be treated as stable by default by the Compose compiler, similar to regular lambdas, unless their captured values or return types make them unstable at runtime. However, the report shows
func2as RUNTIME despite being a function type.The missing
func3would clarify whether the RUNTIME classification is due to: (1) suspend functions inherently requiring runtime checks, or (2) the UnstableUser return type.
| } catch (e: Exception) { | ||
| // Skip modules that fail to parse | ||
| } |
There was a problem hiding this comment.
Don't swallow the JSON parsing failure.
Right now the catch block ignores every exception, so a bad or missing stability-info.json silently wipes the whole module from the tool window with zero diagnostics. At minimum emit a warning (or rethrow) so users can tell why results are incomplete. For example:
- } catch (e: Exception) {
- // Skip modules that fail to parse
- }
+ } catch (e: Exception) {
+ logger.warn("Failed to parse stability metadata from ${jsonFile.path}", e)
+ }(and add a private val logger = Logger.getInstance(ComposableStabilityCollector::class.java) near the top). Based on static analysis hints.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 120-120: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt
around lines 120-122, the catch block currently swallows all Exceptions when
parsing stability-info.json; change it to log the failure instead of ignoring
it: add a private val logger =
Logger.getInstance(ComposableStabilityCollector::class.java) near the top of the
class and, in the catch, call logger.warn or logger.error with a clear message
including the module name/path and the caught exception (or rethrow the
exception after logging if you prefer failing fast) so users and devs can see
why a module was skipped.
| return Triple("Unknown", "Unknown.kt", 0) | ||
| } catch (e: Exception) { | ||
| return Triple("Unknown", "Unknown.kt", 0) | ||
| } |
There was a problem hiding this comment.
Surface source-resolution failures instead of hiding them.
When PSI lookups throw, we currently just return "Unknown" and discard the exception, which again makes the UI incomplete with no clue why navigation stopped working. Please log the failure (same logger as above) before returning the fallback so users can act on it. Based on static analysis hints.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 174-174: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/ComposableStabilityCollector.kt
around lines 173-176, the catch block swallows exceptions and returns a fallback
Triple("Unknown", "Unknown.kt", 0); change it to log the caught exception with
the same logger used earlier (e.g., logger.error("Failed to resolve source for
composable", e)) immediately before returning the fallback so failures are
visible for debugging and UI feedback.
| val virtualFile = com.intellij.openapi.vfs.LocalFileSystem.getInstance() | ||
| .findFileByPath(composable.filePath) ?: return | ||
|
|
||
| ApplicationManager.getApplication().invokeLater { | ||
| val psiFile = PsiManager.getInstance(project).findFile(virtualFile) ?: return@invokeLater | ||
| val document = com.intellij.psi.PsiDocumentManager.getInstance(project).getDocument(psiFile) | ||
| ?: return@invokeLater | ||
|
|
||
| val offset = if (composable.line > 0) { | ||
| document.getLineStartOffset(composable.line - 1) | ||
| } else { | ||
| 0 | ||
| } | ||
|
|
||
| FileEditorManager.getInstance(project).openFile(virtualFile, true) | ||
| val editor = FileEditorManager.getInstance(project).selectedTextEditor | ||
| if (editor != null && editor.document == document) { | ||
| editor.caretModel.moveToOffset(offset) | ||
| editor.scrollingModel.scrollToCaret(com.intellij.openapi.editor.ScrollType.CENTER) | ||
| } |
There was a problem hiding this comment.
Guard against stale line numbers before calling getLineStartOffset.
document.getLineStartOffset(composable.line - 1) will throw IndexOutOfBoundsException if the stability dump points past the current document length (for example, the file changed after the dump). That surfaces as an IDE error on double-click instead of opening the file. Please clamp the line index to document.lineCount - 1 (and bail out when there are no lines) before computing the offset. (intellij-support.jetbrains.com)
🤖 Prompt for AI Agents
In
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/toolwindow/StabilityToolWindow.kt
around lines 170 to 189, the call document.getLineStartOffset(composable.line -
1) can throw IndexOutOfBoundsException if composable.line is past the current
document length; clamp the requested line index by first getting val lineCount =
document.lineCount and if lineCount == 0 return/bail out, otherwise compute val
safeLine = when { composable.line <= 0 -> 0; composable.line - 1 >= lineCount ->
lineCount - 1; else -> composable.line - 1 } and pass safeLine to
getLineStartOffset, keeping the existing behavior for opening the file and
moving the caret.
| // Check for suspend functions using multiple methods | ||
| val typeString = type.render() | ||
| val isSuspend = type.isSuspendFunctionTypeOrSubtype() || | ||
| (fqName?.startsWith("kotlin.coroutines.SuspendFunction") == true) || | ||
| (fqName?.contains("SuspendFunction") == true) || | ||
| typeString.startsWith("suspend ") || | ||
| typeString.contains("SuspendFunction") | ||
|
|
||
| if (isSuspend) { | ||
| return ParameterStability.STABLE | ||
| } |
There was a problem hiding this comment.
Don't treat arbitrary SuspendFunction* class names as suspend functions.
fqName?.contains("SuspendFunction") (and the similar typeString.contains) matches any class whose FQCN merely contains that substring—e.g. com.example.SuspendFunctionality or com.foo.SuspendFunctionAdapter. Those unrelated types will now be marked ParameterStability.STABLE, so we skip property analysis and can silently miss real instability. We already have the precise isSuspendFunctionTypeOrSubtype() API plus the explicit startsWith("kotlin.coroutines.SuspendFunction") guard; we only need a bounded string fallback for the literal suspend keyword (after trimming any leading parens). Please drop the broad substring checks. A safe tweak could look like:
- val isSuspend = type.isSuspendFunctionTypeOrSubtype() ||
- (fqName?.startsWith("kotlin.coroutines.SuspendFunction") == true) ||
- (fqName?.contains("SuspendFunction") == true) ||
- typeString.startsWith("suspend ") ||
- typeString.contains("SuspendFunction")
+ val normalizedTypeString = typeString
+ .dropWhile { it == '(' || it.isWhitespace() }
+ val isSuspend = type.isSuspendFunctionTypeOrSubtype() ||
+ (fqName?.startsWith("kotlin.coroutines.SuspendFunction") == true) ||
+ normalizedTypeString.startsWith("suspend ") ||
+ normalizedTypeString.startsWith("kotlin.coroutines.SuspendFunction")This keeps suspend detection accurate without introducing false positives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for suspend functions using multiple methods | |
| val typeString = type.render() | |
| val isSuspend = type.isSuspendFunctionTypeOrSubtype() || | |
| (fqName?.startsWith("kotlin.coroutines.SuspendFunction") == true) || | |
| (fqName?.contains("SuspendFunction") == true) || | |
| typeString.startsWith("suspend ") || | |
| typeString.contains("SuspendFunction") | |
| if (isSuspend) { | |
| return ParameterStability.STABLE | |
| } | |
| // Check for suspend functions using multiple methods | |
| val typeString = type.render() | |
| val normalizedTypeString = typeString | |
| .dropWhile { it == '(' || it.isWhitespace() } | |
| val isSuspend = type.isSuspendFunctionTypeOrSubtype() || | |
| (fqName?.startsWith("kotlin.coroutines.SuspendFunction") == true) || | |
| normalizedTypeString.startsWith("suspend ") || | |
| normalizedTypeString.startsWith("kotlin.coroutines.SuspendFunction") | |
| if (isSuspend) { | |
| return ParameterStability.STABLE | |
| } |
Introduce tool windows for the compose stability analysis.
Summary by CodeRabbit
New Features
Improvements
Chores