Repository navigation
fix(evals): correct skia reference apps and clip-rect-and-path spec - #388
Open
mikolajadamowicz wants to merge 1 commit into
Open
mikolajadamowicz wants to merge 1 commit into
mikolajadamowicz wants to merge 1 commit into
Conversation
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.
Summary
The
skiaeval category was never typechecked against the real@shopify/react-native-skiapackage, since it was never added as a dependency anywhere in the repo. Once added,tscfound four reference solutions that call APIs that don't exist or are called wrong. This PR adds the dependency and fixes all four.What changed
testbench/package.json: added@shopify/react-native-skia@^2.11.2as a dependency, so the skia evals can actually be typechecked and run against the real API surface.clip-rect-and-path): the prompt, requirements, and reference solution were built aroundClipRectandClipPathcomponents. Neither exists in@shopify/react-native-skia. Clipping is done through theclipprop onGroup(<Group clip={rect(...)}>or<Group clip={somePath}>). Rewrote the reference implementation and updated the prompt and requirements to describe the real API.canvas-fill-background) and eval 08 (text-rendering): both destructuredconst { width, height } = useCanvasSize(), butuseCanvasSize()returns{ ref, size: SkSize }, not flatwidth/height. Fixed the destructuring in both reference files.picture-save-restore): calledcanvas.rotate(degrees)with one argument.rotatetakes a required pivot point:rotate(rotationInDegrees, rx, ry). Fixed both call sites to pass(deg, 0, 0).evals/skia/README.md: rule 13 in the best-practice inventory also described the fakeClipRect/ClipPathAPI, and the API coverage table separately listedGroup clip/invertClipas "omitted" even though it's exactly what eval 16 now tests. Fixed both.Why
The skia category was added in #377 with 20 evals and no dependency ever installed to check them against, so the LLM judge scored eval 16 at 100% despite it referencing components that don't exist. Adding the dependency and running
tsc -p evals/tsconfig.jsonsurfaced this immediately, along with three more reference files with real bugs that had been shipping silently.Verification
Clean except for two pre-existing, intentional exclusions unrelated to this PR (
async-state/12,expo-router/01), both of which typecheck failures are the deliberate point of those evals.Also ran
bunx prettier --checkon all touched files.Not included
No CI wiring.
tsc -p evals/tsconfig.jsonstill isn't run automatically anywhere (onlybun run typecheck, which excludesevals/, runs in CI), so a similar bug in this category could ship again without someone running it by hand. Working on a follow-up PR that should apply tsconfig to every eval.