feat(snapshot): Merge snapshot extension into main sentry DSL#1136
feat(snapshot): Merge snapshot extension into main sentry DSL#1136runningcode merged 16 commits intomainfrom
Conversation
Move snapshot configuration from the standalone `sentrySnapshot` plugin
into the main `sentry { snapshots { } }` extension. This consolidates
the DSL surface and auto-detects Paparazzi for test generation.
- Add `SnapshotsExtension` nested under `SentryPluginExtension`
- Move Paparazzi detection and test generation wiring into `SentryPlugin`
- Wire `sentryUploadSnapshots` task to Paparazzi output directory
- Rename generate task to `sentryGenerateSnapshotsTests` for consistency
- Remove standalone `SentrySnapshotPlugin` and `SentrySnapshotExtension`
- Use plural "snapshots" naming consistently
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
|
Use dependsOn to ensure Paparazzi record task runs before upload, and derive snapshotsPath from the test task's output directory instead of a hardcoded path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
romtsn
left a comment
There was a problem hiding this comment.
LGTM, but I guess I'd still prefer to have an enabled flag -- in case something goes off with the snapshots' tasks, people that don't use snapshots could simply disable the feature. Because now we're configuring those tasks unconditionally, right (even though they don't execute without the paparazzi plugin applied)?
Gate test generation and dependency addition on snapshots.enabled so that applying Paparazzi alongside the Sentry plugin doesn't affect normal Paparazzi task execution. Defaults to false. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Actually, that's a really good point. I built it so that all the tasks are only registered and not dependencies of other tasks unless you have the paparazzi plugin applied. |
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Outdated
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Outdated
Show resolved
Hide resolved
Remove the sentrySnapshotPlugin marker distribution and publication task references that were left behind when the plugin was removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The upload task and its Paparazzi wiring were registered unconditionally for all variants. This caused projects using both the Sentry plugin and Paparazzi to get unwanted task dependencies even with snapshots disabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryPlugin.kt
Outdated
Show resolved
Hide resolved
testImplementation is a global configuration, not per-variant. Move the ComposablePreviewScanner dependency addition to afterEvaluate (gated on enabled) so it runs once instead of once per variant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The testDebugUnitTest task is not yet registered by AGP when onVariants runs. Wrap the Paparazzi wiring in afterEvaluate so the test task exists when we look it up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tsConfig Move test generation, dependency addition, and upload task wiring into configureSnapshotsTasks so all snapshot logic lives in one place. Use the AGP hostTests/unitTest API for generated source wiring, and afterEvaluate for the test task output lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| "testImplementation", | ||
| "io.github.sergio-sastre.ComposablePreviewScanner:android:0.8.1", | ||
| ) |
There was a problem hiding this comment.
Bug: The code unsafely calls .first() to find a "snapshots" directory, which will crash the build if the directory is not found.
Severity: MEDIUM
Suggested Fix
Replace the call to .first() with a safer alternative like .firstOrNull(). If the result is null (meaning the directory was not found), you should handle it gracefully, for example by logging a warning or skipping the task's operation, to prevent the build from crashing.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt#L495-L497
Potential issue: The code at line 526 calls `.first()` on a collection of files to find
one named "snapshots". If the Paparazzi test task runs but does not produce a
"snapshots" output directory for any reason, this call will throw a
`NoSuchElementException` during Gradle's execution phase, causing the build to fail.
This can happen if the Paparazzi configuration is altered or if it behaves unexpectedly
for a specific variant.
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Show resolved
Hide resolved
Not all variants have unit test tasks (e.g. users can disable them), so skip wiring the snapshot upload task if the test task doesn't exist. Uses tasks.names to avoid eagerly realizing tasks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Outdated
Show resolved
Hide resolved
Adding the testImplementation dependency inside a nested afterEvaluate risks the configuration already being resolved and immutable. The withPlugin callback already fires during configuration, so the wrapper is unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Show resolved
Hide resolved
The testImplementation dependency for ComposablePreviewScanner was being added N times (once per variant) since configureSnapshotsTasks runs inside onVariants. Move it to a single withPlugin block before onVariants to add it only once. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oop" This reverts commit f77e724.
| project.afterEvaluate { | ||
| // Not all variants have unit test tasks (e.g. users can disable them), |
There was a problem hiding this comment.
Bug: The SentryUploadSnapshotsTask is registered unconditionally but its required snapshotsPath property is not set if the Paparazzi plugin is missing or if unit tests are disabled for a variant, causing a build failure.
Severity: HIGH
Suggested Fix
Move the registration of SentryUploadSnapshotsTask inside the project.pluginManager.withPlugin("app.cash.paparazzi") block. This ensures the task is only created when its required snapshotsPath property can be correctly configured, preventing build failures when Paparazzi is not in use.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt#L523-L524
Potential issue: The `SentryUploadSnapshotsTask` is registered if
`extension.snapshots.enabled` is true. However, its required `snapshotsPath` property is
only configured under specific conditions. This leads to build failures in two
scenarios: 1) When the Paparazzi plugin is not applied, the configuration block is
skipped, leaving `snapshotsPath` unassigned. 2) When the Paparazzi plugin is applied but
unit tests are disabled for a specific build variant (e.g., `release`), an early return
in an `afterEvaluate` block prevents `snapshotsPath` from being set for that variant's
task. In both cases, Gradle's task validation fails because a required property is
missing.

Summary
sentrySnapshotplugin into the mainsentry { snapshots { } }extensionenabledflag needed)sentryUploadSnapshotstask to Paparazzi output directory and orderingDSL before
DSL after
sentry { snapshots { includePrivatePreviews = true packageTrees = listOf("com.example.app") theme = "MyTheme" } }#skip-changelog
🤖 Generated with Claude Code