fix: guard getNativeAspectRatioValue against zero denominator#455
fix: guard getNativeAspectRatioValue against zero denominator#455shaun0927 wants to merge 3 commits intosiddharthvaddem:mainfrom
Conversation
The helper performed an unguarded division, returning Infinity, NaN, or negative values when videoHeight, the crop region height, or the numerator was 0 / non-finite. The result is consumed by nine call sites in VideoEditor/VideoPlayback that feed canvas sizing and CSS aspect-ratio, producing 0x0 previews before video metadata loads or when the crop height collapses. Fall back to 16/9 (the same default getAspectRatioValue uses for 'native') whenever the computed ratio is not a finite positive number. Covered by a new vitest suite exercising zero, NaN, negative, and Infinity inputs.
📝 WalkthroughWalkthroughA module-level fallback Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
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. 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.
🧹 Nitpick comments (2)
src/utils/aspectRatioUtils.ts (1)
44-45: Use the fallback constant everywhere (nit: cleaner).good call introducing
NATIVE_ASPECT_FALLBACK; lowkey risky to keep other16/9literals around in the same module because they can drift later. Reuse this constant in the"native"branches too.Suggested diff
export function getAspectRatioValue(aspectRatio: AspectRatio): number { switch (aspectRatio) { @@ case "native": - return 16 / 9; + return NATIVE_ASPECT_FALLBACK; @@ export function formatAspectRatioForCSS(aspectRatio: AspectRatio, nativeRatio?: number): string { - if (aspectRatio === "native") return String(nativeRatio ?? 16 / 9); + if (aspectRatio === "native") return String(nativeRatio ?? NATIVE_ASPECT_FALLBACK); return aspectRatio.replace(":", "/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/aspectRatioUtils.ts` around lines 44 - 45, Some places still use the literal 16/9 instead of the new constant; replace those literals with the NATIVE_ASPECT_FALLBACK constant so the module uses one source of truth. Locate the branches/handlers that check for the "native" mode (e.g., the "native" case in any getAspectRatio or computeAspect* functions) and swap any 16/9 numeric literals for NATIVE_ASPECT_FALLBACK, and also search the module for other 16/9 occurrences to replace them with the constant.src/utils/aspectRatioUtils.test.ts (1)
39-52: Consider extending the pathological loop to include crop-region weirdness.optional but useful: add cases with crop values like
height: Number.NaN,width: Number.POSITIVE_INFINITY,width: 0,height: -1so future edits don’t accidentally re-open that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/aspectRatioUtils.test.ts` around lines 39 - 52, Extend the pathologicalInputs array used in the "never returns Infinity, NaN, or a non-positive ratio" test for getNativeAspectRatioValue to include crop-region edge cases (e.g., [width, height] entries with height: Number.NaN, width: Number.POSITIVE_INFINITY, width: 0, height: -1) so the loop tests these additional invalid inputs; update the array in src/utils/aspectRatioUtils.test.ts where getNativeAspectRatioValue is invoked to include those tuples and keep the same assertions (Number.isFinite(ratio) and ratio > 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/aspectRatioUtils.test.ts`:
- Around line 39-52: Extend the pathologicalInputs array used in the "never
returns Infinity, NaN, or a non-positive ratio" test for
getNativeAspectRatioValue to include crop-region edge cases (e.g., [width,
height] entries with height: Number.NaN, width: Number.POSITIVE_INFINITY, width:
0, height: -1) so the loop tests these additional invalid inputs; update the
array in src/utils/aspectRatioUtils.test.ts where getNativeAspectRatioValue is
invoked to include those tuples and keep the same assertions
(Number.isFinite(ratio) and ratio > 0).
In `@src/utils/aspectRatioUtils.ts`:
- Around line 44-45: Some places still use the literal 16/9 instead of the new
constant; replace those literals with the NATIVE_ASPECT_FALLBACK constant so the
module uses one source of truth. Locate the branches/handlers that check for the
"native" mode (e.g., the "native" case in any getAspectRatio or computeAspect*
functions) and swap any 16/9 numeric literals for NATIVE_ASPECT_FALLBACK, and
also search the module for other 16/9 occurrences to replace them with the
constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acd95585-1039-43cc-8fc7-00d049b900a9
📒 Files selected for processing (2)
src/utils/aspectRatioUtils.test.tssrc/utils/aspectRatioUtils.ts
…nstant Move the NATIVE_ASPECT_FALLBACK constant above its first usage and replace all remaining 16/9 literals in getAspectRatioValue (native case) and formatAspectRatioForCSS with the shared constant so the fallback value has a single source of truth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi, The new crop-related test uses equal crop width/height (0.5/0.5), so the ratio is unchanged and the test would pass even if Severity: action required | Category: maintainability How to fix: Use non-canceling crop values Agent prompt to fix - you can give this to your LLM of choice:
We noticed a couple of other issues in this PR as well - happy to share if helpful. Found by Qodo code review |
…eling values The previous "applies the crop region when provided" case used width/height = 0.5/0.5, which cancels out and returns the same ratio as no crop — the test would still pass even if cropRegion were ignored. Switch to non-proportional 0.75 × 0.5 (expected 8/3) so a future regression in the crop branch actually fails the assertion. Also broaden the pathological-input loop to drive the crop-region branch directly: zero / NaN / Infinity / negative crop dimensions, all of which must still resolve to the 16/9 fallback.
|
@qodo-ai-reviewer Good catch — the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/aspectRatioUtils.test.ts`:
- Around line 42-65: The test currently only asserts finiteness and positivity
for pathologicalInputs in the loop using getNativeAspectRatioValue, which lets
incorrect positive values slip through; change the assertions for these specific
cases to assert the exact fallback 16/9 behavior (e.g., replace the
Number.isFinite and >0 checks with an assertion that ratio equals the fallback
16/9, using a strict or numeric-precision matcher such as toBeCloseTo if needed)
so the pathological-input loop enforces the contract that
getNativeAspectRatioValue returns the 16/9 default for those inputs.
🪄 Autofix (Beta)
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
Run ID: 0dfc1a49-b6c5-4625-92df-cea61f9a77d5
📒 Files selected for processing (1)
src/utils/aspectRatioUtils.test.ts
| it("never returns Infinity, NaN, or a non-positive ratio", () => { | ||
| const pathologicalInputs: Array< | ||
| [number, number, { x: number; y: number; width: number; height: number }?] | ||
| > = [ | ||
| [0, 0], | ||
| [1920, 0], | ||
| [0, 1080], | ||
| [Number.POSITIVE_INFINITY, 1080], | ||
| [1920, Number.POSITIVE_INFINITY], | ||
| [Number.NaN, Number.NaN], | ||
| // Same idea, but exercising the crop-region branch so a future | ||
| // regression there can't slip past the dimension-only cases above. | ||
| [1920, 1080, { x: 0, y: 0, width: 0.5, height: 0 }], | ||
| [1920, 1080, { x: 0, y: 0, width: 0, height: 0.5 }], | ||
| [1920, 1080, { x: 0, y: 0, width: Number.NaN, height: 0.5 }], | ||
| [1920, 1080, { x: 0, y: 0, width: 0.5, height: Number.NaN }], | ||
| [1920, 1080, { x: 0, y: 0, width: Number.POSITIVE_INFINITY, height: 0.5 }], | ||
| [1920, 1080, { x: 0, y: 0, width: 0.5, height: -1 }], | ||
| ]; | ||
| for (const [w, h, crop] of pathologicalInputs) { | ||
| const ratio = getNativeAspectRatioValue(w, h, crop); | ||
| expect(Number.isFinite(ratio)).toBe(true); | ||
| expect(ratio).toBeGreaterThan(0); | ||
| } |
There was a problem hiding this comment.
tighten the pathological-input assertion to enforce the actual contract.
right now (Line 63–64) you only assert “finite + positive”. that still allows incorrect positive values to pass. for these pathological cases, expected behavior is specifically fallback 16/9.
nit: stronger assertion (prevents sneaky regressions)
for (const [w, h, crop] of pathologicalInputs) {
const ratio = getNativeAspectRatioValue(w, h, crop);
expect(Number.isFinite(ratio)).toBe(true);
expect(ratio).toBeGreaterThan(0);
+ expect(ratio).toBe(FALLBACK);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/aspectRatioUtils.test.ts` around lines 42 - 65, The test currently
only asserts finiteness and positivity for pathologicalInputs in the loop using
getNativeAspectRatioValue, which lets incorrect positive values slip through;
change the assertions for these specific cases to assert the exact fallback 16/9
behavior (e.g., replace the Number.isFinite and >0 checks with an assertion that
ratio equals the fallback 16/9, using a strict or numeric-precision matcher such
as toBeCloseTo if needed) so the pathological-input loop enforces the contract
that getNativeAspectRatioValue returns the 16/9 default for those inputs.
2 participants
Description
getNativeAspectRatioValueinsrc/utils/aspectRatioUtils.tsperformed an unguarded division and could returnInfinity,NaN, or negative values. The result flows into nine call sites inVideoEditor.tsxandVideoPlayback.tsxthat use it for canvas sizing and CSSaspect-ratio, so a bad value collapses the preview to 0×0 or produces a broken export.This PR clamps the denominator and falls back to
16 / 9— the same defaultgetAspectRatioValuealready returns for the"native"case — whenever the computed ratio is not a finite positive number. A new vitest suite locks the behaviour in.Motivation
Introduced in v1.3.0 when the
"native"aspect ratio option was added. Two realistic paths hit the bug:<video>element has firedloadedmetadata(videoHeight === 0).0while Native is active.Both cases reproduce with a one-liner against v1.3.0 source:
Type of Change
Related Issue(s)
Fixes #454
Testing
src/utils/aspectRatioUtils.test.ts(8 cases) — covers the normal path, crop-applied path,videoHeight = 0, both-zero,cropHeight = 0,NaNinputs, negative dimensions, and a property-style check that no pathological input returnsInfinity/NaN/ non-positive.npx vitest run src/utils/aspectRatioUtils.test.ts→ 8/8 pass.Manual repro steps for reviewers:
Checklist
Summary by CodeRabbit
Bug Fixes
Tests