test(transitions): add coverage for renderers/gpu.ts pure helpers + registry#258
Merged
Conversation
…egistry The file had no test coverage after the dead-code purge in PR #252. Covers what's testable without a real OffscreenCanvas: - 6 pure helpers (clamp01, smoothStep, getNumericProperty, seededRandom, fadeOpacity, crossDissolveT) — exported and tested for invariants. fadeOpacity's constant-power crossfade is asserted explicitly so a future "just use linear opacity" rewrite fails loudly. - Registry smoke: registerGpuTransitions on a fresh TransitionRegistry registers exactly 15 known IDs, each with renderCanvas and a matching gpuTransitionId. Locks the GPU transition contract — the IDs appear on persisted projects and are looked up by the GPU pipeline (TransitionPipeline) keyed by gpuTransitionId, so adding/removing one is a schema-affecting change worth flagging. Per-renderer pixel tests skipped — jsdom can't run OffscreenCanvas without the canvas npm package, so they'd be mock-call assertions with low signal. 39 tests added. Full suite: 367 files / 2304 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExports six GPU helper functions from ChangesGPU Transition Utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/shared/timeline/transitions/renderers/gpu.test.ts (1)
1-11: ⚡ Quick winUpdate guidance for
src/shared/timeline/transitions/renderers/gpu.test.ts
- Keep
vite-plus/testhere (the repo’s tests consistently use it).- Don’t add an explicit
src/test/setup.tsimport:vite.config.tsalready setstest.environment: 'jsdom'andtest.setupFiles: ['./src/test/setup.ts'].@/*exists intsconfig.json, but nearby transition renderer tests also use relative imports, so this isn’t a correctness issue (optional to align on@/*for consistency).🤖 Prompt for 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. In `@src/shared/timeline/transitions/renderers/gpu.test.ts` around lines 1 - 11, Keep the existing top imports as-is: retain "vite-plus/test" (so the test uses describe/expect/it from that module) and the relative imports of TransitionRegistry and functions (clamp01, crossDissolveT, fadeOpacity, getNumericProperty, registerGpuTransitions, seededRandom, smoothStep); do not add an explicit import of "src/test/setup.ts" because vite.config.ts already configures test.environment and test.setupFiles, and ensure no changes replace the relative module paths with an absolute "`@/`..." alias unless you intentionally want to standardize all nearby tests to that style.
🤖 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.
Nitpick comments:
In `@src/shared/timeline/transitions/renderers/gpu.test.ts`:
- Around line 1-11: Keep the existing top imports as-is: retain "vite-plus/test"
(so the test uses describe/expect/it from that module) and the relative imports
of TransitionRegistry and functions (clamp01, crossDissolveT, fadeOpacity,
getNumericProperty, registerGpuTransitions, seededRandom, smoothStep); do not
add an explicit import of "src/test/setup.ts" because vite.config.ts already
configures test.environment and test.setupFiles, and ensure no changes replace
the relative module paths with an absolute "`@/`..." alias unless you
intentionally want to standardize all nearby tests to that style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8e2401b-8d5f-42d8-b23a-1455687249b2
📒 Files selected for processing (2)
src/shared/timeline/transitions/renderers/gpu.test.tssrc/shared/timeline/transitions/renderers/gpu.ts
Contributor
Greptile SummaryAdds a focused test file for the pure helper functions and registry smoke test in
Confidence Score: 4/5Safe to merge — only The gpu.test.ts — the smoothStep monotonicity assertions near line 75 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[gpu.test.ts] -->|imports| B[gpu.ts pure helpers]
A -->|imports| C[TransitionRegistry]
B --> D[clamp01]
B --> E[smoothStep]
B --> F[getNumericProperty]
B --> G[seededRandom]
B --> H[fadeOpacity]
B --> I[crossDissolveT]
A -->|calls| J[registerGpuTransitions]
J -->|populates| C
C -->|asserts 15 IDs| K[Registry smoke test]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/shared/timeline/transitions/renderers/gpu.test.ts:75-83
**Misleading comment and loose upper bound in "is smooth" test**
The inline comment `// shallower than linear (0.02)` is factually wrong. The derivative of `3t²-2t³` at `t=0.5` is `6t(1-t) = 1.5`, so the finite difference over `[0.49, 0.51]` is approximately `0.02 × 1.5 = 0.03` — *steeper* than the linear `0.02`, not shallower. The upper bound `0.1` is also five times the linear value and lets any function with slope ≤ 5 pass, meaning a near-linear step or a triangle wave at the midpoint would not be caught. As written this test mainly checks monotonicity (which the prior test already confirms) and doesn't protect against a "just use linear" rewrite the PR description says it should guard against.
### Issue 2 of 2
src/shared/timeline/transitions/renderers/gpu.test.ts:184-194
Each `it.each` iteration builds a brand-new `TransitionRegistry` and calls `registerGpuTransitions` for all 15 entries, so the function is invoked 15 times in this single test block alone (on top of the two other `describe` tests in `registerGpuTransitions`). Using a shared `beforeAll` registry avoids the redundant work and matches the pattern used in the surrounding `it` tests that already create a shared registry per describe block.
```suggestion
it.each(EXPECTED_GPU_TRANSITION_IDS)(
'registers "%s" with a renderCanvas method and a matching gpuTransitionId',
(id) => {
// Use the registry already populated by the first test in this describe block.
const registry = new TransitionRegistry()
registerGpuTransitions(registry)
const renderer = registry.getRenderer(id)
expect(renderer, `${id} renderer should be registered`).toBeDefined()
expect(typeof renderer?.renderCanvas, `${id} should have renderCanvas`).toBe('function')
expect(renderer?.gpuTransitionId, `${id} should set gpuTransitionId`).toBe(id)
},
)
```
Reviews (1): Last reviewed commit: "test(transitions): add coverage for rend..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+75
to
+83
| it('handles a zero-width interval (edge0 === edge1) without NaN', () => { | ||
| const value = smoothStep(0.5, 0.5, 0.6) | ||
| expect(Number.isFinite(value)).toBe(true) | ||
| expect(value).toBe(1) | ||
| }) | ||
| }) | ||
|
|
||
| describe('getNumericProperty', () => { | ||
| it('returns the property when it is a finite number', () => { |
Contributor
There was a problem hiding this comment.
The inline comment // shallower than linear (0.02) is factually wrong. The derivative of 3t²-2t³ at t=0.5 is 6t(1-t) = 1.5, so the finite difference over [0.49, 0.51] is approximately 0.02 × 1.5 = 0.03 — steeper than the linear 0.02, not shallower. The upper bound 0.1 is also five times the linear value and lets any function with slope ≤ 5 pass, meaning a near-linear step or a triangle wave at the midpoint would not be caught. As written this test mainly checks monotonicity (which the prior test already confirms) and doesn't protect against a "just use linear" rewrite the PR description says it should guard against.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/timeline/transitions/renderers/gpu.test.ts
Line: 75-83
Comment:
**Misleading comment and loose upper bound in "is smooth" test**
The inline comment `// shallower than linear (0.02)` is factually wrong. The derivative of `3t²-2t³` at `t=0.5` is `6t(1-t) = 1.5`, so the finite difference over `[0.49, 0.51]` is approximately `0.02 × 1.5 = 0.03` — *steeper* than the linear `0.02`, not shallower. The upper bound `0.1` is also five times the linear value and lets any function with slope ≤ 5 pass, meaning a near-linear step or a triangle wave at the midpoint would not be caught. As written this test mainly checks monotonicity (which the prior test already confirms) and doesn't protect against a "just use linear" rewrite the PR description says it should guard against.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+184
to
+194
| it.each(EXPECTED_GPU_TRANSITION_IDS)( | ||
| 'registers "%s" with a renderCanvas method and a matching gpuTransitionId', | ||
| (id) => { | ||
| const registry = new TransitionRegistry() | ||
| registerGpuTransitions(registry) | ||
| const renderer = registry.getRenderer(id) | ||
| expect(renderer, `${id} renderer should be registered`).toBeDefined() | ||
| expect(typeof renderer?.renderCanvas, `${id} should have renderCanvas`).toBe('function') | ||
| expect(renderer?.gpuTransitionId, `${id} should set gpuTransitionId`).toBe(id) | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
it.each iteration builds a brand-new TransitionRegistry and calls registerGpuTransitions for all 15 entries, so the function is invoked 15 times in this single test block alone (on top of the two other describe tests in registerGpuTransitions). Using a shared beforeAll registry avoids the redundant work and matches the pattern used in the surrounding it tests that already create a shared registry per describe block.
Suggested change
| it.each(EXPECTED_GPU_TRANSITION_IDS)( | |
| 'registers "%s" with a renderCanvas method and a matching gpuTransitionId', | |
| (id) => { | |
| const registry = new TransitionRegistry() | |
| registerGpuTransitions(registry) | |
| const renderer = registry.getRenderer(id) | |
| expect(renderer, `${id} renderer should be registered`).toBeDefined() | |
| expect(typeof renderer?.renderCanvas, `${id} should have renderCanvas`).toBe('function') | |
| expect(renderer?.gpuTransitionId, `${id} should set gpuTransitionId`).toBe(id) | |
| }, | |
| ) | |
| it.each(EXPECTED_GPU_TRANSITION_IDS)( | |
| 'registers "%s" with a renderCanvas method and a matching gpuTransitionId', | |
| (id) => { | |
| // Use the registry already populated by the first test in this describe block. | |
| const registry = new TransitionRegistry() | |
| registerGpuTransitions(registry) | |
| const renderer = registry.getRenderer(id) | |
| expect(renderer, `${id} renderer should be registered`).toBeDefined() | |
| expect(typeof renderer?.renderCanvas, `${id} should have renderCanvas`).toBe('function') | |
| expect(renderer?.gpuTransitionId, `${id} should set gpuTransitionId`).toBe(id) | |
| }, | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/timeline/transitions/renderers/gpu.test.ts
Line: 184-194
Comment:
Each `it.each` iteration builds a brand-new `TransitionRegistry` and calls `registerGpuTransitions` for all 15 entries, so the function is invoked 15 times in this single test block alone (on top of the two other `describe` tests in `registerGpuTransitions`). Using a shared `beforeAll` registry avoids the redundant work and matches the pattern used in the surrounding `it` tests that already create a shared registry per describe block.
```suggestion
it.each(EXPECTED_GPU_TRANSITION_IDS)(
'registers "%s" with a renderCanvas method and a matching gpuTransitionId',
(id) => {
// Use the registry already populated by the first test in this describe block.
const registry = new TransitionRegistry()
registerGpuTransitions(registry)
const renderer = registry.getRenderer(id)
expect(renderer, `${id} renderer should be registered`).toBeDefined()
expect(typeof renderer?.renderCanvas, `${id} should have renderCanvas`).toBe('function')
expect(renderer?.gpuTransitionId, `${id} should set gpuTransitionId`).toBe(id)
},
)
```
How can I resolve this? If you propose a fix, please make it concise.Two P2 comments on the new gpu.test.ts:
1. The "is smooth" test had its math backwards in both the comment and
the bound. d/dt(3t²-2t³) at t=0.5 is 1.5, so the central difference
over [0.49, 0.51] is ~0.03 — STEEPER than the linear 0.02, not
shallower as the comment claimed. The upper bound 0.1 was also too
loose to catch a "just use linear" rewrite. Replaced with two
independent assertions:
- eased S-shape: smoothStep(0.25) < 0.25 and smoothStep(0.75) >
0.75 (a linear curve fails both)
- midpoint slope in a tight band [0.025, 0.035] around the
analytical 0.03 (a linear curve at 0.02 or a step at 0 fails)
2. it.each rebuilt the registry on every iteration — 15 redundant
registrations per test block. Hoisted the registry construction to
a shared const inside the describe (the registry is the unit under
test and never mutates, so reuse is safe and matches the pattern
used in the surrounding describe blocks).
39 tests still pass; tsc clean, lint 0/0.
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
1 participant
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 1293-line `shared/timeline/transitions/renderers/gpu.ts` had no test file after the dead-code purge in PR #252. Adds focused coverage for what's testable without a real `OffscreenCanvas`:
Pure helpers (exported and tested for invariants):
Registry smoke test: `registerGpuTransitions` on a fresh `TransitionRegistry` registers exactly 15 known IDs, each with `renderCanvas` and a matching `gpuTransitionId`. The IDs are persisted on projects and are looked up by `TransitionPipeline` keyed by `gpuTransitionId`, so adding/removing/renaming one is a schema-affecting change worth flagging in CI.
What this PR is NOT
Per-renderer pixel tests were skipped intentionally — jsdom can't run `OffscreenCanvas` without the `canvas` npm package, so they'd reduce to mock-call assertions with low signal. The renderers' geometric/pixel correctness is best caught by visual review or browser-environment smoke tests.
Diff shape
Summary by CodeRabbit
Tests
Refactor