[BUG] Fix KotlinReflectionInternalError for value-class @PreviewParameter previews - #142
Conversation
A @Preview whose @PreviewParameter provider yields a Kotlin value class (e.g. PreviewParameterProvider<Dp>) crashed both getPreviews() and invocation. The scanner reflected/invoked the composable via kotlin-reflect, whose ValueClassAwareCaller asserts the Kotlin-descriptor arity equals the JVM method arity. That fails for @composable functions because the Compose compiler appends synthetic Composer/changed/default-mask parameters that aren't in the descriptor (3 != 1). Non-value-class previews skip that caller, which is why only value-class previews crashed. Switch both paths to androidx.compose.runtime.reflect.ComposableMethod (java.lang.reflect, Compose-ABI aware), which accounts for the synthetic params: - ProvideComposablePreview.methodParametersTypeAsString: derive the real parameter count via asComposableMethod() instead of kotlinFunction. - ComposablePreviewInvocationHandler: invoke via ComposableMethod.invoke, unboxing value-class arguments; preserve the PreviewWrapper path, bound-instance invocation, and the "@PreviewParameter must be first" guard. Add a PreviewParameterProvider<Dp> fixture (in its own top-level package) with an enumeration/toString test and a Paparazzi render test; the suite previously had no value-class coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThe scanner now uses Compose-aware reflection for preview parameter counting and composable invocation. It supports boxed Kotlin value-class parameters, validates parameter placement, recognizes both Compose ChangesValue-class preview parameter support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PreviewParameterProvider
participant ComposablePreviewInvocationHandler
participant ComposableMethod
participant PreviewComposable
PreviewParameterProvider->>ComposablePreviewInvocationHandler: provide Dp value
ComposablePreviewInvocationHandler->>ComposableMethod: invokeComposable with Composer, changed, and value
ComposableMethod->>PreviewComposable: supply synthetic arguments and invoke
PreviewComposable-->>ComposableMethod: render preview content
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
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
`@core/src/main/java/sergio/sastre/composable/preview/scanner/core/preview/ComposablePreviewInvocationHandler.kt`:
- Around line 72-88: Update the invocation flow around
ComposablePreviewInvocationHandler and ComposableMethod.invoke so a
provider-emitted null for a nullable first `@PreviewParameter` is passed
explicitly rather than interpreted as a request to use the Kotlin default.
Preserve existing coercion and no-parameter behavior, and add rendering coverage
for a nullable preview parameter with a default receiving explicit null.
In
`@core/src/main/java/sergio/sastre/composable/preview/scanner/core/preview/ProvideComposablePreview.kt`:
- Around line 69-74: Update the fallback calculation in the preview parameter
extraction flow around previewMethod and realParametersCount to determine the
real parameter count from the index of the last Composer parameter, rather than
subtracting two from parameterTypes.size. Preserve the asComposableMethod()
count when available and use the resulting count for genericParameterTypes.take
so synthetic mask parameters are excluded.
🪄 Autofix
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 Plus
Run ID: d7a6de7f-923a-407c-bee1-8a5ffb9dd460
⛔ Files ignored due to path filters (2)
tests/src/test/snapshots/images/Paparazzi_Preview_Test_valueclass.previewparameters.android.composableskt.valueclasspreviewparameterpreview-8feqmps_float_0.pngis excluded by!**/*.pngtests/src/test/snapshots/images/Paparazzi_Preview_Test_valueclass.previewparameters.android.composableskt.valueclasspreviewparameterpreview-8feqmps_float_1.pngis excluded by!**/*.png
📒 Files selected for processing (5)
core/src/main/java/sergio/sastre/composable/preview/scanner/core/preview/ComposablePreviewInvocationHandler.ktcore/src/main/java/sergio/sastre/composable/preview/scanner/core/preview/ProvideComposablePreview.kttests/src/main/java/valueclass/previewparameters/android/Composables.kttests/src/test/java/sergio/sastre/composable/preview/scanner/tests/api/main/screenshotid/ComposablePreviewToStringTest.kttests/src/test/java/sergio/sastre/composable/preview/scanner/tests/paparazzi/runtime/PaparazziValueClassPreviewParameterInvokeTest.kt
|
@nshurtz Thanks for reporting the issue and opening a PR! The only thing I am missing is to check that the changes work also in Roborazzi. The new Composable sample that is used to reproduce the bug, seems that it is placed in a package that the Roborazzi tests do not scan. That’s also why a new Parameterized test was needed, the Roborazzi one is missing though. FYI, I am currently on holidays till the end of the week, but I promise to do the final check and release a bugfix version as soon as I am back. Sorry for the inconvenience! 🙏 |
Mirrors the Paparazzi test so the value-class @PreviewParameter fix is verified under both rendering libraries, which download resources differently. Scans the valueclass.previewparameters.android package and records reference snapshots. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@tests/src/test/java/sergio/sastre/composable/preview/scanner/tests/roborazzi/runtime/RoborazziValueClassPreviewParameterInvokeTest.kt`:
- Around line 36-44: Update the values() parameter provider to check that
cachedPreviews is not empty before returning it, using
check(cachedPreviews.isNotEmpty()) so the test fails immediately when no
previews are discovered.
🪄 Autofix
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 Plus
Run ID: edb736f5-0a98-4569-8285-cf564e60d29f
⛔ Files ignored due to path filters (2)
tests/src/test/screenshots/valueclass/valueclass.previewparameters.android.ComposablesKt.ValueClassPreviewParameterPreview-8Feqmps_float_0.pngis excluded by!**/*.pngtests/src/test/screenshots/valueclass/valueclass.previewparameters.android.ComposablesKt.ValueClassPreviewParameterPreview-8Feqmps_float_1.pngis excluded by!**/*.png
📒 Files selected for processing (1)
tests/src/test/java/sergio/sastre/composable/preview/scanner/tests/roborazzi/runtime/RoborazziValueClassPreviewParameterInvokeTest.kt
|
For context on the boundaries of this fix: while verifying, I found that Android Studio's own preview renderer ( I'm filing that (and the separate |
- For PreviewParameters Value-classes that are not nullable, it returns the underlying type. This returns the Value class name too for consistency - Adds coverage for other types of PreviewParameter classes
I just moved the tests to keep the current structure, ensure that the name includes the Value Class name instead of the one of the underlying class, and add some extra tests to cover several @PreviewParameter naming edge cases. I'll release a bugfix version as soon as I find the time, likely today or tomorrow. Thank you very much for your contribution! |
2 participants
Fixes #141
Problem
A
@Previewwhose@PreviewParameterprovider yields a Kotlin value class crashed bothgetPreviews()andComposablePreview.invoke()withKotlinReflectionInternalError: Inconsistent number of parameters … arity != expectedArgsSize. The scanner resolved the preview method via kotlin-reflect, whoseValueClassAwareCallerasserts the Kotlin-descriptor arity equals the JVM arity — which never holds for@Composablefunctions, because the Compose compiler appends syntheticComposer/changed/default-mask parameters absent from the descriptor. Only value-class previews are routed through that caller, so only they crashed.Fix
Switch both paths off kotlin-reflect onto
androidx.compose.runtime.reflect.ComposableMethod(java.lang.reflect-based, Compose-ABI aware):ProvideComposablePreview.methodParametersType— derive the real parameter count viaasComposableMethod()instead ofkotlinFunction.ComposablePreviewInvocationHandler— invoke viaComposableMethod.invoke, unboxing value-class arguments; preserves thePreviewWrapperpath, bound-instance invocation, and the "@PreviewParametermust be first" guard.Tests
PreviewParameterProvider<Dp>fixture in its own top-level package (so the broad invoke suites don't pick it up).toStringtest asserting the value-class preview is scanned without crashing.Notes
The scanned id keeps the raw JVM method name, which for a value-class parameter is mangled (
<name>-<hash>); this is intentional so previews whose value-class params share an underlying JVM type don't collide (see the comment inComposablePreviewToStringTest).This contribution (code and description) was prepared with AI assistance (Claude Code) and reviewed by me before submission.
Summary by CodeRabbit
New Features
Bug Fixes
Tests