Repository navigation
ci: validate Compose screenshot references on pull requests - #154
Merged
Merged
Conversation
The full build/test matrix stays manual because its emulator jobs are slow, but screenshot validation needs no emulator and no device, so it can guard every PR that changes how the pickers render. This is also the only automatic check that the reference images reproduce on a host other than the one that recorded them. The committed baselines were rendered on macOS and have never been compared on Linux; without this the imageDifferenceThreshold is an untested assumption. Scoped with a paths filter so docs-only PRs do not pay for it, and the android-37 platform is installed because layoutlib takes its fonts from the compileSdk platform directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
sdkmanager has no platforms;android-37 package. compileSdk 37 maps to API level 37.0, published as platforms;android-37.0, so the install step failed with "Failed to find package" and took the job down with it. integration-build-test.yml has the same wrong id in its ABI check and Android jobs, so fix it there too; that workflow is manual-only, which is why nobody had hit it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The HTML report links to images under build/outputs, which the artifact did not include, so a failing run gave you a report full of broken image references and no way to see the actual diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kez-lab
added a commit
that referenced
this pull request
Aug 23, 2026
#154 added screenshot-test.yml, which validates the Compose references on every PR using a macOS runner. This branch had independently added the same task to pr-verification.yml's Linux job, where it fails: all 9 references differ from a Linux render. Measured both, so the split is recorded rather than assumed: | | differing pixels | per-pixel delta | | --- | --- | --- | | Linux x64 runner | 0.164%-0.888%, all 9 fail | exactly 1/255 | | macOS runner | 0 | - | | One digit changed (LocalTime(13, 5) -> 13, 6), same host | 0.189% | up to 225/255 | Linux host noise is 16-90x the configured 0.01% `imageDifferenceThreshold`, and raising the threshold is not an alternative: it counts differing pixels with no per-pixel tolerance, so any value large enough for Linux (>=0.9%) also masks a one-digit regression at 0.189%. What separates them is how much each pixel differs - 1 versus 225 - which the setting cannot express. So pr-verification.yml keeps diff hygiene, the desktop and Robolectric tests, the sample compile, and the ABI gate; screenshot-test.yml keeps screenshot validation on macOS. docs/testing/compose-screenshot-tests.md records the numbers and replaces its earlier claim that real regressions are "far larger" than host noise, which the digit measurement contradicts. Also reverted this branch's android-actions/setup-android v3 -> v4 bump. It was justified as needed to resolve a minor-versioned platform, but screenshot-test.yml installs platforms;android-37.0 on v3 and passes, so the package id was the whole fix and the bump was gratuitous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kez-lab
added a commit
that referenced
this pull request
Aug 23, 2026
* refactor!: shrink the published API surface to what 0.8.0 should freeze Three groups of symbols were reaching consumers without being part of the supported picker API, and the generic wheel API was published twice. Public API: - Remove public `Picker`. `WheelPicker` already covers it: leaving `onSelectedItemChange` empty and handling `onSelectionSettled` reproduces the settled-only contract exactly. `Picker` stays as the internal entry point the composite pickers need, because dependent columns must not observe a value that is still moving. - Delete the `util` package from the public API. `TimeFormat` / `TimePeriod` move to `io.github.kezlab.compose.pickers.time` next to the component that uses them, the six `*_RANGE` lists become private defaults inside `PickerDefaults` (their only call site), and `currentYear`, `currentMonth`, `currentHour`, `currentMinute` are removed - nothing in the library, sample, tests, or docs called them. - `currentDate` / `currentDateTime` move to the root package and take an explicit `timeZone` defaulting to the system zone. They previously hard-coded `TimeZone.currentSystemDefault()` with no way to override it, which is wrong for pickers that represent time in a fixed or user-selected zone. - Stop generating the Compose resources `Res` class. The module has no `composeResources` directory, so `publicResClass = true` was publishing an empty `Res` object plus five `all*Resources` collections into the klib ABI. Published artifact: - `compose.ui.tooling` moves to `debugImplementation` and the redundant `androidx.ui.tooling.preview` dependency is dropped. `androidMain` has no Kotlin sources at all - the `@Preview` composables live in `commonMain` and compile against the multiplatform `ui-tooling-preview` - so the Android Studio preview renderer was being published as a runtime dependency of the release artifact for no reason. - Drop the unused `kotlinx-serialization` plugin, `binaries.executable()` from the `wasmJs` library target, the empty per-target dependency blocks, and the `release` build type whose `proguardFiles` pointed at a `proguard-rules.pro` that does not exist and would not apply to consumers anyway. - Render picker items with `BasicText`. `itemTextStyle` already carries the interpolated color and font size, so material3 `Text` only re-derived values the picker had resolved. material3 itself stays: `PickerDefaults` reads `LocalContentColor` / `LocalTextStyle` so defaults follow the host MaterialTheme, and foundation has no replacement for that. Removed the leftover empty `com/kez/picker` directories and the `jsMain` source set, which has had no matching Kotlin target. Verified with :pickers:checkKotlinAbi (dumps updated), :pickers:test, :pickers:testDebugUnitTest, :screenshot-tests:validateDebugScreenshotTest after a clean (9 references unchanged, so the BasicText swap is pixel-identical), compilation of every target plus :sample on Android/Desktop/Wasm, and git diff --check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * build: enforce explicit API mode in :pickers Kotlin's explicit API mode makes every public declaration spell out its visibility and return type, so a new declaration can no longer become part of the published API just by defaulting to public. The committed ABI dumps catch that after the fact; this catches it at compile time. The diff is mechanical: 236 declarations across 19 files gain the `public` keyword and nothing else. `:pickers:checkKotlinAbi` passes against the dumps from the previous commit, which confirms no visibility actually changed. Verified with :pickers:checkKotlinAbi, :pickers:test, :pickers:testDebugUnitTest, and compilation of every target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: gate every pull request on the fast verification subset The full matrix in integration-build-test.yml is too slow to run on every push, so PR automation was commented out entirely. That left no hosted gate at all: a PR could change the public API and merge without `checkKotlinAbi` ever running. pr-verification.yml runs the subset that must never regress and finishes without an emulator: diff hygiene, `:pickers:desktopTest`, `:pickers:testDebugUnitTest`, `:screenshot-tests:validateDebugScreenshotTest`, `:sample:compileDebugKotlinAndroid`, and `:pickers:checkKotlinAbi`. The ABI job runs on macOS because the klib dump covers the iOS targets and cannot be produced on Linux. integration-build-test.yml stays `workflow_dispatch`-only for hosted iOS, Wasm, and managed-device evidence. AGENTS.md records which gate is which. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: publish a real API reference instead of an empty javadoc jar The library documents its public API thoroughly in KDoc, and none of it reached anyone: without Dokka, vanniktech's javadoc jar was 261 bytes containing only a manifest, so Maven Central had no documentation and IDE "download documentation" produced nothing. Applying Dokka 2.2.0 makes vanniktech fill the javadoc jar from the real KDoc (1.4 MB), and the same HTML now ships to GitHub Pages under /api/ next to the Wasm demo, so the API reference has a stable public URL. The generated reference lists exactly the four supported packages - root, date, duration, time - which is a second, independent confirmation that the util and resources packages are gone from the public API. Verified with :pickers:publishToMavenLocal (javadoc jar populated) and by assembling the Pages site locally from :sample:wasmJsBrowserDistribution plus :pickers:dokkaGeneratePublicationHtml. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: document the 0.8.0 API changes, stability promise, and upgrade path 0.8.0 is a hard breaking release with no compatibility layer, and until now the only description of it was a one-line README note about the package rename. - CHANGELOG gains the four breaking entries from this release plus the artifact and tooling changes, each with the reason the symbol was removed rather than just the fact that it was. - docs/migration/0.7-to-0.8.md maps every rename and calls out the one change a mechanical find-and-replace gets wrong: `WheelPicker.onSelectedItemChange` fires live, so code moving off `Picker` must handle `onSelectionSettled` instead. - docs/product/api-stability-policy.md states what counts as public API (the committed ABI dumps, nothing else), what 0.x versus 1.0.0 promises, the deprecation procedure, and which platform and toolchain changes count as breaking. The repository had no such statement, which is a problem for a library heading toward 1.0. - Both READMEs drop every reference to the removed `Picker` and `util` package, gain a Dependencies section explaining why material3 is transitive and why ui-tooling is not, and link the API reference, migration guide, and stability policy. - AGENTS.md and RELEASING.md record the new durable rules: explicit API mode, keeping debug-only dependencies out of the POM, checking the generated POM after dependency changes, and why `Picker` must stay internal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: split the PR gate so screenshots keep their macOS runner #154 added screenshot-test.yml, which validates the Compose references on every PR using a macOS runner. This branch had independently added the same task to pr-verification.yml's Linux job, where it fails: all 9 references differ from a Linux render. Measured both, so the split is recorded rather than assumed: | | differing pixels | per-pixel delta | | --- | --- | --- | | Linux x64 runner | 0.164%-0.888%, all 9 fail | exactly 1/255 | | macOS runner | 0 | - | | One digit changed (LocalTime(13, 5) -> 13, 6), same host | 0.189% | up to 225/255 | Linux host noise is 16-90x the configured 0.01% `imageDifferenceThreshold`, and raising the threshold is not an alternative: it counts differing pixels with no per-pixel tolerance, so any value large enough for Linux (>=0.9%) also masks a one-digit regression at 0.189%. What separates them is how much each pixel differs - 1 versus 225 - which the setting cannot express. So pr-verification.yml keeps diff hygiene, the desktop and Robolectric tests, the sample compile, and the ABI gate; screenshot-test.yml keeps screenshot validation on macOS. docs/testing/compose-screenshot-tests.md records the numbers and replaces its earlier claim that real regressions are "far larger" than host noise, which the digit measurement contradicts. Also reverted this branch's android-actions/setup-android v3 -> v4 bump. It was justified as needed to resolve a minor-versioned platform, but screenshot-test.yml installs platforms;android-37.0 on v3 and passes, so the package id was the whole fix and the bump was gratuitous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
변경 사항
.github/workflows/screenshot-test.yml을 추가해 PR에서:screenshot-tests:validateDebugScreenshotTest를 실행합니다.배경
전체 빌드/테스트 매트릭스(
integration-build-test.yml)는 에뮬레이터 잡이 느려서 의도적으로workflow_dispatch전용으로 유지합니다. 이 정책은 그대로 둡니다.다만 스크린샷 검증은 에뮬레이터도 기기도 필요 없고 로컬 실측 ~20초라, 피커 렌더링을 바꿀 수 있는 PR을 매번 지키기에 충분히 쌉니다. 매트릭스와 분리된 별도 워크플로로 추가한 이유입니다.
더 중요한 이유가 하나 있습니다. #153의 기준 이미지 9장은 macOS에서 기록됐고, Linux에서 재현되는지 한 번도 확인된 적이 없습니다. 이 검사가 없으면
imageDifferenceThreshold = 0.0001f은 검증되지 않은 가정으로 남습니다. 이 PR 자체가 그 첫 검증이 됩니다.설계 메모
paths필터로pickers/**,screenshot-tests/**,gradle/**,gradle.properties,settings.gradle.kts, 그리고 이 워크플로 자신에만 반응합니다. 문서 전용 PR은 비용을 내지 않습니다.compileSdk플랫폼 디렉터리의 폰트로 텍스트를 렌더링하므로platforms;android-37설치가 필요합니다.paths필터로 잡이 실행되지 않아도 머지가 막히지 않습니다.검증
actionlint는 로컬 미설치)git diff --check✅